Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Atharv-G-Kulkarni
Copy link

What I did

  • Fixed issues mentioned in the issue.
  • Improved accessibility score of pf-table (Table, Expandable Rows Compound, Sortable) from 91 to 100

Before:
Screenshot from 2024-12-06 12-11-29
Screenshot from 2024-12-06 12-11-53
Screenshot from 2024-12-06 12-12-17

After:
Screenshot from 2024-12-06 12-08-09
Screenshot from 2024-12-06 12-09-16
Screenshot from 2024-12-06 12-09-40


  • Improved accessibility score of pf-table (Expandable Rows) improved from 81 to 90.

Before:
Screenshot from 2024-12-06 12-12-55

After:
Screenshot from 2024-12-06 12-10-17

Copy link

changeset-bot bot commented Dec 6, 2024

⚠️ No Changeset found

Latest commit: 0136786

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Atharv-G-Kulkarni Atharv-G-Kulkarni changed the title 2813: Fix table accessibility issue fix(table): Fix table accessibility issue-2813 Dec 6, 2024
@Atharv-G-Kulkarni Atharv-G-Kulkarni changed the title fix(table): Fix table accessibility issue-2813 fix(table): fix table accessibility issue-2813 Dec 6, 2024
Copy link
Contributor

github-actions bot commented Dec 6, 2024

✅ Commitlint tests passed!

More Info
{
  "valid": true,
  "errors": [],
  "warnings": [],
  "input": "fix(table): fix table accessibility issue-2813"
}

Copy link

netlify bot commented Dec 6, 2024

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 1e2f9a1
🔍 Latest deploy log https://app.netlify.com/sites/patternfly-elements/deploys/67530d561e7b5e00080c3f59
😎 Deploy Preview https://deploy-preview-2885--patternfly-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -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"
Copy link
Collaborator

@zeroedin zeroedin Dec 6, 2024

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.

Suggested change
role="button"

Copy link
Author

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.

@@ -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"
Copy link
Collaborator

@zeroedin zeroedin Dec 6, 2024

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.

Suggested change
aria-label="Expand Button"
label="Expand Button"

Copy link
Author

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.

Copy link
Collaborator

@zeroedin zeroedin left a 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.

@bennypowers
Copy link
Member

can the button's aria-controls reference be to the row on

<div id="container" role="${ifDefined(this.expandable ? 'row' : undefined)}">
?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants