Skip to content

Conversation

@jjaeggli
Copy link
Contributor

@jjaeggli jjaeggli commented Apr 11, 2023

This change converts the accessibility help dialog to use correct markup for assistive technology / screen readers. This work was mentioned in #179689 and so I'm associating that here.

The modal dialog now uses aria-modal and is labeled by aria-labelledby which points to the h1 element within the modal. aria-describedby points to the text content of the dialog, as is optionally part of the dialog pattern.

The content of the dialog has the document role but does not has an aria-label as it is not required for this element, but an aria label is required for the dialog role per https://w3c.github.io/aria/#dialog.

List markup is used to identify list elements rather than plain text.

Focus points to the dialog text content as per the aria APG example but could focus the heading instead for better support in VoiceOver in particular, which doesn't announce the dialog title implicitly, unlike JAWS and NVDA.

Questions to the reviewer:

  • I attempted to follow existing coding style but I was uncertain about whether I should use $ jquery style element creation or FastDomNode everywhere.
  • Neither the electron app or the web app appear to be using AccessibilityHelpWidget within accessibilityHelp.ts, so I'm uncertain as to how to invoke this code (they're both using the code within accessibility.ts).
  • I didn't know if there was an existing global style for H1 elements which should be used for the heading. If you have style recommendations or an existing style which should be recommended, please let me know.

Web app:

Screenshot 2023-04-11 at 3 05 41 PM

Electron app:

Screenshot 2023-04-11 at 3 07 28 PM

@meganrogge
Copy link
Contributor

Until I get get to the work mentioned in #179689, these changes look like a nice improvement

@meganrogge meganrogge requested a review from isidorn April 12, 2023 14:07
@isidorn
Copy link
Collaborator

isidorn commented Apr 12, 2023

@jjaeggli thanks a lot for this PR 👏 It looks good to me.
@meganrogge how could we best combine this work with #179689

fyi @hediet @alexdima since this touches the Editor as well.

@isidorn isidorn requested a review from hediet April 12, 2023 18:34
@meganrogge meganrogge merged commit efbd412 into microsoft:main Apr 13, 2023
@isidorn isidorn added this to the April 2023 milestone Apr 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants