-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
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. |
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 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. |
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. |
@SantosGuillamot thank you for the quick fix. The most important point is covered I think. Couple more things that I'd consider enhancements: 1
This produce a confusing announcement for screen reader users:
This announcement is a little confusing and unnecessarily noisy for screen reader users and ideally should be avoided. 2
is pointless because the component doesn't accept a |
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. |
I just created this pull request to handle that: link. |
@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? |
From my tests, I think the original issue is solved. The |
Thank you 🙇 I'm going to close this out and @afercia can reopen if there's something we missed here. |
@SantosGuillamot @getdave I just quickly inspected the output of a navigation block in the front end and I see it sill renders an empty Again: _all the |
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 |
This comment has been solved in this pull request. Should we close this one, or is this still an issue @afercia ? |
Closing, thanks for the quick fix! |
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
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.
<div>
element with no role (the role attribute is empty).Bad value for attribute role on element div
Screenshot of the current markup:
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
role=""
aria-label="Menu"
aria-modal="false"
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
The text was updated successfully, but these errors were encountered: