Fix <Modal> component so it is possible to use the aria.labelledby prop

[p-jackson]

Description

Fixes a bug where <Modal> wasn't forwarding the aria.labelledby prop to <ModalFrame> if the title prop was missing. However the purpose of labelledby is to allow modals to provide their own title element, in which case they won't want to use the title prop.

The aria.labelledby prop is now correctly forwarded to <ModalFrame> if no title is provided. The title prop will take precedent over aria.labelledby.

How has this been tested?

Added new unit tests that check the modal aria props are set correctly in the DOM.

I tested this in a dev environment by changing the preferences modal to render its own heading itself rather than use the title prop.

Inspecting in the Firefox a11y tools, the modal's name is still listed as "Preferences".

Dev's will still need to test how their specific modal behaves in a screen reader as the DOM ordering could still impact the user experience. This PR just makes it possible to set the aria-labelledby attribute where it wasn't really possible to do so before.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[autumnfjeld]

Very nice to see that checklist in the PR description. We should do more of that. 🥇

[tellthemachines]

The code looks good, and thanks for adding tests 😺 , but I'd like to know more about the use case for an explicit aria-labelledby prop. (I know it was already there and not working properly, but can't find the history on why it was added.)

For most cases, we are encouraged to use the modal title, which the dialog aria-labelledby will point to by default.

In cases where the modal can't have a title for some reason (the welcome modal is an example), we have the contentLabel prop, that applies an aria-label to the dialog.

Is there any case where we would want the dialog aria-labelledby to point to a visible description elsewhere than the modal title? Note that we'd need to be careful with where that description is located, because when the modal is active, the rest of the document is hidden with aria-hidden, so a description located outside the modal itself will be inaccessible.

[p-jackson]

@tellthemachines I think the aria.labelledby prop is more useful to folks using @wordpress/components as a component library, not for those working on the editor UI. If a new modal is being added to the block editor then I think this makes more sense:

• Is the heading visible? Use the title prop, we should be encouraging consistent styling of modal headings
• Is the heading invisible? Use the contentLabel prop to provide a title for assistive technology

For my case, I'm using @wordpress/components more like a component library. I might want a modal that looks more like this:

I could also achieve this by using the title prop and CSS overrides to absolutely position the title where I want it, however I also want the <p> after the <h1> to be flow naturally if for example the heading text wraps.

[tellthemachines]

I could also achieve this by using the title prop and CSS overrides to absolutely position the title where I want it, however I also want the <p> after the <h1> to be flow naturally if for example the heading text wraps.

I reckon you could achieve that layout just by fiddling with display, borders and padding, and absolutely positioning the close button instead 🤔

I'm not opposed to passing in aria-labelledby explicitly though. In any case, it's already part of the component API so we're stuck with it 😅 . What we can do is make sure we minimise the chances for incorrect usage by improving its docs. The Readme isn't very clear on when aria-labelledby should be used; perhaps we can improve it a bit?

[diegohaz]

Thanks, @p-jackson! I agree that this could be useful, but it looks like we should update the README file, which currently says that title is a required prop and aria.labelledby has a default value.

---

I know that this is not the point of this PR, but I'd like to make a note that this API feels weird. For example, why do we have a custom object prop called aria instead of relying on the aria-* attributes that are standardized and already documented by W3C with plenty of references online?

[diegohaz]

Nice! I didn't know about this within method. 👍

[p-jackson]

Neither did I! Required a bit of googling for how to test portals.

[p-jackson]

Thanks for the reviews @tellthemachines @diegohaz! I've updated the readme to try and make it clear that you really should have a title, and that one of title, contentLabel or aria.labelledby should be used.

I'd like to make a note that this API feels weird

Yeah I agree. Looks like this API goes pretty far back in the history; perhaps we thought providing aria objects was the way to go back then?

Integrating the g2 components feels like a good opportunity to deprecate these props and switch to the standard aria-* names.

[tellthemachines]

Looks good, thanks!