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

ha-form-multi_select accessibility improvements #21023

Merged
merged 6 commits into from
Dec 23, 2024

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Jun 7, 2024

Proposed change

The current multi_select control (combobox variant) for ha-form is very poorly accessible:

  • it is not possible to interact at all with the keyboard, it doesn't appear to be selectable
  • if you could open it, is is not possible to change the selection of any of the items with the keyboard
  • screen reader don't understand any of the structure of the menu.

I have attempted to rewrite this part to improve this. I initially tried fixing it with the existing components (I think it's material 2?), but I wasn't successful in getting something to work. I then spent a little time trying it with the newer material 3 components, and I think I got something satisfactory:

  • I was able to open the form with the keyboard and navigate the item list
  • Pressing space or enter checks/unchecks items
  • NVDA seems to correctly announce the items in the list and announces if they are checked.

I am fairly new with dealing with complex usage of material components, or anything accessibility related, so a lot of this was just guessing and trial and error, so I may have done some things incorrectly.

Sample screenshot:
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Summary by CodeRabbit

  • New Features

    • Improved event handling for opening and closing events in the button menu component.
    • Enhanced keyboard interaction and item toggling in the multi-select form component.
  • Refactor

    • Replaced ha-svg-icon with ha-icon-button for better icon handling.
    • Updated from ha-check-list-item to ha-menu-item for options rendering in multi-select forms.

This comment was marked as off-topic.

@karwosts karwosts added the Accessibility Related to accessibility for people with disabilities label Jun 7, 2024
coderabbitai[bot]

This comment was marked as off-topic.

@karwosts karwosts requested a review from steverep June 7, 2024 14:36
coderabbitai[bot]

This comment was marked as off-topic.

@steverep
Copy link
Member

steverep commented Jun 7, 2024

I'm pretty busy today and tomorrow, but I'll review sometime this weekend.

@bramkragten bramkragten self-requested a review June 10, 2024 07:27
@steverep
Copy link
Member

I've been dealing with some data loss because of a Docker update, but I'll get to this soon.

Copy link

github-actions bot commented Sep 8, 2024

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 8, 2024
@karwosts karwosts removed the stale label Sep 8, 2024
silamon
silamon previously approved these changes Sep 10, 2024
Copy link
Contributor

@silamon silamon left a comment

Choose a reason for hiding this comment

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

Looking good on the material 3 part. Sooner or later we may want to move the keydown listener to the button menu component so this gets accesible wherever the component is used.

@bramkragten bramkragten self-assigned this Nov 20, 2024
Comment on lines 51 to 57
private _handleOpening(): void {
fireEvent(this, "opening", undefined, { bubbles: false });
}

private _handleClosing(): void {
fireEvent(this, "closing", undefined, { bubbles: false });
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we re-fire these, are they originally fired un-composed? And why do we disable bubbling, should we also fire them un-composed then instead if disabling bubbling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we re-fire these, are they originally fired un-composed?

It's been a while, but I think I recall that was the issue.

And why do we disable bubbling, should we also fire them un-composed then instead if disabling bubbling?

I am not so knowledgeable about this. I mostly got it working via guessing/trial & error. Can try something else if you have a better suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised they fire un-composed, but ok 🤷‍♂️ I suggest we do the same then.

    fireEvent(this, "closing", undefined, { composed: false });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems to work, updated

@bramkragten bramkragten merged commit 147098f into home-assistant:dev Dec 23, 2024
15 checks passed
@karwosts karwosts deleted the multi-select-accessibility branch December 23, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Related to accessibility for people with disabilities cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility issues for blind people when creating an automation by selecting days of the week
4 participants