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

[WNMGDS-2712] Update deprecated aria #3051

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

Conversation

zarahzachz
Copy link
Collaborator

@zarahzachz zarahzachz commented Apr 22, 2024

WNMGDS-2712

Made assorted ARIA tweaks. ARIA has deprecated some of its attrs on certain roles, so we needed to update those roles to better work with ARIA. More context is available in #2978.

Demo url

@zarahzachz zarahzachz marked this pull request as draft April 22, 2024 20:36
@TiffanyPender
Copy link

For WNMGDS-2769 A11y QA, I've added my findings for the components updated as part of WNMGDS-2712.

  • Autocomplete

    • aria-selected="false" is still present on li elements.
    • JAWS doesn't announce options as you move through the option list. It announces "collapsed" after making a selection, but not what you selected.
    • NVDA announces the letter you typed as you move through the option list. It announces what you selected plus "collapsed" after making a selection.
  • ChoiceList

    • Checkbox

      • What changed? When I enable the error state by providing an error message, aria-invalid is applied to the fieldset element.
        • From 2712: "Set role="listbox" on fieldset, works with aria-invalid" —— This is not valid - ARIA role listbox is not allowed for given element (Deque) (fieldset element). This role could possibly be used on a div, but I'm not sure that we'd want to deviate from using the fieldset element.
      • aria-invalid is not allowed on the fieldset element (unless it has the radiogroup role).
        • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.
        • NVDA announces "grouping invalid entry, check some boxes".
    • Radio

    • role="radiogroup" is applied, along with aria-invalid="true"

    • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.

    • NVDA announces "grouping invalid entry, choose an option".

    • Additional resources:

  • Dialog

    • What changed? Focus moves to the dialog as expected.
    • H2 is present as default.
  • Drawer

    • aria-labelledby is on the dialog element and referencing the drawer heading text. The default of H3 may cause an issue with heading hierarchy.
    • The drawer body is in the tab order, with a visible focus. Users should not be able to tab to the drawer body. Tabs should only go to interactive elements.
  • Dropdown

  • MonthPicker

    • What changed? I see aria-invalid="true" on the fieldset. ARIA role listbox is not allowed for given element (Deque) (fieldset element). This role could possibly be used on a div, but I'm not sure that we'd want to deviate from using the fieldset element.
    • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.
    • NVDA announces "grouping invalid entry, select available months".
    • As an aside, NVDA reads out the abbreviations, so it would be good to add in screen reader text with the full month name, hiding the abbreviations from assistive technologies.

@zarahzachz
Copy link
Collaborator Author

zarahzachz commented Jun 5, 2024

Autocomplete

  • aria-selected="false" is still present on li elements.
  • JAWS doesn't announce options as you move through the option list. It announces "collapsed" after making a selection, but not what you selected.
  • NVDA announces the letter you typed as you move through the option list. It announces what you selected plus "collapsed" after making a selection.

This appears to be additional markup aria-react adds to the options in the component. Unfortunately, I don't have control over how aria-selected appears on those elements.

We have a component called renderStatusMessage that's used to show "no results" or "loading..." messages in the Autocomplete and for these examples, aria-selected is not present.

Because the code I changed impacts only those two examples, it seems the NVDA and JAWS issues are unrelated to my changes. I'll make a follow-up ticket to address them (and investigate if it's possible to modify the aria-react library to remove the redundant attr).

@zarahzachz
Copy link
Collaborator Author

zarahzachz commented Jun 5, 2024

Checkbox

  • What changed? When I enable the error state by providing an error message, aria-invalid is applied to the fieldset element.

    • From 2712: "Set role="listbox" on fieldset, works with aria-invalid" —— This is not valid - ARIA role listbox is not allowed for given element (Deque) (fieldset element). This role could possibly be used on a div, but I'm not sure that we'd want to deviate from using the fieldset element.
  • aria-invalid is not allowed on the fieldset element (unless it has the radiogroup role).

    • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.
    • NVDA announces "grouping invalid entry, check some boxes".

No changes were made to the checkbox implementation of Choice/ChoiceList because a suitable role that worked with aria-invalid couldn't be found. Currently, checkboxes are still wrapped in a <fieldset> tag, which has an implicit role of group and aria-invalid is not allowed on that role.

Setting aria-invalid for checkboxes still requires guidance. Since this code has been in PROD for awhile, this need doesn't appear to be a show-stopper or anything. I can create a follow-up ticket to further discuss (would apply to both checkbox Choice/ChoiceList and MonthPicker)

@zarahzachz
Copy link
Collaborator Author

zarahzachz commented Jun 5, 2024

  • Radio
  • role="radiogroup" is applied, along with aria-invalid="true"
  • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.
  • NVDA announces "grouping invalid entry, choose an option".

