-
Notifications
You must be signed in to change notification settings - Fork 97
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
fix(table): fix table accessibility issue-2813 #2885
base: main
Are you sure you want to change the base?
fix(table): fix table accessibility issue-2813 #2885
Conversation
|
✅ Commitlint tests passed!More Info{
"valid": true,
"errors": [],
"warnings": [],
"input": "fix(table): fix table accessibility issue-2813"
} |
✅ Deploy Preview for patternfly-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
elements/pf-table/pf-tr.ts
Outdated
@@ -110,6 +110,8 @@ export class PfTr extends LitElement { | |||
<pf-button id="toggle-button" | |||
aria-expanded=${String(this.expanded) as 'true' | 'false'} | |||
plain | |||
role="button" |
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.
We shouldn't need to set role here as this gets applied at element internals level if the variant is not set to link
, aka role should just be automatically set by the pf-button
component itself.
role="button" |
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.
Thank you for your suggestion.
I have removed role="button" and pushed the changes.
elements/pf-table/pf-tr.ts
Outdated
@@ -110,6 +110,8 @@ export class PfTr extends LitElement { | |||
<pf-button id="toggle-button" | |||
aria-expanded=${String(this.expanded) as 'true' | 'false'} | |||
plain | |||
role="button" | |||
aria-label="Expand Button" |
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.
pf-button
has an API for adding the accessible label. We'd use label
here instead of using the aria-label
on the host.
aria-label="Expand Button" | |
label="Expand Button" |
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.
Thank you for your suggestion.
I have changed aria-label to label and pushed the changes.
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.
So enacting the requested changes myself locally I think I opened a broader issue here. Because we are using button as an expander, and using aria-expanded
on it we equally need a aria-controls
which isn't currently being applied. Also due to the fact the element being opened#expansion
is across shadow dom roots I'm going to assume we are going to run into cross root aria issues with that.
@adamjohnson when you get the chance lets review this together.
can the button's
|
1e2f9a1
to
7f189ae
Compare
What I did
Before:
After:
Before:
After: