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

Alternative pending label approach #5368

Merged
merged 6 commits into from Nov 13, 2023

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Nov 6, 2023

Closes

Needs cleanup, but check if this accomplishes what we need in terms of announcements across different AT and browser combinations.
I expect it to read the regular button label when focused. Then when it enters pending, I expect it to read the button label again, followed by the pending string (we could put a comma here to add a pause to the AT). When it exits pending, it reads the button label once more.

Might need to do some id merging to make sure we didn't clobber anything.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Nov 8, 2023

@iamwillpowell
Copy link
Contributor

I definitely think this approach is cleaner. I did some preliminary testing with different screen readers and here are my findings. There are still some gaps with announcements, but there were problems with my approach also.

  • Voiceover on MacOs works pretty well.
    • Tooltip version didn’t announce
    • Sometimes it didn’t announce labels esp when ctl option space clicking on buttons
  • Jaws Chrome
    • Regular buttons: Didn't announce anything for pending state.
    • Icon buttons: Announced aria-label without “pending” when pressed
  • Jaws Firefox
    • Announces “50 pending”, but only once which is better.
    • Icon buttons don’t announce
  • NVDA Firefox
    • Still announces “50 pending” multiple times
    • Icon button with aria label on button only announces “pending”, misses aria-label
  • NVDA Chrome
    • Announces multiple “click me! pending”
    • Aria label icon buttons: reads "Pending" then the aria label

@adobe adobe deleted a comment from rspbot Nov 10, 2023
@yihuiliao
Copy link
Collaborator

yihuiliao commented Nov 10, 2023

Yeah, it seems like there's no way to prevent the 50 from announcing in Firefox unless we add an aria-label on the button. I submitted an issue for that a couple weeks ago here. Not sure when that will get picked up though.

From what I recall, the aria-live is what seems to cause the announcement to be announced multiple times in NVDA. I think we either accept that it will make multiple announcements, or we remove aria-live and add an aria-label on the button when it is pending.

Otherwise, I think this is the right approach for labeling the pending button.

@snowystinger snowystinger merged commit 94b23de into append-pending Nov 13, 2023
26 checks passed
@snowystinger snowystinger deleted the alternative-pending-label-approach branch November 13, 2023 00:46
@snowystinger
Copy link
Member Author

snowystinger commented Nov 13, 2023

I'm getting good announcements with Chrome + VO

Safari + VO I get no announcements, however, I didn't get anything before my changes either. This seems to have something to do with the role button https://codesandbox.io/s/goofy-bird-wdpk2z?file=/src/App.tsx (I've tried all permutations of aria-relevant/aria-disabled/aria-live/aria-atomic/etc to no avail) We could render a visually hidden live region along-side every button, but I'm not sure what this would break. I am sure that it would break something though, as last time we did something similar to ActionButton, it broke ActionGroup. Otherwise, I need to be able to render something arbitrary into LiveAnnouncer so I can attach ids.

iOS + VO I do get a good number of duplicate announcements, but I think that's fine.

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

4 participants