- 
                Notifications
    
You must be signed in to change notification settings  - Fork 105
 
fix(table): fix table accessibility issue-2813 #2885
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
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
          
        
      | <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
          
        
      | 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:
