table a11y fix #4042
table a11y fix #4042
Conversation
anishkumar-bhut
requested review from
abpaul1993,
davidicus,
herleraja,
jessieyan and
sls-ca
as code owners
anishkumar-bhut
removed request for
abpaul1993,
davidicus,
herleraja,
jessieyan,
simson1 and
sls-ca
✅ Deploy Preview for carbon-addons-iot-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| .#{$prefix}--table-sort { | ||
| padding-left: 0; | ||
| padding-right: 0; | ||
| // padding-left: 0; |
There was a problem hiding this comment.
This can be removed if not needed
| padding-left: 0; | ||
| padding-right: 0; | ||
| // padding-left: 0; | ||
| padding-inline-start: 1rem; |
There was a problem hiding this comment.
| padding-inline-start: 1rem; | |
| padding-inline-start: $spacing-05; |
| .#{$prefix}--table-header-label, | ||
| .#{$prefix}--tooltip--definition { | ||
| padding-left: $spacing-05; | ||
| padding-left: 0; |
There was a problem hiding this comment.
Prefer using padding-inline-start instead
|
|
||
| return ( | ||
| // eslint-disable-next-line jsx-a11y/click-events-have-key-events | ||
| // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions |
There was a problem hiding this comment.
Isn't this clickable? Why all the rules to disable interaction?
Also, why isn't it a button?
| // .#{$prefix}--definition-term > span:last-child { | ||
| // padding-inline-start: 0rem; | ||
|
|
||
| // } |
There was a problem hiding this comment.
Please remove this if not needed
| TriggerElement = () => React.createElement(as, triggerProps, children); | ||
| } else { | ||
| // Option 3: Custom React component | ||
| TriggerElement = () => React.createElement(as, triggerProps, children); |
| } | ||
|
|
||
| // Common trigger props | ||
| const triggerProps = { |
There was a problem hiding this comment.
Ideally, this should be moved to outside of the component. if that's not possible, it should be a memo to avoid unnecessary re-renders
|
|
||
| // Add onClick for anchor tags | ||
| if (as === 'a' || (typeof as === 'string' && as.toLowerCase() === 'a')) { | ||
| triggerProps.onClick = (event) => { |
There was a problem hiding this comment.
onClick is not part of the expected props:
And there's no guard agains that. It should at least have an if to check if triggerProps.onClick exists, or an optional chaining
There was a problem hiding this comment.
I think this event is not clicking here it binding click event along with other props.
There was a problem hiding this comment.
This 3 prop will not came from props its dynamic adding if as == "a" passed and user can not passing this event from props. its binding run time with a.
| }; | ||
| // Add href for anchor tags if not provided | ||
| if (!rest.href) { | ||
| triggerProps.href = '#'; |
There was a problem hiding this comment.
This 3 prop will not came from props its dynamic adding if as == "a" passed and user can not passing this event from props. its binding run time with a.
|
|
||
| // Add type for button | ||
| if (as === 'button' || (typeof as === 'string' && as.toLowerCase() === 'button')) { | ||
| triggerProps.type = rest.type || 'button'; |
There was a problem hiding this comment.
This 3 prop will not came from props its dynamic adding if as == "a" passed and user can not passing this event from props. its binding run time with a.
2 participants
Closes #
Summary
This PR addresses multiple accessibility issues in the Table component, focusing on keyboard navigation, ARIA attributes, and screen reader support. The changes include refactoring the DefinitionTooltip component to support flexible trigger elements, improving column resize keyboard controls, and enhancing TableToolbar batch actions accessibility.
Change List
New utility module for keyboard event handling
Exported keys object with common key constants
match() function for checking keyboard events
Change List (commits, features, bugs, etc)
Acceptance Test (how to verify the PR)
Regression Test (how to make sure this PR doesn't break old functionality)
Things to look for during review
iotorbxclass prefix is using the prefix variabledata-testidattribute. New test ids should have test written to ensure they are not changed or removed.