Skip to content

frontend: ShowHideLabel: Fix aria attributes - #7267

Open
Alok-work23 wants to merge 2 commits into
kubernetes-sigs:mainfrom
Alok-work23:fix/show-hide-label-aria
Open

frontend: ShowHideLabel: Fix aria attributes#7267
Alok-work23 wants to merge 2 commits into
kubernetes-sigs:mainfrom
Alok-work23:fix/show-hide-label-aria

Conversation

@Alok-work23

Copy link
Copy Markdown

Summary

This PR fixes an accessibility issue in the ShowHideLabel component by moving the expand/collapse IconButton outside of the label element. The button now correctly controls the label text while keeping aria-expanded and aria-controls on the interactive element.

Related Issue

Fixes #7194

Changes

  • Updated ShowHideLabel to move the expand/collapse IconButton outside of the label .
  • Kept aria-expanded and aria-controls on the IconButton.
  • Updated the ShowHideLabel Basic and Expanded Storybook snapshots to reflect the new DOM structure.

Steps to Test

  1. Navigate to the ShowHideLabel Storybook stories.
  2. Open the Basic story and verify the long label is truncated with an expand button.
  3. Click the expand button and verify the full label text is displayed.
  4. Click the button again and verify the label collapses.
  5. Open the Expanded story and verify the label is initially expanded and can be collapsed.
  6. Verify the expand/collapse button has the appropriate aria-expanded and aria-controls attributes.

Screenshots (if applicable)

N.A.

Notes for the Reviewer

  • The change is focused on the accessibility semantics and DOM structure of ShowHideLabel.
  • The Storybook snapshots were updated to reflect the new element structure.

@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Alok-work23
Once this PR has been reviewed and has the lgtm label, please assign illume for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2026

@unlikelyzero unlikelyzero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for tracking down this aria fix — moving the IconButton out of the span it references is the right call, and it does correctly resolve the aria-controls/aria-expanded relationship.

A couple of things before merging though:

  1. Moving the button out of the <span> also changes its layout participation. When collapsed, the Box is display: flex, so the span and the button are now two separate flex items instead of the button being inline content inside the span. On multi-line text (which is the common case for long messages in table cells — Overview.tsx, ObjectEventList.tsx, CustomResourceInstancesList.tsx all use this), the button can end up detached as a stretched column beside the wrapped text instead of trailing right after the ellipsis. Can you check this visually in Storybook at a narrow width with a long string? If it's an issue, adjusting the Box's flex properties (e.g. flexWrap + alignItems: flex-end) or keeping the button flowing inline would fix it while still keeping the DOM structure change for the aria fix.

  2. frontend/.axe-storybook-baseline.test-a11y.json still lists both common/ShowHideLabel/Basic and common/ShowHideLabel/Expanded under aria-allowed-attr. Since that's an allowlist, CI isn't actually proving the violation is gone — if the fix were incomplete, or regressed later, it'd still pass silently. Can you run the axe check locally against these two stories and, if clean, remove those two baseline entries so CI enforces it going forward?

Separately (not blocking this PR, just flagging while I was in the code): needsButton in the useMemo unconditionally returns true when expanded is true, so if ShowHideLabel is ever initialized with show={true} on text shorter than maxChars, it renders a Collapse button that disappears the moment it's clicked. That logic predates this PR though, so probably worth its own follow-up rather than folding into this one.

Nice work on the core fix — just want the layout and CI-enforcement pieces closed out first.

@unlikelyzero

Copy link
Copy Markdown
Contributor

Filed the pre-existing needsButton bug I mentioned above as #7270, and opened #7271 to fix it separately.

@Alok-work23

Alok-work23 commented Aug 15, 2026

Copy link
Copy Markdown
Author

Thanks @unlikelyzero for the detailed review!

I'll address the two points before making further changes:

  • I'll check the collapsed layout at a narrow Storybook width with a long string and adjust the flex properties if needed so the expand/collapse button stays visually aligned with the wrapped text.
  • I'll remove the Basic and Expanded ShowHideLabel entries from the axe Storybook baseline and verify both stories pass cleanly.

I'll leave the existing needsButton behavior unchanged and keep that issue scoped to the separate follow-up in #7271.

@unlikelyzero

Copy link
Copy Markdown
Contributor

#7271

@aliok Would feel comfortable reviewing #7271?

@Alok-work23

Copy link
Copy Markdown
Author

#7271

@aliok Would feel comfortable reviewing #7271?

Yes, I'd be happy to review #7271.

@Alok-work23

Copy link
Copy Markdown
Author

#7271

@aliok Would feel comfortable reviewing #7271

I think you tagged someone other person.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Moves the ShowHideLabel toggle outside the text span to correct ARIA semantics.

Changes:

  • Separates the interactive button from the label text.
  • Updates Basic and Expanded snapshots.
  • CI was still running with no reported failures.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
ShowHideLabel.tsx Updates DOM structure and ARIA ownership.
ShowHideLabel.Basic.stories.storyshot Updates collapsed-state snapshot.
ShowHideLabel.Expanded.stories.storyshot Updates expanded-state snapshot.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread frontend/src/components/common/ShowHideLabel/ShowHideLabel.tsx
Comment thread frontend/src/components/common/ShowHideLabel/ShowHideLabel.tsx Outdated
@Alok-work23
Alok-work23 force-pushed the fix/show-hide-label-aria branch from 2c1bc4e to fb5edb3 Compare August 17, 2026 05:42
@Alok-work23

Copy link
Copy Markdown
Author

hii @illume ,
Please review this pr once.
I have made suggested changes by copilot.

Thank You !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

3 participants