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

Append "pending" to button label and copy to isPending spinner aria-label #5245

Merged
merged 27 commits into from Nov 23, 2023

Conversation

iamwillpowell
Copy link
Contributor

@iamwillpowell iamwillpowell commented Oct 14, 2023

Closes

✅ 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:

Go to Button > Pending Spinner story and text the various incantations of buttons. Inspect pending spinner aria label or use your screen reader of choice to check what is announced.

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Oct 14, 2023

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

I'm quite confused by this computed label. It says no tooltip, but there is one, and I never hear the computed label read out.
Screenshot 2023-10-16 at 10 21 15 AM

What drove the decision to do the computed getLabelFromChildren approach? Were you facing issues with the order things were read if you just left it as it was and changed the string from loading to pending?

What are the announcements you're expecting to hear, it might be good to list those out first (without considering implementation details) so that we have something to review against, the ticket is a bit sparse, unfortunately.

packages/@react-spectrum/button/src/Button.tsx Outdated Show resolved Hide resolved
packages/@react-spectrum/button/src/Button.tsx Outdated Show resolved Hide resolved
packages/@react-spectrum/button/src/Button.tsx Outdated Show resolved Hide resolved
<PendingButtonContainerComponent {...props}>
<PendingButtonComponent
{...props}
aria-label="Aria label on button">
Copy link
Collaborator

@yihuiliao yihuiliao Oct 17, 2023

Choose a reason for hiding this comment

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

for this particular story on Chrome VO, the "pending" is never getting announced. my guess is because the aria-label on the button is never getting updated, and so the screenreader doesn't announce anything since nothing has changed presumably. i think a similar thing is happening with the "Notifications" pending button where the aria-label on the button is not getting updated so "pending" is never announced.

@rspbot
Copy link

rspbot commented Oct 20, 2023

@rspbot
Copy link

rspbot commented Nov 6, 2023

snowystinger and others added 2 commits November 13, 2023 11:46
* Alternative pending label approach

* remove extra labelledby attribute

* don't clobber button id

* remove some unneeded tests

* fix lint

---------

Co-authored-by: Will Powell <3374077+iamwillpowell@users.noreply.github.com>
# Conflicts:
#	packages/@react-spectrum/button/test/Button.test.js
@rspbot
Copy link

rspbot commented Nov 13, 2023

@rspbot
Copy link

rspbot commented Nov 13, 2023

@rspbot
Copy link

rspbot commented Nov 14, 2023

@snowystinger
Copy link
Member

snowystinger commented Nov 14, 2023

So I've found the Partial support for Safari which is giving me so many issues. It is demonstrated here https://a11ysupport.io/tests/tech__html__button-name-change#assertion-aria-aria-label_attribute-convey_name_change-html-button_element-vo_macos-safari You should now be able to SOMETIMES get a MacOS announcement using the VO Ctrl+Opt+Space.

I know we wait a little while to show the visible pending state, however, the only way to get an announcement right now is if the change takes place immediately on VO command click.

The reason we get multiple announcements is because we have multiple changes, and they kind of step on each other. We have aria-disabled, as well as aria/label change, and possibly visibility.
Unfortunately, I think we have to live with these for now.

You will also note that JAWS does not work, this is expected, even with live region. See link earlier in this comment.

@rspbot
Copy link

rspbot commented Nov 14, 2023

@rspbot
Copy link

rspbot commented Nov 14, 2023

@snowystinger
Copy link
Member

Linking another useful topic thread. nvaccess/nvda#7996

@yihuiliao
Copy link
Collaborator

Yeah I’m definitely able to get announcements with Safari + VO. Sometimes doesn’t work but much better than before. Seems like we will just have to live with the multiple announcements in Chrome + VO/Firefox + NVDA like you said. Also it seems like Firefox + NVDA doesn’t announce 50 anymore which is great!

I can’t remember if you added anything to prevent the controlled pending button getting announced unless there was focus. Seems like right now it announces even when there isn’t focus.

@rspbot
Copy link

rspbot commented Nov 17, 2023

@rspbot
Copy link

rspbot commented Nov 20, 2023

@rspbot
Copy link

rspbot commented Nov 21, 2023

@rspbot
Copy link

rspbot commented Nov 22, 2023

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

@rspbot
Copy link

rspbot commented Nov 22, 2023

@rspbot
Copy link

rspbot commented Nov 23, 2023

@rspbot
Copy link

rspbot commented Nov 23, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@snowystinger snowystinger merged commit 9d8c74a into main Nov 23, 2023
26 checks passed
@snowystinger snowystinger deleted the append-pending branch November 23, 2023 02:27
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

8 participants