For radio Choice/ChoiceList, I've added role={props.type === 'radio' ? 'radiogroup' : null} to the <fieldset> element. This will apply the radiogroup role to <fieldset> only if the Choice type is "radio," and this seems to comply with the guidance present in the list of references. I wish checkboxes had a similar solution, but c'est la vie.

It's worth noting, the logic I added will result in checkbox fieldsets having the implicit role of "group," which is no change to how it functions today. So the only thing changing for Choice/ChoiceList is a slight improvement for radio types.

@zarahzachz
Copy link
Collaborator Author

zarahzachz commented Jun 5, 2024

Dialog

  • What changed? Focus moves to the dialog as expected.
  • H2 is present as default.

For Drawer, I moved the aria-labelledby off an internal <div> and placed it on the <dialog> element itself, since aria-labelledby can't be placed on an element with a "generic" role.

I also updated some of the semantic markup used within:

  • Replaced <header> and <main> with <div> and removed role="document" to match the markup we're using on Dialog (Drawer is just a different layout/style of Dialog, so they should have the same markup).
  • Updated the heading tag to not be an <h1> by default.

Since my changes didn't indicate anything had changed, it's probably safe to assume they're good to merge.

@zarahzachz
Copy link
Collaborator Author

zarahzachz commented Jun 5, 2024

Dropdown

No changes were made to Dropdown (aside from adding a code comment). I tested re-implementing role: "combobox", but ran into the same VO issues mentioned in the code comments (VO announcing <button> as an input with misspelling errors).

This is another side effect of using the react-aria library, and library authors appear to be aware of it.

Since this has code comments around it, I'm not going to make a follow-up ticket to investigate. Looks like we've already investigated things.

@zarahzachz
Copy link
Collaborator Author

MonthPicker

  • What changed? I see aria-invalid="true" on the fieldset. ARIA role listbox is not allowed for given element (Deque) (fieldset element). This role could possibly be used on a div, but I'm not sure that we'd want to deviate from using the fieldset element.
  • VO, JAWS doesn't announce "invalid" with the attribute. The only way to know is with the error message text present.
  • NVDA announces "grouping invalid entry, select available months".
  • As an aside, NVDA reads out the abbreviations, so it would be good to add in screen reader text with the full month name, hiding the abbreviations from assistive technologies.

Because MonthPicker is just a more extreme version of checkbox ChoiceList, I didn't implement any changes (due to the same <fieldset> issues mentioned in the checkbox ChoiceList comment).

Making a follow-up ticket to address the abbreviated month names for NVDA. Will need a11y testing when this one's complete since we don't have NVDA screen readers.

@zarahzachz zarahzachz changed the title [WIP] [WNMGDS-2712] Update deprecated aria [WNMGDS-2712] Update deprecated aria Jun 5, 2024
@zarahzachz zarahzachz marked this pull request as ready for review June 5, 2024 19:26
@zarahzachz zarahzachz requested a review from pwolfert June 5, 2024 19:26
Comment on lines 35 to 43
renderDialog({
const { container } = renderDialog({
actions: <span>Pretend these are actions</span>,
actionsClassName: 'test-action',
className: 'test-dialog',
headerClassName: 'test-header',
size: 'full',
});
expect(screen.getByRole('document')).toMatchSnapshot();

expect(container.querySelector('.ds-c-dialog__window')).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might actually be helpful to include the dialog element in the snapshots too, so updating the snapshot for this test rather than the test.

Comment on lines -127 to +129
<div
className="ds-c-dialog__window"
role="document"
ref={containerRef}
tabIndex={-1}
aria-labelledby={headingId}
>
<header className={headerClassNames}>
<div className="ds-c-dialog__window" ref={containerRef} tabIndex={-1}>
<div className={headerClassNames}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just know the original design was tested by accessibility specialists with a variety of screen readers, so will this be tested again by the accessibility team before merging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TiffanyPender would it be possible to get another review of the Dialog changes in this PR?

The changes made to this component are:

  • Moving aria-labelledby to the <dialog> element
  • Replacing the <header> and <main> tags with <div>
    • <header> and <main> didn't have accessible names
    • Including <main> seemed to fly against the rule of only 1 <main> element / page
  • Removing role="document" from an internal <div>

These changes would align Dialog with Drawer markup (Drawer does not use these elements) and I have a sneaking suspicion the original Dialog markup was written well before the release of <dialog>, so attributes like role="document" are unnecessary.

Comment on lines 75 to 77
// TODO: Investigate why this is needed.
// Block DOM els already have a column layout?
// Our supported browsers support flex, so this query not needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, it does seem sus. I traced it back to this commit, and according to that message, these @supports queries were added to support IE11. Let's remove them!

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

Successfully merging this pull request may close these issues.

None yet

3 participants