Dialogs aria-labelledby#278
Conversation
…to the dom, label the dialog with that same id
|
|
||
| export function useSlotId(): string { | ||
| let [id, setId] = useState(useId()); | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I think this is one of those times where this is the more appropriate of the two. We can change the id back to null if it isn't in the dom before the repaint. I don't know how it affects voice over though, this may still be an unnecessary optimization.
|
Build successful! 🎉 |
| useLayoutEffect(() => { | ||
| let setCurr = map.get(id); | ||
| if (setCurr && !document.getElementById(id)) { | ||
| setId(null); |
There was a problem hiding this comment.
So if the id does not find a home in the DOM, we set it back to null?
| {[`spectrum-Dialog--${sizeVariant}`]: sizeVariant}, | ||
| otherProps.className | ||
| )} | ||
| aria-labelledby={titleId} |
There was a problem hiding this comment.
Can the Dialog be labelled using aria-label or and explicit aria-labelledby prop, without it being overwritten as null? I'm thinking of the DatePicker use case where the DialogTrigger supplies the label for the Dialog.
There was a problem hiding this comment.
I'm not sure about this, how would it work and when would we do one vs the other?
What if there is a DialogTrigger that supplies a labelled by AND there is a title in the dialog, do we want both?
I don't think this is currently interfering with aria-label, it's only forcing aria-labelledby
There was a problem hiding this comment.
explicitly setting aria-labelledby on the Dialog should take precedence.
There was a problem hiding this comment.
Also, aria-labelledby will take precedence over aria-label, so the internally set aria-labelledby referring to the slot, will override an explicitly set aria-label on the Dialog itself.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| function BaseDialog({children, slots, size, role, ...otherProps}: SpectrumBaseDialogProps) { | ||
| let ref = useRef(); | ||
| let titleId = useSlotId(); | ||
| titleId = otherProps['aria-label'] ? undefined : titleId; |
There was a problem hiding this comment.
The logic here and below (setting aria-labelledby) should go in useDialog.
| hero: {UNSAFE_className: styles['spectrum-Dialog-hero']}, | ||
| header: {UNSAFE_className: styles['spectrum-Dialog-header']}, | ||
| heading: {UNSAFE_className: styles['spectrum-Dialog-heading']}, | ||
| heading: {UNSAFE_className: styles['spectrum-Dialog-heading'], id: titleId}, |
There was a problem hiding this comment.
return titleProps from useDialog with this id
|
Build successful! 🎉 |
|
Build successful! 🎉 |
5 participants
pass an id through slots to the header, if it successfully renders into the dom, label the dialog with that same id
Closes https://jira.corp.adobe.com/browse/RSP-1583
✅ Pull Request Checklist:
📝 Test Instructions:
Run storybook, go to dialog stories
Inspect the title of the dialog and verify that it's id is the same as the role=dialog aria-labelledby
🧢 Your Team: