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.

Sign up for GitHub

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

Jump to bottom

Fix: a11y combobox attributes (fixes #147) #198

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+7 −5
Open

Fix: a11y combobox attributes (fixes #147) #198

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06ff974
Fix: a11y combobox attributes (fixes #147)
oliverfoster Nov 11, 2024
9d92a1c
Recommendations
oliverfoster Nov 11, 2024
08c15ea
Recommendations
oliverfoster Nov 11, 2024
271aa86
make ul and button aria-labelledby the same as per WCAG example
kirsty-hames Nov 14, 2024
ee5520d
move aria-activedescendant from ul to button as per WCAG example
kirsty-hames Nov 14, 2024
98b84d6
revert aria-activedescendant back to ul
kirsty-hames Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions templates/matching.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export default function Matching(props) {
const activeOption = _options.find(option => (option._itemIndex === _index) && option._isActive);
const displayItemAsCorrect = (!_isEnabled && _shouldShowMarking && (_isCorrectAnswerShown || activeOption?._shouldBeSelected));
const questionTitleId = `${_id}-matching-item-${_index}__title`;
const questionTextId = `${_id}-matching-item-${_index}__text`;
const hasMultipleCorrectAnswers = _correctAnswers.length > 1;

return (
Expand All @@ -70,7 +69,7 @@ export default function Matching(props) {

<div className="matching-item__select-container js-matching-item-select-container">

<templates.matchingDropDown {...props} _itemIndex={_index} questionTitleId={questionTitleId} questionTextId={questionTextId}/>
<templates.matchingDropDown {...props} _itemIndex={_index} questionTitleId={questionTitleId}/>

<div className="matching-item__select-state">
<div className="matching-item__select-icon matching-item__select-correct-icon" aria-label={_globals._accessibility._ariaLabels.correct}>
Expand Down
11 changes: 7 additions & 4 deletions templates/matchingDropDown.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,19 @@ export default function MatchingDropDown(props) {
!_isEnabled && 'is-disabled',
hasActiveOption && 'is-selected'
])}
role="combobox"
aria-haspopup="listbox"
aria-expanded={isOpen}
aria-controls={`${_id}-matching-item-${_itemIndex}__list`}
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.

>

<span id={props.questionTextId} className="dropdown__inner js-dropdown-inner" dangerouslySetInnerHTML={{ __html: displayActiveOption?.text }}>
<span className="dropdown__inner js-dropdown-inner" dangerouslySetInnerHTML={{ __html: displayActiveOption?.text }}>
</span>

<span className="dropdown__icon" aria-hidden="true">
Expand All @@ -205,13 +208,13 @@ export default function MatchingDropDown(props) {
left: buttonOffsetLeft,
width: buttonWidth
}}
id={`${_id}-matching-item-${_itemIndex}__list`}
role="listbox"
tabIndex="-1"
onBlur={onListBlur}
ref={list}
disabled={!_isEnabled}
aria-activedescendant={highlightedOption && `dropdown__item__${_id}__${_itemIndex}__${highlightedOption._index}`}
aria-labelledby={`${_id}-matching-item-${_itemIndex}__title`}
aria-labelledby={props.questionTitleId}
>

{options.map(({
Expand Down