fix(aria): make aria-* component props consistent #1731
Conversation
redallen
added
Breaking change 💥
Codecov Report
@@ Coverage Diff @@
## master #1731 +/- ##
==========================================
+ Coverage 82.67% 82.69% +0.01%
==========================================
Files 598 598
Lines 6626 6639 +13
Branches 75 75
==========================================
+ Hits 5478 5490 +12
- Misses 1118 1119 +1
Partials 30 30
Continue to review full report at Codecov.
|
redallen
changed the title
|
PatternFly-React preview: https://1731-pr-patternfly-react-patternfly.surge.sh |
packages/patternfly-4/react-core/src/components/AboutModal/AboutModal.test.js
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Alert/AlertActionCloseButton.js
Outdated
Show resolved
Hide resolved
|
Yeah, the styles are generally wrong on the new doc site. Please add that issue (and any others you find) to #1724 ! |
| id={id} | ||
| className={css(styles.dataListExpandableContent, className)} | ||
| hidden={isHidden} | ||
| aria-label={ariaLabel} |
There was a problem hiding this comment.
This might be out of scope for this PR and belong in a separate issue...
The props documentation states:
| Name | Type | Required | Default | Description |
|---|---|---|---|---|
| aria-label | string | Adds accessible text to the DataList toggle |
But the description seems to describe the DataListToggle component. In this context, it's providing an accessible label for the expandable content section.
And when looking at the DataListToggle component the following are documented:
| Name | Type | Required | Default | Description |
|---|---|---|---|---|
| aria-labelledby | string | '' | Adds accessible text to the DataList toggle | |
| aria-label | string | 'Details' | Adds accessible text to the DataList toggle |
But only aria-label is accepted as a prop. If I provide a value for aria-labelledby on the component, that value doesn't display in the html.
Currently, aria-labelledby is defined on the html element only if the consumer doesn't define aria-label. If aria-label is defined, then the aria-labelledby attribute is removed from the html. This is as designed (e.g. either the consumer uses our default label, which we generate using aria-labelledby, or they explicitly define the full text string using aria-label).
There was a problem hiding this comment.
You are correct, it's ignored. The old logic is:
aria-labelledby={ariaLabel !== "Details" ? null : ${rowid} ${id}}
I'm changing it to
if (ariaLabel === "Details") {
ariaLabelledBy = `${rowid} ${id}`;
}
since it probably wasn't intended for the property to do nothing.
There was a problem hiding this comment.
Thanks! These updates work as expected except for the following...
When I define aria-label on the DataListToggle, in the PR preview I will see aria-labelledby included without a value.
In the current implementation, aria-labelledby is not included on the element at all. This is the expected behavior.
As for removing aria-labelledby from the documentation for DataListToggle, should I create a separate issue for that?
| rememberMeAriaLabel: props => { | ||
| if (props.rememberMeLabel && !props.rememberMeAriaLabel) { | ||
| return new Error('rememberMeAriaLabel is required with the Remember me checkbox'); | ||
| 'aria-label': props => { |
There was a problem hiding this comment.
This one might also be a separate issue, but this attribute should be removed from this checkbox. In this case, the aria-label is overwriting the text defined in the <label>.
Let me know if an issue is needed for this.
There was a problem hiding this comment.
Also, there's an issue in the preview. This error is displaying:
SyntaxError: Invalid or unexpected token
I'm not sure if this is what's causing the issue, but I see that this prop is still being defined on LoginForm even though you changed it to aria-label:
rememberMeAriaLabel="Remember me Checkbox"
There was a problem hiding this comment.
Why should the attribute be removed? This is starting to get outside the scope of this PR, so a separate issue would be nice.
There was a problem hiding this comment.
👍 I created #1737 for my first comment.
But the second comment might still be relevant to this PR?
There was a problem hiding this comment.
The login page has been broken since #1656 (see preview). Fixing it is a part of #1724 which is next on my list.
packages/patternfly-4/react-core/src/components/Select/CheckboxSelect.js
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Select/GroupedCheckboxSelectInput.js
Show resolved
Hide resolved
|
Let me know if there are still any problem with the Modal or Select, but I think updating them in the docs fixed them! The preview just now redeployed with the changes. |
| rightScrollAriaLabel: 'Scroll Right', | ||
| 'aria-label': null, | ||
| 'aria-label-left-scroll': 'Scroll left', | ||
| 'aria-label-right-scroll': 'Scroll Right', |
There was a problem hiding this comment.
When I define aria-label-left-scroll as a prop on the Tabs component, I'm not seeing that string included on the button.
| const unique_id = id ? id : getUniqueId(); | ||
| return variant === TabsVariant.nav | ||
| ? <TabsWithNav ariaLabel={ariaLabel} id={unique_id} {...props} /> | ||
| ? <TabsWithNav id={unique_id} {...props} /> |
There was a problem hiding this comment.
Currently there's an example labeled Accessible primary tabs with aria-label that shows the <nav> variant with aria-label. I'm not seeing this example in the preview.
and remove some unused ones
|
The issues with Modal and Tabs are resolved now! 🎉 |
| className: PropTypes.string, | ||
| onClose: PropTypes.func, | ||
| 'aria-label': PropTypes.string | ||
| 'aria-label-button': PropTypes.string |
There was a problem hiding this comment.
Related to discussion in slack, let's use ariaLabelClose instead.
| position: PropTypes.oneOf(Object.values(ApplicationLauncherPosition)), | ||
| /** Adds accessible text to the button. Required for plain buttons */ | ||
| 'aria-label': PropTypes.string | ||
| 'aria-label-button': PropTypes.string |
There was a problem hiding this comment.
Let's use ariaLabelToggle instead.
| children: PropTypes.node, | ||
| /** Aria Label for close button */ | ||
| 'aria-label': PropTypes.string, | ||
| 'aria-label-button': PropTypes.string, |
There was a problem hiding this comment.
Let's use ariaLabelClose instead.
| toggleText: PropTypes.string, | ||
| /** aria-label for the Context Selector Search Button */ | ||
| 'aria-label': PropTypes.string, | ||
| 'aria-label-search': PropTypes.string, |
There was a problem hiding this comment.
Let's use ariaLabelSearch instead.
| /** Callback function to handle the side nav toggle button */ | ||
| 'aria-label': PropTypes.string, | ||
| /** aria-label for the button */ | ||
| 'aria-label-button': PropTypes.string, |
There was a problem hiding this comment.
Let's use ariaLabelNavToggle instead.
|
Rather than try to find these inline and be wrong, here are a few more prop names to change based on the slack discussion.
|
|
With the Typescript conversion, this PR is in a terrible state. The issue still exists and can be resolved in a breaking change release later. |
5 participants
What: Resolves #1529. Resolves jest console errors related to aria props.
Additional issues: Check snapshots to make sure non-custom elements are not changed.