Consider making 'detail page' a modal window instead of a separate "page" in app

I think not need Popup at all, since user don't lost pagination/filtration/ scroll user to down when they click back.

Want to point out this solves some other issues around proper page titling / H1. When on a separate "page" the H1 really should be the module's title if you're on that page. Serving it on a modal makes it clear you're still in "Browse projects" and just getting some additional info about something. This may also make it easier to communicate back-and-forth between the 'add and install' on the detail page and the underlying buttons on the card.

kwiseman → changed the visibility of the branch 1.0.x to hidden.

gslexie → changed the visibility of the branch 3310908-consider-making-detail to hidden.

gslexie → changed the visibility of the branch 3310908-consider-making-detail to active.

There's been a lot of discussion about backend internals and they further support the idea that this should be a modal. I'm no longer undecided. Anyone who wants to work on the code, please do!

kwiseman → changed the visibility of the branch 2.0.x to hidden.

I started working on this for 2.0.x. Given that there are some issues with the modal right now and 📌 Update project detail page layout & elements Needs work , it's still a work in progress. I copied ModulePage.svelte into the new file DetailModal.svelte to make edits so that simultaneously working on this issue and the one above is (hopefully?) easier.

I've tested the 3310908-2x-make-detail-modal branch. A few thoughts:

even though it is not applied consistently in core yet. but a dialog modal should have an h1 title. at the moment the title in the titlebar is wrapped just in a span, i wonder if the cleanest approach would be making the title in the titlebar the h1 and remove the title currently wrapped in an h2 in the main content area of the dialog modal. and it has to be noted that with drupal 11 and the jquery ui 0.14 there will be finally a fix for screenreader users in dialog modal by adding an aria-modal attribute as a short term fix. that way it is even more important to have an h1 within a dialog modal since the DOM elements in the background of the dialog modal are finally excluded from the AOM now. james scholes from PAC left a a comment explaining the ins and outs of the h1 in the context of dialog modals topic pretty well on that thread https://github.com/mui/material-ui/issues/34250#issuecomment-1244006106 (but there are many more to name)

at the moment the modal can be closed by clicking the close button or pressing the esc key. i wonder if it would make sense to close the modal also when the user is clicking outside of the dialog modal as seen in this example from deque as well: https://dequeuniversity.com/library/aria/simple-dialog

another detail to note, at the moment when a dialog modal is opened the "compatible" button is getting into focus as the first element. the screenreader (voiceover) announces "compatible, button" and then "this module is compatible with your version of drupal. you are currently on a button inside a web dialog. to click this button press control option space". if i do so, without the visual context available, i would expect i would be able to click this compatible button, but nothing is happening. me as a blind user would feel lost. so the question is does it make sense to auto focus the first focusable element in this details page dialog modal at all and if a focus should happen would there be another, better choice? and in general the question, is a button the correct element for these informal icons at all?

in regards of the width of the dialog modal, at the moment the max width is at 800px. i wonder if it would make sense to make the maximum width of the dialog modal a bit wider. cuz with a two column layout within the modal the content of both columns looks a bit squeezed. and at the same time on widescreen monitors, 800px leaves a lot of unused screen real estate:

if you open a dialog modal in views the modal is wider (it uses 75% instead of a fixed value).

but that width used for views would be too wide for project browser (that way you would again get these long line lengths). at the moment the dialog modal in project browser has a max width of 50rem, how about going with 80 or 90rem. that way the columns wouldnt be that squeezed but the line lengh would be also acceptable and not too long.

and @chrisfromredfin, have you made a backup copy of the invision board for the priority guides for the details page in phase 1 and phase 2 (moving to the dialog modal) from back then (where the results from that questionaire were aggregated)? cuz invision went out of business if i a remember correctly?

I've taken another look last night, the point about the html element used on the title as well as the behavior for closing a dialog modal is all outside of project browser. i will open up an issue for core in general. so the only detail left to do might be to increase the width of the dialog modal in project browser (my suggestion would be 80 or 90rem). aside that this issue looks good to go?

@rkoller Thanks for your feedback - I definitely agree with all your suggestions. While the HTML for the modal's title and its closing behavior are out of scope for project browser, the "compatible" button being the first focused element might not be. I'm working on this today and will increase the modal's width as well as test with a screen reader (Voice Over).

I like how this issue and its PR simplify the current workflow.
My only concern is this removes the ability to have multiple tabs open, which becomes useful when looking into multiple competing modules.
However, if one can queue the modules in a shopping cart of sorts which I heard may be in the future then that would not matter much I suppose.

Since the modal is inside the tab, I think having multiple tabs open should be fine, unless I'm missing something.
I'm not sure if right-click->open in new tab will work when opening a modal. Will test later.

Opening the Browse page in different tabs and opening different modules' modals simultaneously works for me.

