Update authorization button semantics to improve accessibility #5298

smhigley · 2019-04-12T21:27:05Z

Resolves #5297

Description

Adds aria-haspopup to authorization buttons to communicate existence of dialog
Updates the main authorize button to include locked/unlocked in accessible name
Hides operation authorize buttons' SVG to prevent screen readers accessing internal svg elements using caret navigation in scan mode
Associates operation-specific authorization buttons with the name of the operation they control through aria-describedby

Something worth noting: I created element IDs that should be unique, but are not guaranteed to be. Since accessibility features often rely on id refs, it might be a good idea to include a uuid package to generate unique IDs. I would be happy to do so in this PR if the project maintainers agree.

Motivation and Context

Improves experience for screen reader users (and potentially other assistive tech users).

How Has This Been Tested?

Tested in NVDA/Firefox, NVDA/Chrome, JAWS/Chrome, Narrator/Edge, and iOS/Safari/VoiceOver

Checklist

My PR contains...

No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
Dependency changes (any modification to dependencies in package.json)
Bug fixes (non-breaking change which fixes an issue)
Improvements (misc. changes to existing features)
Features (non-breaking change which adds functionality)

My changes...

are breaking changes to a public API (config options, System API, major UI change, etc).
are breaking changes to a private API (Redux, component props, utility functions, etc.).
are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
are not breaking changes.

Documentation

My changes do not require a change to the project documentation.
My changes require a change to the project documentation.
If yes to above: I have updated the documentation accordingly.

Automated tests

My changes can not or do not need to be tested.
My changes can and should be tested by unit and/or integration tests.
If yes to above: I have added tests to cover my changes.
If yes to above: I have taken care to cover edge cases in my tests.
All new and existing tests passed.
(I did not find existing tests for the components in question, so I didn't add new test files. As this change introduces no change in functionality it didn't seem pressing, but I would also be happy to add tests if that would improve the PR).

smhigley · 2019-04-12T21:31:33Z

src/core/components/operation-summary-path.jsx

