-
Notifications
You must be signed in to change notification settings - Fork 599
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
feat(Announce): add support for computing text equivalence #5724
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
export const WithCustomElement = () => { | ||
return ( | ||
<Announce> | ||
<RelativeTime date={new Date('2020-01-01T00:00:00Z')} noTitle={true} /> |
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.
Note, the announcement in this story will still be the default date (and not the "on ..." message) since when the story renders and gets announced the custom element hasn't updated yet.
size-limit report 📦
|
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.
Wow, thank you so much for following up so quickly! I think this is a good starting point! ✨
I wonder if the announcement should be somewhat close to the output we'd get if we added role='alert'
directly on an element.
- I'm actually not sure how
aria-hidden
elements are handled byrole="alert"
, so I think it'd be good to test that with a screen reader, and potentially align the output here! - Developers shouldn't be adding live regions on anything and everything (e.g. complex interactive widgets) since semantics aren't conveyed through live regions. I wonder how much we need to support it 🤔
} else if (element.textContent) { | ||
value = element.textContent | ||
type TextEquivalentOptions = { | ||
allowHidden: boolean |
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 see that setting allowHidden
to true
would include aria-hidden
elements in the announcement and exclude hidden
elements.
What do you think of renaming this to allowAriaHidden
?
This makes sense to me, since I can't think of a usecase for announcing hidden
elements... maybe if one wants to announce something transient that shouldn't appear in the DOM? But in that case I think they could use the announce
helper directly. Thoughts?
* | ||
* @see https://www.w3.org/TR/accname-1.2/#computation-steps | ||
*/ | ||
function computeTextEquivalent( |
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.
This seems like a good start - I think we can refine this as we identify different usecases (alongside stories/tests).
if (role === 'combobox' || role === 'listbox') { | ||
const options = elementOrText.querySelectorAll('option[aria-selected="true"]') |
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 how much if/how much support we need for complex interactive controls.
Ideally, consumers aren't wrapping entire interactive components with a live region, since semantics won't properly be conveyed 🤔
Context #5723
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist