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(comboBox): refactor components for accessibility #636

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Tianhui-Han
Copy link
Contributor

@Tianhui-Han Tianhui-Han commented Sep 14, 2024

IMPORTANT

Currently, this project is closed to any external contributions. Any pull request made against this project from external sources will likely be closed. If you would like to make changes to this project, please fork this project.

Guide

This "Help" section can be deleted before submitting this pull request.

Update the name of this pull request to reflect the following shape:

{type}/{scope?}/{message}
  • type - A conventional commit type REQUIRED
  • scope - The kabab-case scope of the changes in this request
  • message - A short, kebab-case statement describing the changes REQUIRED

Provide a general summary of the scope of the changes in this pull request.

Description

refactor components for accessibility

In total, the changes involve four components, Combobox, Popover, ListBoxItem and listBoxBase:

The modifications to ListBoxItem and ListBoxBase are all based on the functionality of Combobox, (add data-focused to the element to implement the focus style), Why?
Since the Combobox implementation based on React-aria, when we use the keyboard to select the listItem (using the up and down keys to switch targets), the focus is actually still on the input, so we need to do an additional focus hint here.

Regarding Popover, there are two changes here, one is to remove aria-hidden from the tippyContainer (to ensure that the focus of the SR can enter the Popover), and the other is to fix the positioning of the Popover.
The full description of the changes made in this request.

Links

https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-545812
Links to relevent resources.

@maxinteger
Copy link
Collaborator

I just did a quick look on the PR, but looks like it tries to fix multiple unrelated issues in many component. It would be nice if this PR have a proper description about which issues handled.
If the issues are unrelated to each other then please create a new PR for each issue. It will help the review process and reduce risk to introduce new issues in Cantina.

@Tianhui-Han
Copy link
Contributor Author

Tianhui-Han commented Oct 8, 2024

I just did a quick look on the PR, but looks like it tries to fix multiple unrelated issues in many component. It would be nice if this PR have a proper description about which issues handled. If the issues are unrelated to each other then please create a new PR for each issue. It will help the review process and reduce risk to introduce new issues in Cantina.

@maxinteger
Sorry to get back to you so late,
In total, the changes involve four components, Combobox, Popover, ListBoxItem and listBoxBase:

The modifications to ListBoxItem and ListBoxBase are all based on the functionality of Combobox, (add data-focused to the element to implement the focus style), Why?
Since the Combobox implementation based on React-aria, when we use the keyboard to select the listItem (using the up and down keys to switch targets), the focus is actually still on the input, so we need to do an additional focus hint here.

Regarding Popover, there are two changes here, one is to remove aria-hidden from the tippyContainer (to ensure that the focus of the SR can enter the Popover), and the other is to fix the positioning of the Popover.

I agree with you, so I'm going to split out the part of Popover's fix positioning, and let me know if you have any other suggestions!

@Tianhui-Han Tianhui-Han marked this pull request as ready for review October 11, 2024 08:48
@maxinteger
Copy link
Collaborator

@Tianhui-Han Thank you for the explanation. You should split up this PR into digestible size ones. I still see changes in the Popover folder are they needed for the combobox changes?

Please note there are many change happened around the menus and list items in the last few week.
Please checkout the latest version of MRv2 and check again your use cases, they might already fixed by someone else.

@maxinteger maxinteger added the validated If the pull request is validated automation. label Oct 11, 2024
@Tianhui-Han
Copy link
Contributor Author

@maxinteger
Thanks for your reminder, I have synced the branch to the latest version to check the changes of the components.

Previously, the changes related to option in popover were removed. The remaining changes are to support the accessibility of combobox. See the figure below. Here, the attribute aria-hidden='true' is present in tippy, which will cause the screen reader to be unable to read the content. This is the problem solved by the changes in popover.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants