Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure the Modal dialog is always properly labelled #47885

Open
afercia opened this issue Feb 8, 2023 · 4 comments
Open

Make sure the Modal dialog is always properly labelled #47885

afercia opened this issue Feb 8, 2023 · 4 comments
Assignees
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Feb 8, 2023

Updated Description

As suggested, I updated the title of this issue to address the problem in a more holistic way. The previous title mentioned a specific case where a Modal dialog was unlabelled: Site editor: the global Save modal dialog is unlabelled.

The underlying problem is more general though.
A modal dialog should always be labelled. Keeping into consideration that a visible title is always strongly preferred, there are two options to label a modal dialog. The element with role="dialog" should:

  • Either have an aria-labelledby attribute referencing the ID of an element that wraps the modal title.
  • Or have an aria-label attribute with a meaningful title.

This mechanism is implemented in the Modal and it's based on the title and the contentLabel props. However, these props are not required. One of these two props should always be set to ensure proper labelling.

Also, when __experimentalHideHeader is true to hide the modal dialog header, the H1 within the header should still be rendered. It can be visually hidden. It needs to be in the DOM though otherwise setting a title prop won't have any effect.

Old Description

In #47142, a global 'save' panel has been wrapped in a modal dialog.

The modal dialog is unlabelled:

Screenshot 2023-02-08 at 15 08 18

Any modal dialog must always be labelled by the means of:

  • either a visible H1 heading referenced by aria-labelledby set on the element with role="dialog"
  • or an aria-label attribute set on the element with role="dialog"

In the past, there have been a few cases of unlabelled modal dialogs which had to be fixed. It would be great to enforce via code that a proper labelling mechanism is in place.

  • Is there any way to make required either the title prop or the contentLabel prop? One of these two props should always be set. Cc @youknowriad 🙏
  • When __experimentalHideHeader is true to hide the modal dialog header, the H1 within the header should still be rendered. It cab be visually hidden. It needs to be in the DOM though otherwise setting a title prop won't have any effect.
  • Designers: please consider that a visible heading is always strongly preferred. That helps all users to understand what the modal dialog is about. Also, when a modal dialog opens, the modal content is the only perceivable content on the page. Also assistive technologies will 'see' only the modal dialog. With no H1 heading, there won't be any heading on the page.

Step-by-step reproduction instructions

  • Go to the Site editor.
  • Make a change to a template and do not save.
  • Go back go 'browse mode'.
  • Click the 'Save' button at the bottom of the navigation sidebar.
  • The modal dialog opens.
  • Observe the modal dialog has no visible title.
  • Inspect the source and observe the modal dialog is unlabelled (see the screenshot above).

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Labelling [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Feb 8, 2023
@glendaviesnz glendaviesnz self-assigned this Feb 9, 2023
@glendaviesnz
Copy link
Contributor

I have added the aria label in this PR - my suggestion would be that we get this simple fix into 6.2 and then repurpose this issue to look at ways to improve the Dialog component in general post 6.2, eg. keeping the heading but visually hidden if __experimentalHideHeader is set as you suggest @afercia

@glendaviesnz
Copy link
Contributor

@afercia with #47898 merged I think we can possibly take this off the 6.2 project board, and update the title and description a little bit to focus it more on improving the Dialog component __experimentalHideHeader option, looking to make title compulsory, etc. What do you think?

@afercia
Copy link
Contributor Author

afercia commented Feb 10, 2023

@glendaviesnz thanks for looking into this and for #47898.
Totally agree to consider more general improvements in follow-up PRs. I'll update this issue title and description.

@afercia afercia changed the title Site editor: the global Save modal dialog is unlabelled Male sure the Modal dialog is always properly labelled Feb 10, 2023
@annezazu annezazu changed the title Male sure the Modal dialog is always properly labelled Make sure the Modal dialog is always properly labelled Feb 10, 2023
@Mamaduka
Copy link
Member

I've removed the issue from WP 6.2 project board. Let's try and land general improvements before the next major versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants