fix: Add aria attributes to popover control #796
Conversation
* update tests and snapshots
|
Deploy preview for fundamental-react ready! Built with commit 861c4ac |
prsdthkr
added
A11y
src/Popover/Popover.js
Outdated
| // eslint-disable-next-line no-undefined | ||
| if (newPopperProps.id === undefined || newPopperProps.id === null) { | ||
| Object.assign(newPopperProps, { id: this.ariaControls }); | ||
| } |
There was a problem hiding this comment.
I wonder if this could be simplified and avoid the safe checking by:
- Setting
defaultPropsforpopperProps = {} - Just assigning an
idinline?
const id = popperProps.id || this.ariaControls;
...
popperProps={{ ...popperProps, id }}| 'aria-haspopup': true, | ||
| 'aria-controls': newPopperProps.id, | ||
| 'aria-expanded': this.state.isExpanded, | ||
| 'aria-haspopup': !!type ? type : true, |
There was a problem hiding this comment.
Is this right? The positive/first ternary return type seems like it could end up being a string.
I imagine the intention is a boolean, but when should this be false? (there should probably be a test for this)
Aaaaand I stand corrected! Wasn't aware haspopup takes boolean values as well as enum types.
src/Popover/Popover.js
Outdated
|
|
||
| //A generated shortId as fallback, incase props.popperProps.id is unset. | ||
| //This ID binds the popover and it's control by 'aria-controls'. | ||
| this.ariaControls = shortId.generate(); |
There was a problem hiding this comment.
ariaControls is a bit confusing - maybe controlId would be better?
There was a problem hiding this comment.
Since it is not the control's ID how about popoverId instead? or popoverContainerId?
There was a problem hiding this comment.
Yeah, I think popoverId makes sense. It's a bit difficult to distinguish between that and popperProps.id, but I think it's clear enough that one comes from props and the other is defined here as a fallback.
Can you fix a couple small typos while you're here? in case & its
Description
Add following aria attributes to popover control/target.
This allows screen readers and other assistive technologies to understand the popover interactions semantically.
fixes #791