I learned recently that aria-label doesn't do anything on elements that don't have a role: https://gomakethings.com/aria-label-requires-a-valid-role/

That's correct but it's worth reminding the <dialog> element does have e native role. As such, it is exposed in the browser's accessibility tree as role=dialog. Adding an aria-label does work as expected. Screenshot illustrating how manually adding an aria-label on the MDN demo does work:

Thanks for highlighting this. The attribute was already here but I did move the code for it—it was here. Now this element is wrapped in the dialog so I’d removed the role and aria-modal attributes. I suppose this label should move to the dialog element.

I moved the aria-label to the dialog element in 01643fa. This could still use some scrutiny. The label is applied only when the overlay is open. That’s how it was yet now the overlay can open without JS and the label won’t be applied. I think the idea behind conditionally applying it is to avoid its presence when the navigation has no overlay—i.e. in larger viewports. With this PR, the dialog element is there regardless of the viewport size and perhaps it’s not wrong for it to have the label even when it’s not acting as a modal.

I think it would be ideal if this class toggling wasn't required. Namely, the CSS rule could check if body:has(dialog.wp-block-navigation__responsive-container:open) instead. Then this would not require any JS to apply the styling.

Namely:

and

html.has-modal-open can be replaced with html:has(dialog.wp-block-navigation__responsive-container:open).

And I suppose .has-modal-open & can be replaced with html:has(dialog.wp-block-navigation__responsive-container:open) &

I think it would be ideal if this class toggling wasn't required.

I’d agree yet I kept the classes in accord this part of the BC guidelines:

Class names and DOM nodes used inside the tree of React components are not considered part of the public API and can be modified.

Changes to these should be done with caution as it can affect the styling and behavior of third-party code (Even if they should not rely on these in the first place). Keep the old ones if possible. If not, document the changes and write a dev note.

Thanks to your suggestions though I realized I missed adding a no-JS equivalent selector for this one:

Ah, got it. Ok, I think we should then keep the class name for now, but we can add a comment to say it is deprecated, and we can write a dev note to comment this. I'll do a search for themes in the directory to see if anyone is targeting this class name so we'll have a list for outreach.

There are 132 themes that are using this class name, at least at the time that WPdirectory last indexed the directory: https://wpdirectory.net/search/01JQQ5VY70TAAEMSXTAQVE69GW

Even were these class names to be kept indefinitely it would be nice to do outreach that recommends updating any selectors using them because otherwise the theme’s styles won’t apply if JS is unavailable.

This tests focus wrapping, i.e. the focus moving either to the first tabbable to the last or vice versa. Since this PR removes the logic for this, focus doesn’t wrap (and tends to move into the browser’s UI). Yet thanks to the modal dialog nothing in the document outside the modal can be focused. I think that suffices and seems more accessible because focus isn’t trapped within the document by wrapping it. I just wanted to highlight this in case someone thinks differently.

I’ll go ahead and push a commit that removes these commented lines.

This isn’t necessary but should introduce no behavioral differences. The type === 'submenu' conditions were useless because these handlers are only invoked by submenus (not the overlay). The .filter( Boolean ).length === 0 is basically a more verbose version of .some( Boolean ) which replaces it. Other than that this just revises the comments a bit.

The overlay (dialog) is now in the top-layer and the admin bar is unable to appear in front of it so these styles are irrelevant.

I had removed these because these properties were set to inherit otherwise and I didn’t realize the !important bit was key. I double-checked and found that out. Removing them hadn’t caused the e2e tests for colors to fail so along with restoring this I added an e2e test that fails without these.

Shouldn't this be :open as opposed to [open]? I consulted Gemini who suggests they are functionally equivalent, but then says:

:open is often preferred: It's semantically clearer as it targets the state of the element (being open) rather than the implementation detail (having the open attribute). This aligns conceptually with other state pseudo-classes like :checked, :disabled, or :focus.

It's also 1 character shorter 😄

Thanks! I agree :open should be used. Done in 370b204. Our style lint doesn’t like it but ignores work for now.

It turns out Safari doesn’t yet support :open and that was causing most of the recent e2e test failures. Even besides Safari the versions of Chrome and Firefox that don’t support it are still at usage levels above %1 which means we shouldn’t use it yet. So 966098f switches back to [open].

these changes seem unrelated? (thru line 330)

Yes these are not directly related. This adds coverage for something I broke while trying to clean up some styles. It’s possible that one of the existing tests could be modified so it would fail instead of adding this new one but I figured this is more explicit.

Does this test no longer work or apply?

This test duplicates the test above it. As the comment preceding it indicates, the intention was to avoid flakiness of the test in Safari purportedly caused by Tab key presses. I removed those parts of the test so I followed up with the rest of what the comment recommends.

The reason I removed the assertions for Tab key and focus is because the “focus trapping“ implementation is removed and testing for it is not needed. The “focus trapping” can be removed because the dialog element, as a modal, makes all content outside it inert. Edit: also commented here

Aside: this test was a complete duplicate of the one above it and included Tab key presses so I don’t see how it was actually making any difference for Safari in the first place.