-
Notifications
You must be signed in to change notification settings - Fork 139
Add ariaNotify #2577
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
base: main
Are you sure you want to change the base?
Add ariaNotify #2577
Conversation
✅ Deploy Preview for wai-aria ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@cookiecrook, @jcsteh, @front-endian, thanks in advance for reviewing this change! While I'll be out of the office from tonight (July 29th) until next Tuesday (Aug 5th), I'll be able to answer questions or make clarifications, just with a delay. Once I am back on July 11th, I can apply any feedback to this PR. Looking forward to hearing your thoughts! @scottaohara, while I'll be out, would it make sense for this to be added to the agenda for this or next week's meeting, or would that only make sense if reviewers had something they would like to discuss specifically? I just want to ensure this keeps momentum. Adding a few relevant links in case they are helpful:
|
@keithamus why is there also another PR with the same name, should we close it as a duplicate? #2211 |
@cookiecrook, @front-endian, friendly ping to PTAL at this when you get the chance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor suggestions and questions around two bits that might be able to be written a bit clearer, but everything else looked good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wedging the door to prevent the merge (sorry), since there are now two approving reviews. I am trying to get a Standards Position answer for you by later this week. Won't be much longer.
…in a separate issue.
…e Content/Control view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebKit Standards position for ariaNotify coming in later today will be Supports (with remaining concerns).
The WebKit Standards Position for ariaNotify has been posted outlining ongoing feedback. WebKit/standards-positions#370 (comment)
There is an official position of Support on Mozilla's side. mozilla/standards-positions#1049 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM.
This was discussed with James Craig at Apple here: w3c/aria#2577 (comment) Today, there is no functional difference with this change, but there could be in the future, especially considering that the element/node this is called on is intended to inform ATs about the language the announcement should be read in. To facilitate, `PostAnnouncementNotification` was refactored to take an `id` instead of a NSWindow*, making it closer match the API it calls, `NSAccessibilityPostNotificationWithUserInfo`. This part is pure refactoring, and will not have any behavioral change. Change-Id: I1c31273338602d420c6858decd680c295f2eb7ce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6897470 Reviewed-by: Benjamin Beaudry <[email protected]> Commit-Queue: Jacques Newman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1508466}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few outstanding questions, but no blocking changes needed. Thanks again for the follow-through.
<th>Language</th> | ||
<td> | ||
<p> | ||
User agents and assistive technologies MUST determine the language of the announcement by taking the first valid BCP 47 language tag from the following sources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: Would it be useful to link to a definition of “BCP 47 language tag”?
I looked for something we could xref with respec, but https://respec.org/xref/?term=BCP+47 returns no results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments; none blocking. Excited to get this merged! 🚀
One additional question—the PR body says:
…the v2 roadmap includes ideas that are not yet fully implementable…
Where can folks review “the v2 roadmap”?
❧
github/arianotify-polyfill will need a couple updates in light of this:
- It always uses a
polite
live region (rather than varying based onpriority
). - It doesn’t check whether the node is in the accessibility tree.
Co-authored-by: Clay Miller <[email protected]>
Other than the open issues, the "future considerations" section of the edge explainer goes over the kinds of things we think aria notify should support, though as mentioned, there isn't consensus on the details yet. Much of the v2+ functionality will require additional API or screen reader support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the recent edits look good!
This adds the mechanics for introducing the ariaNotify API on Elements, which is a function that will map to various AT APIs, as defined in core-aam.
Aria Notify Explainer
Original PR: #2211
Test, Documentation and Implementation tracking
Once this PR has been reviewed and has consensus from the working group, tests should be written and issues should be opened on browsers. Add N/A and check when not applicable.
Preview | Diff