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

Interactivity API renders incorrect ARIA attributes for the Navigation block #54560

Closed
afercia opened this issue Sep 18, 2023 · 14 comments
Closed
Labels
[Block] Navigation Affects the Navigation Block [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 Sep 18, 2023

Description

See #54343

Last week, #54343 merged into trunk some code for the Interactivity API Server Side Rendering. That change adds some default markup that I guess is meant to be changed dymanically via JavaScript. However, the default markup is incorrect.

I noticed whlle testing Twenty Twent-Four. so it is important to be aware the markup of the new default theme is invalid at the moment. Cc @carolinan

Quoting from #54343

I just had to add the aria-modal="false" and the role=""

These attributes are rendered by default in the desktop view when the navigation block is not inside a dialog, which is incorrect. Any Accessibility automated checker will flag this as an error.

  • The ARIA property aria-modal can only be used on elements with an ARIA role of window, dialog, or alertdialog. However, it is now set on a <div> element with no role (the role attribute is empty).
  • An ARIA role attribute can't be empty. Checking the markup through the W3C validator at https://validator.w3.org/nu/#textarea thRows an error Bad value for attribute role on element div
  • Technically, the aria-label property can be set on a div without a role. Howeever, it doesn'e make much sense as a non-focusable div without role won't ne perceived by assistive technology.

Screenshot of the current markup:

Screenshot 2023-09-18 at 15 28 12

These attributes should be entirely omitted when the Navigation block is not rendered within a dialog. Not even sure why they are there but printing out markup that makes a page invalid and may impact assistive technology doesn't seem ideal.

@annezazu pinging you for considering to include this in the 6.4 board, when you have a chance) 🙏

Step-by-step reproduction instructions

  • Activate Twenty Twent-Four (I think thebug can be reproduced also with other themes).
  • Make sure you have set a primary menu
  • Go to any front end page
  • Inspect the source of the navigation block container
  • Observe that in the desktop view the container has these attributes:
    • role=""
    • aria-label="Menu"
    • aria-modal="false"
  • Switch to the responsive view by using your browser dev tools or by shrinking your browser window.
  • Inspect the source and observe these attributes have now changed to:
    • role="dialog"
    • aria-label="Menu"
    • aria-modal="ftrue"

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). labels Sep 18, 2023
@skorasaurus skorasaurus added the [Block] Navigation Affects the Navigation Block label Sep 18, 2023
@SantosGuillamot
Copy link
Contributor

Thanks for raising the issue 🙂

Just to clarify, what I did in the PR you mentioned was replicate what is happening in the client. So I believe we should fix the JS logic as well. I'll try to fix that and submit a new PR.

@afercia
Copy link
Contributor Author

afercia commented Sep 19, 2023

I had a quick look at the JS and as far as I can tell, it just sets correct and valid HTML attributes, when the dialog opens.

@SantosGuillamot
Copy link
Contributor

I had a quick look at the JS and as far as I can tell, it just sets correct and valid HTML attributes, when the dialog opens.

Yes, but it wasn't removing the role attribute after closing it if I am not mistaken.

We've been triaging it and it seems it is a bug with Preact, which is used by the Interactivity API. I opened an issue in their repo for that. In the meantime, I opened this pull request with a workaround for that and fixing the issue reported here.

@SantosGuillamot
Copy link
Contributor

I've merged the pull request that is supposed to fix this to ensure it is included in the next release. @afercia Please let me know if you think something else is needed, and I can work on it in a new pull request if needed.

@afercia
Copy link
Contributor Author

afercia commented Sep 20, 2023

@SantosGuillamot thank you for the quick fix. The most important point is covered I think. Couple more things that I'd consider enhancements:

1
Ideally, this aria-label should not be set on the PHP side, as it is always there even when the dialog is not open:

<div class="wp-block-navigation__responsive-dialog" aria-label="%8$s" %13$s>

This produce a confusing announcement for screen reader users:

  • They will first hear the navigation being announced e.g. "{menu name}, navigation"
Screenshot 2023-09-20 at 11 52 16
  • Then, when moving into the navigation, they will hear "Menu, group". This gets announced becouse:
    • The aria-lable is Menu even if this is not necessary when the dialog is not open.
    • Sinthe the div element with no role attribute does have an aria-label, screen readers fallback to the most generic role that is group and announce the element as such.

This announcement is a little confusing and unnecessarily noisy for screen reader users and ideally should be avoided.

Screenshot 2023-09-20 at 11 52 25 Screenshot 2023-09-20 at 11 52 20

2
Seems to me this label prop passed to the ResponsiveWrapper component:

is pointless because the component doesn't accept a label prop. As far as I can tell, this prop doesn't do anything and I guess it should be removed.

@bph bph moved this to Needs Dev / Todo in WordPress 6.4 Editor Tasks Sep 23, 2023
@bph bph moved this from Needs Dev / Todo to In Progress in WordPress 6.4 Editor Tasks Sep 23, 2023
@SantosGuillamot
Copy link
Contributor

Thanks for the detailed information! I didn't change that in the previous PR because those attributes had been there for a while, and I wasn't sure about the implications. I can work on a new PR to remove those attributes.

@SantosGuillamot
Copy link
Contributor

I just created this pull request to handle that: link.

@bph bph moved this from In Progress to Punted to 6.5 in WordPress 6.4 Editor Tasks Oct 13, 2023
@getdave
Copy link
Contributor

getdave commented Nov 21, 2024

@SantosGuillamot Are you able to summarise the state of this Issue? It looks like a related PR was merged but perhaps that wasn't able to address all the concerns? Are you aware of elements that are outstanding?

@SantosGuillamot
Copy link
Contributor

From my tests, I think the original issue is solved. The aria-modal, role, and aria-label attributes are only present when the modal is actually opened.

@getdave
Copy link
Contributor

getdave commented Nov 27, 2024

Thank you 🙇 I'm going to close this out and @afercia can reopen if there's something we missed here.

@getdave getdave closed this as completed Nov 27, 2024
@afercia
Copy link
Contributor Author

afercia commented Nov 28, 2024

@SantosGuillamot @getdave I just quickly inspected the output of a navigation block in the front end and I see it sill renders an empty aria-label by default:

Image

Again: _all the aria-* attributes, role, etc. should only be rendered in the markup when the navigation is a dialog. Could you please havea look again? Thanks.

@SantosGuillamot
Copy link
Contributor

Again: _all the aria-* attributes, role, etc. should only be rendered in the markup when the navigation is a dialog.

I got that part. In my tests, I checked the original reported issue and the attributes handled by the Interactivity API. This looks like a different (similar) issue happening in a different element, independent of whether the menu is open or not, and is not related to the Interactivity API at all. Anyway, I've created a pull request aiming to solve that: #67381

@SantosGuillamot
Copy link
Contributor

This comment has been solved in this pull request. Should we close this one, or is this still an issue @afercia ?

@afercia
Copy link
Contributor Author

afercia commented Nov 29, 2024

Closing, thanks for the quick fix!

@afercia afercia closed this as completed Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [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
No open projects
Status: Punted to 6.5
Development

No branches or pull requests

4 participants