Modal functionality #76

[TomaszDziezykNetcentric]

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #76

Test URLs:

• Before: https://main--vg-volvotrucks-us-rd--netcentric.hlx.page/drafts/tdziezyk/v2/modal
• After: https://76-modal--vg-volvotrucks-us-rd--netcentric.hlx.page/drafts/tdziezyk/v2/modal

[aem-code-sync]

Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

• Re-run PSI Checks

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[cogniSyb]

Add a ? to modalContent.querySelector('iframe, video').remove(); just like on line 123/124

[Lakshmishri]

I see some console errors on closing the modal

[cogniSyb]

I’m missing the possibility to add text to the modal. It’s not documented well:

You can see it in use on Digital Reveal.

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[TomaszDziezykNetcentric]

I see some console errors on closing the modal

Done

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[TomaszDziezykNetcentric]

I’m missing the possibility to add text to the modal. It’s not documented well:

You can see it in use on Digital Reveal.

I added the heading. I'm checking if the first section of the modal document contains a heading only. If so, the heading is displayed as a modal heading.

[cogniSyb]

I added the heading. I'm checking if the first section of the modal document contains a heading only. If so, the heading is displayed as a modal heading.

Nice solution. I’m noting it for documentation.

[cogniSyb]

Some accessibility things should be worked on:

• Allow users to close the window with the ESC key
• The <div> containing the modal should be inaccessible if it’s closed with aria-hidden
• If the modal has a visible title, use the ARIA property aria-labelledby to associate the visible title with the modal container (the element that has role=dialog). Otherwise we can use aria-label with some standard text on the element that has role=dialog
• When the modal dialog opens, keep and store a reference to the UI control (likely a button) that opened the modal; we’ll call this the “invoking context”
• when the modal closes, either by pressing the ESC key or pressing the Close button, focus should be moved back to the “invoking context” previously stored
• “trap” focus within the dialog: interaction with the rest of the page shouldn’t be allowed; when pressing the Tab key within the modal and the last focusable element is reached, focus should be moved to the first focusable element and vice-versa. We can utilise inert here.

[cogniSyb]

I see some console errors on closing the modal

Done

Still experiencing this, but maybe it’s a similar error elsewhere:

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[TomaszDziezykNetcentric]

Some accessibility things should be worked on:

* Allow users to close the window with the ESC key

* The `<div>` containing the modal should be inaccessible if it’s closed with `aria-hidden`

* If the modal has a visible title, use the ARIA property `aria-labelledby` to associate the visible title with the modal container (the element that has `role=dialog`). Otherwise we can use `aria-label` with some standard text on the element that has `role=dialog`

* When the modal dialog opens, keep and store a reference to the UI control (likely a button) that opened the modal; we’ll call this the “invoking context”

* when the modal closes, either by pressing the ESC key or pressing the Close button, focus should be moved back to the “invoking context” previously stored

* “trap” focus within the dialog: interaction with the rest of the page shouldn’t be allowed; when pressing the Tab key within the modal and the last focusable element is reached, focus should be moved to the first focusable element and vice-versa. We can utilise [inert](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert) here.

Done

[cogniSyb]

@TomaszDziezykNetcentric another –major– concern I have is the fact that blocks break in the modal due to a missing <main> element. Not only is this a generic dependency, it’s also specific in some blocks. I’ve edited the modal in your drafts folder to make the issue visible.

I thought of some solutions directions…

1. Using multiple <main> elements with a hidden attribute is an approach to align with our existing CSS dependencies, but it might not be the best practice semantically or for accessibility.
2. Including the content in the page <main> element raises the same concerns as mentioned above.
3. Rewriting the CSS to use a class would be tricky, as it will impact specificity: main img has a value of 0-0-2, .main img scores 0-1-1. Maybe rewriting the CSS to scope body img would work here. Apart from feeling a bit silly, we would have to deal with regression in the header and footer blocks, and maybe third-party content like OneTrust.
4. Since I saw <div role="dialog"> being used, how much of a hassle would it be to switch to <dialog> and expand our CSS selectors? We’d keep semantics/accessibility for modern browsers. However, there are still some bugs in browsers. Not sure whether to take that risk.
5. Looking at the previous solution direction, the approach of expanding our CSS selectors could still work. If we maintain our current markup, we could expand a selector main img with div:where([role="dialog"]) img. The result has the same specificity.

I guess that the latter item has the least impact. What do you think? I would like to hear @Lakshmishri’s opinion about this as well

Edit: I’m unsure how it will impact our JS, didn’t look into that yet.

[TomaszDziezykNetcentric]

@TomaszDziezykNetcentric another –major– concern I have is the fact that blocks break in the modal due to a missing <main> element. Not only is this a generic dependency, it’s also specific in some blocks. I’ve edited the modal in your drafts folder to make the issue visible.

I thought of some solutions directions…

1. Using multiple `<main>` elements with a `hidden` attribute is an approach to align with our existing CSS dependencies, but it might not be the best practice semantically or for accessibility.

2. Including the content in the page `<main>` element raises the same concerns as mentioned above.

3. Rewriting the CSS to use a class would be tricky, as it will impact specificity: `main img` has a value of 0-0-2, `.main img` scores 0-1-1. Maybe rewriting the CSS to scope `body img` would work here. Apart from feeling a bit silly, we would have to deal with regression in the header and footer blocks, and maybe third-party content like OneTrust.

4. Since I saw `<div role="dialog">` being used, how much of a hassle would it be to switch to `<dialog>` and expand our CSS selectors? We’d keep semantics/accessibility for modern browsers. However, there are still [some bugs in browsers](https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element?label=stable&label=master&aligned). Not sure whether to take that risk.

5. Looking at the previous solution direction, the approach of expanding our CSS selectors could still work. If we maintain our current markup, we could expand a selector `main img` with `div:where([role="dialog"]) img`. The result has the same specificity.

I guess that the latter item has the least impact. What do you think? I would like to hear @Lakshmishri’s opinion about this as well

Edit: I’m unsure how it will impact our JS, didn’t look into that yet.

The problem doesn't sound like a simple one :(
No matter which solution we select there will be a lot of testing required...

I think the last solution (replacing main img with div:where([role="dialog"]) img ) is the simplest for the code we have. I'm wondering if we need to fix all the blocks or if should we just update the ones that are in the designs modal. It could shorten the testing.

BTW IMO we should solve those problems in the scope of another ticket. The scope of this story was to display a modal top bar and was estimated to be 2 SP without testing (we assumed that it is the POC) :)

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/tdziezyk/v2/modal