Skip to content

DataViews Table - only output aria-label for media when item is clickable #73034

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.

Sign up for GitHub

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

Jump to bottom
Merged
talldan merged 4 commits into from
Nov 7, 2025
Merged

DataViews Table - only output aria-label for media when item is clickable #73034

talldan merged 4 commits into from
Nov 7, 2025

Conversation

@talldan
Copy link
Contributor

@talldan talldan commented Nov 6, 2025 •
edited
Loading

What?

Fixes a small issue with the DataViews table layout observed in #72914 (comment).

When a table layout's item is not clickable, an aria-label saying "Click item" is still being rendered. This PR should fix the issue.

How?

  • Adds some extra checks for isItemClickable( item ) and onClickItem | renderItemLink, to more closely match the logic that the grid layout has.
  • Also removes the 'Click item' part of the text. I don't think this adds anything for a screenreader user, who will already know they can click the item because it's a button. The text would be more useful if it told the user what clicking the item will do, but that's up to the implementation.

Testing Instructions

  1. Checkout the branch and run npm run storybook:dev to open the storybook locally
  2. Navigate to the DataViews story
  3. From the controls at the bottom toggle off 'hasClickableItems'
  4. Inspect the images and check that the wrapping element no longer has an aria label.

To reproduce the original bug you can also try checking out trunk, change this line to return false:

gutenberg/packages/dataviews/src/stories/dataviews.story.tsx

Line 103 in 0e79f37

isItemClickable={ () => true }

Then observe that the images in the table have a 'Click item' aria label on a non-interactive wrapping element.

@talldan talldan self-assigned this Nov 6, 2025
@talldan talldan requested a review from oandregal as a code owner November 6, 2025 08:19
@talldan talldan added [Type] Bug An existing feature does not function as intended [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Nov 6, 2025
@github-actions
Copy link

github-actions bot commented Nov 6, 2025 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: talldan <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: andrewserong <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@talldan talldan mentioned this pull request Nov 6, 2025
Merged
@talldan talldan force-pushed the update/dataviews-primary-column-aria-label branch from fbf88dc to 88aef93 Compare November 6, 2025 08:23
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

Flaky tests detected in 88aef93.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19129329998
📝 Reported issues:

oandregal
oandregal reviewed Nov 6, 2025
View reviewed changes
packages/dataviews/src/stories/dataviews.story.tsx
export const Default = ( { perPageSizes = [ 10, 25, 50, 100 ] } ) => {
export const Default = ( {
perPageSizes = [ 10, 25, 50, 100 ],
hasClickableItems = true,
Copy link
Member

@oandregal oandregal Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, appreciate providing a test case, that's really helpful.

Given the impact this has, is it necessary to have this in the story? Perhaps just providing a diff for testing is enough? The stories should be something other devs and designers use to see what's possible with DataViews, and the hasClickableItems argument doesn't seem very useful. What do you think?

Copy link
Contributor Author

@talldan talldan Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer the options, though agree that the presentation in the storybook isn't great. Configuring DataViews without clickable rows is a valid configuration that devs and designers might be interested in, so maybe its useful to someone.

It might also more easily allow us to see bugs like this (and #71220, which was also a very similar issue) for configurations that we don't have in production.

I'll merge with the option, but if you feel strongly about it I don't mind removing it in a separate PR.

oandregal
oandregal approved these changes Nov 6, 2025
View reviewed changes
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This logic is better.

I just left a comment about removing the storybook argument https://github.com/WordPress/gutenberg/pull/73034/files#r2498520933 but thought I'd pre-approve anyway.

andrewserong
andrewserong approved these changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, too!

@talldan talldan merged commit 5ac6cbc into trunk Nov 7, 2025
39 of 40 checks passed
@talldan talldan deleted the update/dataviews-primary-column-aria-label branch November 7, 2025 02:05
@github-actions github-actions bot added this to the Gutenberg 22.1 milestone Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oandregal oandregal oandregal approved these changes

@andrewserong andrewserong andrewserong approved these changes

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

Assignees

@talldan talldan

Labels

[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 22.1

Development

Successfully merging this pull request may close these issues.

4 participants

@talldan @oandregal @andrewserong