-
Notifications
You must be signed in to change notification settings - Fork 218
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
Aria notify updates #943
Aria notify updates #943
Conversation
not able to silence or flush existing notifications, as that behavior is not supported in ARIA live regions. In the | ||
case that the web browser does not yet support `ariaNotify`, it is the responsibility of the web author to detect and | ||
fallback to ARIA live regions. The above conversion may serve as a guide on how to do so. One can detect whether or not | ||
`ariaNotify` is supported by checking if the method exists on the document or element in question: | ||
|
||
``` | ||
```js |
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.
Does this need to handle three cases now? No ariaNotify, ariaNotify without interrupt, and ariaNotify with interrupt.
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.
Yeah actually that is probably a good point. Although for browsers that don't support it, it wouldn't probably parse anyways.
I think if we do add interrupt
later, we will need to add some way to do feature detection to the ariaNotify
API itself. Which reminds me, Alex had pointed me to https://developer.mozilla.org/en-US/docs/Web/API/ClipboardItem/supports_static which is something we should potentially consider adding to our proposal for interrupt
to provide authors a way to detect if a browser supports that property yet
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.
Only a couple of minor comments, but otherwise, this looks really great. Thanks for helping with all of these updates!
pointless given no matter the list, it will always fall short. | ||
|
||
Possible examples of predefined `notificationId` could be something like: | ||
Possible examples of predefined `type` could be something like: |
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 wonder if we should include some of the possible type categories that the Github team helped put together in https://docs.google.com/document/d/1kNtM_tguuaYKPrhd8cAWfwxj8CJVnWONSZLQp99dtlU/edit?usp=sharing
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 don't have access, Just requested it, can add more in a future PR.
Sweeping changes based on ARIA WG discussions:
priority
values -none
tonormal
andimportant
tohigh
.notificationId
totype
(doesn't seem like there was consensus here, but thattype
was a step in the right direction).sandbox
property name more clear.interrupt
to future considerations.type
to future considerations.I noticed a bit too late that this file had linebreaks at 120 characters. I inadvertently removed some of those and didn't try to keep consistent with that (it's markdown not C++ 😅), so sorry for any review pain that causes. It's probably worth looking at the individual commit diffs for review to remove the noise of the unrelated parts, specifically the "Move interrupt and type to future options." commit has the most major changes.
One lingering note I think we might need to fix - Moving
interrupt
to the future means we probably need to expand the ARIA live region fallback for that to handle the three cases - noariaNotify
,ariaNotify
withoutinterrupt
, andariaNotify
withinterrupt
.