I should also mention that there are a few blockers preventing this issue from moving forward:

1. It feels like a bad idea to create more modals when Project Browser doesn't remove a modal's HTML after it's closed: 🐛 Remove Modals after They're Closed Fixed
2. I haven't figured out how to add target="_blank" to links within a module/recipe's description in its modal. I don't think that can be done upon initial page load because the HTML for a modal doesn't exist until the title is clicked. It would be nice if the target attribute could be added to those links when the information is first fetched from drupal.org, (i.e. within the PHP of DrupalDotOrgJsonApi.php and Recipes.php).
3. Trying to reuse the code in popup.js (which uses Drupal's dialog function) made it feel like I had to use new to create modals because I can't pass in a svelte component. The linter didn't like the new keyword, so I assigned the modal to a variable, which the linter also doesn't like because it's unused.

Just a thought—why not keep both options, the current 'detail page' and a 'modal window'? We could set it up so that one is the default, and site builders can choose whichever works best for them.

@preshant.c the main problem with the current detail page is that it is not meeting with SC 2.4.2 which is an A level criterion (https://www.w3.org/WAI/WCAG21/Understanding/page-titled.html) and the minimum requirement for drupal core is a WCAG double A compliance. in addition to that you also have the problem of WCAG SC 1.4.8 (https://www.w3.org/WAI/WCAG22/Understanding/visual-presentation.html). On widescreen monitors you get extra long line lengths and a lo. The goal should be around 80 characters, which the dialog modal solution would fix as well, if the modal width gets limited.

Thank you @rkoller for the detailed clarifications :)

kwiseman → changed the visibility of the branch 3310908-consider-making-detail to hidden.

I made a new branch for a few reasons:

1. Accommodate changes to the module detail page's layout in 📌 Update project detail page layout & elements Needs work and the fix for 🐛 Remove Modals after They're Closed Fixed .
2. Styling from the commands modal was interfering with buttons and img elements in the new project detail modal.
3. I hid the "View Commands" button in the project detail modal to avoid opening a modal within a modal. However, the "Queue" button will show if Project Browser is allowed to install modules.
4. The HTML elements around a project's summary and description had the same id and are both displayed on a module's details page. I changed the id of the project summary's wrapper in the modal.

The modal is the same width (90vw), and the links in a project's description still open in a new tab. There are tests for the detail page in ProjectBrowserUiTest.php, ProjectBrowserUiTestJsonApi.php, ProjectBrowserPluginTest.php that will need to be rewritten.

kwiseman → changed the visibility of the branch 3310908-2x-make-detail-modal to hidden.

Until I used this I underestimated what a HUGE usability win it was. I would love to get this in! If someone can look at the tests, that would be amazing forward progress. I will bring this up in the next meeting.

I did some work on the ESLINT issues.

It didn't like assigning DetailModal to a variable, and then not using it again, but it also didn't like creating it with new and not assigning a variable.

I think it's probably better assigning an unused variable, because it looks a bit more normal. I've added // eslint-disable-next-line no-unused-vars to keep eslint quiet.

However, I wonder is that the best approach?

I've also found a fairly significant bug. If you open the detail modal for a project, then try to open it again, it fails to reopen, and for some reason, also changes the background colour of the Project Browser page to grey. You can still open other project detail pages, but once you've opened a detail page once, you can't open it again without a page refresh.

the pattern @lostcarpark discovered can be pushed even further. after you've clicked for example the token module leading to that grey background. you then click another module for the first time, after you close that modal again and try to open/click that other module also a second time the background. You can proceed that pattern with any other module card. every time the background gets even darker on every second try for a module card. looks like transparent background get added onto each other..

and i've also tried when the greyish background shows up for the first time to try to press esc in case that is some sort of "weird" modal instance and that grey background is some sort of rudiment of a modal. but pressing esc had no effect at all and can be ruled out.

and in regards of the modal width as mentioned in the corresponding discussion on slack i would go with a max width between 80 and 90rem instead of 90vw. i have no strong opinion for or against anything inbetween 80 to 90rem, any of those values would work. my problem with 90vw, on widescreen monitors the line length becomes way too long.

I believe I've identified the cause of the problem that was causing the modal dialogs for each module fail to open a second time.

In Project.svelte the <div> for each modal was being created when the component initialized.

In the handler for the module title, the <div> was being opened in a modal.

In the close handler, the element was being destroyed.

When the title was clicked again, the element no longer existed, so it couldn't be displayed in the modal.

The fix is to move the creation of the <div> element into the click handler for the module card title. This allows it to be recreated every time the title is clicked.

This also means the element isn't created until it's needed, or never if the titles aren#'t clicked, removing some processing from the page load.

one detail i forgot, whichmight be reasonable to add within the scope of this issue, it would be good to add an aria-haspopup="dialog" attribute to the link opening the dialog modal on each of the module cards (see haspopup.mp4 - added that just via the safari web inspector for that single element)

Added "max-width" of 80rem to project-browser-popup class.

Added aria-haspopup="dialog" to the project button.

All tests now passing.

I think this is ready for review.

awesome thank you! only minor details. i've tested on an instance that has the composer install deactivated. in that context i would have two questions. would it make sense to make the dialog modal opened by the view commands button the same width like for the detail page? or will that dialog modal be removed anyway? and the other detail for the view commands button, i would still add the aria-haspopup attribute there as well not sure if it makes sense to open a new issue for a single line and if it wouldnt be the better choice to add it within this issue as well. .

i've then retested another time and opened and closed a details dialog modal several times in a row, by the close button as well as by pressing the esc key. all fine. and the max-width of 80rem is also good. also tested narrowing down the browser window to a mobile viewport and enlarging it again afterwards, all behaving well and as expected (tested in safari, firefox and edge) .

there is only one detail i wanted to note and one issue i ran into but am unable to reproduce. by default the first focusable element of the dialogue modals main content is getting into focus. problem is some projects dont have any links in their description so the first element in focus is the view commands/queue/add to batch button. so screenreader users without the visual context "might" miss the description, plus the first focusable element would be the install or the install instructions. usualy that should be the last step after a user has informed themeselves and got convinced the module is the one they wanted to test or have - but from my perspective this is not necessarily about moving the details page into a dialogue modal but more about how the details page is structured so i would vote for taggling that detail in a dedicated followup issue cuz the details page needs some work anyway imho.

the other detail was related to the problem i'Ve outlined before and i went ahead and simply tried VO-a for announcing the entire pagesequentially. problem was, voiceover after finishing the content for the first time jumped right outside of the dialogue modal and announced, i "think" it was the categories from three different modules, and then jumped right back into the dialog modal again. not reproducible so far so i wouldnt hold back the issue because of that.

so from my point of view the only questions left to answer within this issue are are about the view commands dialogue modal. should those details dealt within this issue or moved to a followup? aside that everything looks good to go from my perspective if there arent any objections on the code end.

I had a look at the "View commands" popup, and it comes from a different component. I feel that either changing the width or adding an aria tag would be outside the scope of this issue.

I had one more thought this morning, would it be worth creating a new test to open the modal, close it, and open it again, so that the use case of reopening the modal details is in an automated test?

sounds reasonable, then i'll open a followup issue and postpone it until this one is in.

i think that would be a reasonable addition to make sure that this problem isnt reintroduced and slips in at one point in the future by accident.

I've added a test to simulate opening a modal detail page for a module, closing it, opening then closing the detail for a different module, and finally reopening the first module's modal.

All tests are passing.

Some minor stuff, and there are out of scope changes that should probably be reverted. But overall I don't see any major problems with this code.

Hopefully ready for review again.

One issue I noticed is that clicking the Queue button in the popup does not change it to Dequeue.
This issue also exists in 2.0.x on going to detail page, but as we are removing details page with popup in this issue, I think it should be addressed here.

@narendrar - if it already exists on 2.0.x then would it be so bad to make it its own separate issue so this can go in?

But first of all, do you believe it's ready if ModulePage is removed and App is "refactored" (in what way does it need to be refactored?)

if it already exists on 2.0.x then would it be so bad to make it its own separate issue so this can go in?

Agreed, we can make it conditional such that if the package manager is available, the queue button is not shown, with a @todo to address it in a new issue.

in what way does it need to be refactored?

I think as we are not using ModulePage now(moved in popup), any code related to it can be removed from App.

Rebased to include latest changes. As the number of commits in this issue has grown, the rebase is taking quite a long time, so hopefully we can avoid too many more!

I've removed ModulePage, and stripped everything relating to it out of App.svelte. It makes it quite svelte!

A possible future enhancement would be to make the pop-up update the address bar and give the detail modal a unique URL.

I agree with Chris, the Queue/Dequeue issue is no also present on the existing 2.0.x branch, so not caused by this change, so I think it should be dealt with by a follow up issue.

I think we are ready for another review again...

Created 🐛 [PP-1] Queue button is not working in modal Active

This issue may require some accessibility review. Feel free to remove tag if this is not the case.

I've checked the CSS styles, and as far as I can tall all the pb-module-page styles are still required for the Modal Detail page. However, I have renamed them to pb-detail-modal, which seems more appropriate.

I have reviewed this, and have blessings on the code from above (Adam, Naren), and I've manually tested it, including navigating with keyboard. I think it's time to ship it!

With that said, I have opened two child issues pursuant to Ralf's concerns in #39.

There's also the issue of whether or not the View Commands modal itself should open at max-width 80 like we did here or not. I'm not convinced it needs to, but will leave that for someone else to file the issue if they disagree.

Automatically closed - issue fixed for 2 weeks with no activity.