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

Fix: a11y - edit page - add block/blocks-chooser #6597

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Manas-Kenge
Copy link

@Manas-Kenge Manas-Kenge commented Jan 16, 2025


Closes #5212

This pull request fixes accessibility in the "Add Block" modal. Focus now stays inside the modal, pressing Enter expands the accordion and moves focus inside, while ESC closes it and returns focus to the header.

Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit a75e8b7
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/678c9d80ce7afb0008c9b0e8

@Manas-Kenge
Copy link
Author

@stevepiercy Can you review this?

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only triaged this PR. It still needs technical review. I'll add the teams to the request.

packages/volto/news/5212.bugfix Outdated Show resolved Hide resolved
packages/volto/news/5212.bugfix Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The news looks good. This PR still needs technical review.

@JeffersonBledsoe
Copy link
Member

Hi @Manas-Kenge, thanks for the work on this. While the work in this PR is definitely an improvement on the existing functionality and is much appreciated, I think the multiple problems noted with the blocks chooser noted in the linked issue need further discussion and some more definitive criteria for what a 'fixed' version of the chooser should be. Let's put this PR on hold while we get a clearer picture of what all the problems are and what the best way of solving them should be. cc @plone/volto-accessibility .

Notes I made while doing a quick test of this PR using VoiceOver on mac via Safari and Google Chrome:

  • "Add block button" markup should reflect what the button is going to control and it's state. We could aria-expanded and aria-controls to associate the button with the picker. We also need to add aria-haspopup="dialog".
  • When using the search, we should use a live region to convey the results are changing (e.g. list the number of results or say there are no results).
  • We should include a close button. This isn't a simple enhancement for an input picker (like a calendar selection for a date input) and so the 'ESC to close' shortcut shouldn't be the only thing we provide to close this dialog. Even alot of accessible date pickers provide a dedicated close button. This does not currently exist.
  • When closing the picker without selecting something, focus should return to the "Add block" button. This does not currently work.
  • When the picker is closed through the selection of a block, the focus should move to the new block. This PR has this behaviour.
  • Some users may expect focus to return to the start of the picker when pressing 'TAB' after being on the last focusable element. This is common behaviour for forms within dialogs. This needs further research so isn't a "MUST" requirement.

@Manas-Kenge
Copy link
Author

Thank you, @JeffersonBledsoe, for the detailed review and thoughtful feedback.

Regarding the close button, I initially implemented an 'X' button for closing the blocks chooser but noticed it negatively impacted the UI, as it increased the height of the accordion more than necessary. To maintain a clean design, I decided to remove it. However, I understand the importance of accessibility and am happy to add it if the accessibility team decides that including a close button is essential.

As for using aria-expanded, aria-controls, aria-haspopup I will look into it.

I'll await further input from the accessibility team to ensure these changes are properly implemented. Please let me know how you’d like to proceed after discussing these points further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a11y - edit page - add block/blocks-chooser - cms ui
3 participants