Skip to content

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

@changeset-bot
Copy link

changeset-botbot 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-kulkarniatharv-g-kulkarni changed the title 2813: Fix table accessibility issuefix(table): Fix table accessibility issue-2813Dec 6, 2024
@atharv-g-kulkarniatharv-g-kulkarni changed the title fix(table): Fix table accessibility issue-2813fix(table): fix table accessibility issue-2813Dec 6, 2024
@github-actions
Copy link
Contributor

github-actionsbot commented Dec 6, 2024

✅ Commitlint tests passed!

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

@netlify
Copy link

netlifybot commented Dec 6, 2024

Deploy Preview for patternfly-elements ready!

NameLink
🔨 Latest commit1e2f9a1
🔍 Latest deploy loghttps://app.netlify.com/sites/patternfly-elements/deploys/67530d561e7b5e00080c3f59
😎 Deploy Previewhttps://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.

<pf-buttonid="toggle-button"
aria-expanded=${String(this.expanded)as'true'|'false'}
plain
role="button"
Copy link
Contributor

@zeroedinzeroedinDec 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"

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.

aria-expanded=${String(this.expanded)as'true'|'false'}
plain
role="button"
aria-label="Expand Button"
Copy link
Contributor

@zeroedinzeroedinDec 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"

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
Contributor

@zeroedinzeroedin 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

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

Sign up for freeto 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

@atharv-g-kulkarni@bennypowers@zeroedin