@@ -37,7 +37,8 @@ export default class OperationSummaryPath extends PureComponent{
    const DeepLink = getComponent( "DeepLink" )

    return(
-      <span className={ deprecated ? "opblock-summary-path__deprecated" : "opblock-summary-path" } 
+      <span className={ deprecated ? "opblock-summary-path__deprecated" : "opblock-summary-path" }
+        id={`${tag}-${operationId}`}


I'm assuming this ID will be unique, but as I mentioned in the PR summary it would be safer to include a uuid package to generate unique IDs. If that seems like a good idea, I would be happy to update this PR with that change

Change request: use path + method instead of tag + operationId here.

Combinations of tag and operationId are not technically guaranteed to be unique, although the OpenAPI spec says non-unique operationIds are illegal. We've had complaints from users in places where we track state by operationId (e.g. #5096). You can also see this in action on the LA Metro Arrivals API - click on /agencies/{agency_tag}/routes/, notice that more than one operation expands (I've been meaning to send them some revisions to their document...).

Fortunately, because (a) paths and methods in OpenAPI documents are key names, and (b) YAML/JSON doesn't allow duplicated key names, path + method is guaranteed to be unique.

This is another instance of accessibility features not being safe for use cases that include multiple Swagger UI instances on one page though - see my thoughts on that in #5299 (comment).

FYI: you should be able to pull method out of operationProps.toJS().

leggsimon · 2019-04-16T10:03:49Z

src/core/components/auth/authorize-operation-btn.jsx

-        aria-label={isAuthorized ? "authorization button locked" : "authorization button unlocked"}
+        aria-describedBy={describedBy ? describedBy : null}
+        aria-haspopup="dialog"
+        aria-label={isAuthorized ? "authorize locked" : "authorize unlocked"}


Admittedly I find the UX of the authorisation stuff a bit unclear but as I understand it we get the "Locked" state when you have put in authentication credentials that the route requires. is that right?

If so, I wonder whether locked/unlocked is the right thing to read out here? Since the thing you’re checking is whether it is authorised I wonder whether just "Authorised"/"Unauthorised" would be better labels for this?

Having said that I might just be misunderstanding the logic for these padlock states

I'm actually not sure -- I was half-hoping someone more familiar with Swagger UI would comment and suggest better wording. Absent that, I just went with locked/unlocked since it was used before and it gave the closest parity with the visual experience.

maybe based on the actions in the popup, "Authorize" and "Remove authorization" might fit best?

Would be good to get some feedback here @shockey and @webron. What would be the clearer description for what these values mean given you might not have the visual context of the entire page?

@leggsimon: Having said that I might just be misunderstanding the logic for these padlock states

You wouldn't be the first - just take a look at #4402! We're moving away from the lock metaphor in general, it's clear it has not been a hit with users.

I can't share the new design proposal publicly yet, but suffice it to say that it unambiguously communicates two states: "authorization is available for this request, and has/but has not been provided".

I see no reason to torture screen reader users with a visual metaphor that even visual users don't like!

Change request: let's change this label!

@smhigley, when we last chatted I don't think we quite landed on an decision for what aria-label should represent in the context of a button that both conveys current state and triggers a dialog that allows users to mutate state - should the label describe the current state or what the button allows the user to change?

If we're focusing on current state, I think something like Authorization is active and Authorization is available but not active makes sense, but we can be more terse if that's conventional for labels, e.g. Authorization active and Authorization available.

Conversely, if we're focusing on mutations that the user can trigger, your suggestion of Authorize and Remove authorization is good.

Which road to take is up to you, @smhigley!

leggsimon · 2019-04-16T10:04:22Z

src/core/components/auth/authorize-btn.jsx

          <span>Authorize</span>
-          <svg width="20" height="20">
+          <svg width="20" height="20" aria-labelledby="authorize-button" role="img">
+            <title id="authorize-button">{isAuthorized ? "Locked" : "Unlocked"}</title>


Similar thoughts to the other comment about the wording of this.

Indeed, let's change the wording here as well.

leggsimon · 2019-04-16T10:13:47Z

src/core/components/operation-summary.jsx

@@ -73,6 +73,7 @@ export default class OperationSummary extends PureComponent {
        {
          (!security || !security.count()) ? null :
            <AuthorizeOperationBtn
+              describedBy={`${tag}-${operationId}`}


love this! although I think there should maybe be a test that ensures that the id in the operation summary path and the describedBy value here should be the same. maybe by passing it as a prop down somehow. I just worry that having the same thing done in two places might break at some point in the future if someone fiddles with the id in the operation summary path.

@leggsimon I worried about the same thing. My ideal solution would actually be a uuid library, but I wanted feedback from project admins before including one in a first PR 😄. Unit tests, at least for this, seem like a good addition though 👍

Unit tests would be great, and then I think rather than doing the id attribute creation in both places it might be nice to define the value outside this return jsx and pass it as an id prop to OperationSummaryPath on line 63 maybe?

But yes, I would be in favour of a uuid I think. Unfortunately I can’t make those calls either 😄

Change request: use path + method instead of tag + operationId here.

Same idea as above (https://github.com/swagger-api/swagger-ui/pull/5298/files#r280963958).

Also, a test would be good for the ID - I think it could fit well in either the Enzyme (component-focused) tests or the Cypress (real DOM end-to-end) tests.

mgifford · 2024-12-17T15:57:13Z

Where are we with this PR? Who can resolve the conflicts?

smhigley force-pushed the bug/5297-authorization-semantics branch from b4b8ac9 to af0d057 Compare April 12, 2019 21:29

smhigley commented Apr 12, 2019

View reviewed changes

update authorize button semantics to improve screen reader accessibility

785a430

smhigley force-pushed the bug/5297-authorization-semantics branch from af0d057 to 785a430 Compare April 12, 2019 22:11

leggsimon reviewed Apr 16, 2019

View reviewed changes

tim-lai mentioned this pull request Oct 19, 2020

a11y: missing labels on controls (text fields, select boxes) #6512

Open

Sundava mentioned this pull request Sep 28, 2021

feat(a11y): Add aria-label to controls #7528

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update authorization button semantics to improve accessibility #5298

Update authorization button semantics to improve accessibility #5298

smhigley commented Apr 12, 2019

smhigley Apr 12, 2019

shockey May 4, 2019

shockey May 4, 2019

leggsimon Apr 16, 2019

smhigley Apr 16, 2019

smhigley Apr 16, 2019

leggsimon Apr 16, 2019

shockey May 4, 2019 •

edited

Loading

leggsimon Apr 16, 2019

shockey May 4, 2019

leggsimon Apr 16, 2019

smhigley Apr 16, 2019

leggsimon Apr 16, 2019

leggsimon Apr 16, 2019

shockey May 4, 2019

shockey May 4, 2019

mgifford commented Dec 17, 2024

Update authorization button semantics to improve accessibility #5298

Are you sure you want to change the base?

Update authorization button semantics to improve accessibility #5298

Conversation

smhigley commented Apr 12, 2019

Description

Motivation and Context

How Has This Been Tested?

Checklist

My PR contains...

My changes...

Documentation

Automated tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Change request: use path + method instead of tag + operationId here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shockey May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Change request: let's change this label!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Change request: use path + method instead of tag + operationId here.

Choose a reason for hiding this comment

mgifford commented Dec 17, 2024

Change request: use `path + method` instead of tag + operationId here.

shockey May 4, 2019 •

edited

Loading