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 combobox attributes (fixes #147) #198

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

Conversation

oliverfoster
Copy link
Member

fixes #147

Fix

  • Added role, aria-controls and relevant id

@oliverfoster oliverfoster self-assigned this Nov 11, 2024
Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Testing this with VoiceOver in Safari functions as expected however the reading of the button isn't. "Misspelt" is announced before reading the button context (see screenshot below). I can't see why the "Select an option" text is being cut off. I'll test this with NVDA to compare.
Screenshot 2024-11-11 at 12 51 09

@oliverfoster
Copy link
Member Author

oliverfoster commented Nov 11, 2024

Testing this with VoiceOver in Safari functions as expected however the reading of the button isn't. "Misspelt" is announced before reading the button context (see screenshot below). I can't see why the "Select an option" text is being cut off. I'll test this with NVDA to compare. Screenshot 2024-11-11 at 12 51 09

looking at the example, we maybe have too many different aria-labelledbys, perhaps the one on the ul could be made the same as the one on the button?

https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/#sc1_label

image

@kirsty-hames
Copy link
Contributor

kirsty-hames commented Nov 14, 2024

Hey @oliverfoster, I've made a couple of commits to align this with the WCAG example you shared. Overall this is reading much better.

We still have the issue mentioned above where VoiceOver in Safari reads "Misspelt" and cuts off the label. I can only replicate this in Safari. VoiceOver in Chrome reads beautifully. NVDA in Chrome and Edge read nicely too.

I'm running Safari v18.0.1 - not sure if this is a Safari specific issue or limited to a particular version. I'll give this a test on iPhone Safari and I'll see if I can replicate the issue. I'll also test the master branch (as it may be an issue prior to this PR).

Update: VoiceOver on iPhone works as expected so the issue seems to be specific to Safari on macOS. I couldn't replicate issue running the master branch so the issue has been introduced during this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

onMouseDown={onStartInteraction}
onTouchStart={onStartInteraction}
onClick={onButtonClick}
ref={button}
aria-labelledby={props.questionTitleId + ' ' + props.questionTextId}
aria-labelledby={props.questionTitleId}
aria-activedescendant={highlightedOption && `dropdown__item__${_id}__${_itemIndex}__${highlightedOption._index}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving aria-activedescendant from the listbox to the combobox is causing some strange behaviour for keyboard navigation/screen readers. Currently if I open the matching item the list items either not being read out (firefox) or missed entirely (chrome), both tested with NVDA. Is moving aria-activedescendant necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is moving aria-activedescendant necessary?

I don't think so. If I revert aria-activedescendant back to listbox, this reads as expected for NVDA on Chrome, Edge and FF. No change to VoiceOver on Safari and Chrome either. I'll commit this back.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ready to retest in 98b84d6 please @joe-allen-89.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Needs Reviewing
Development

Successfully merging this pull request may close these issues.

Align with wcag spec
4 participants