fix(Label): extend clickable hit area to full label bounds (#12367)#12407
fix(Label): extend clickable hit area to full label bounds (#12367)#12407fallmo wants to merge 1 commit into
Conversation
When a Label has an onClick handler (no onClose), promote the outer element to a <button> so the full padding/border area is interactive. When onClose is also present, fall back to the original inner <button> structure to avoid nested buttons. Co-authored-by: Copilot <copilot@github.com>
WalkthroughThe Label component's outer wrapper type and click-handler binding now conditionally depend on the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-react-pr-12407.surge.sh A11y report: https://pf-react-pr-12407-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/Label/Label.tsx (1)
299-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNested focusable element inside outer button — keyboard accessibility regression.
When
onLabelClick && !onClose && isTooltipVisible(text truncated), the outer element is abutton(already in the tab order), but line 301 still appliestabIndex: 0to the innerspan, producing two consecutive tab stops for a single label. Before this PR the outer was aspan, so thetabIndex: 0on the innerbuttonwas harmless (one stop total). Now it creates a nested focusable element, which is an ARIA anti-pattern — and the tooltip itself is anchored to the inner element, so keyboard focus on the outer button won't trigger the tooltip anyway.The guard should skip
tabIndex: 0when the outer is the button (i.e., whenLabelComponent !== 'span'):♿ Proposed fix
- ...(isTooltipVisible && { tabIndex: 0 }), + ...(isTooltipVisible && LabelComponent === 'span' && { tabIndex: 0 }),Note:
LabelComponentis already resolved above this block, so this reference is valid. This also fixes the same pre-existing double-tab-stop foroverflow/addlabel variants as a bonus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Label/Label.tsx` around lines 299 - 302, The labelComponentChildProps currently adds tabIndex: 0 when isTooltipVisible, which creates a nested focusable element if the outer LabelComponent is a button; update the guard so tabIndex: 0 is only added when isTooltipVisible AND LabelComponent === 'span' (i.e., skip adding tabIndex when LabelComponent !== 'span'), modifying the labelComponentChildProps construction (referencing labelComponentChildProps, LabelComponent, isTooltipVisible, and styles.labelContent) so the inner element is not focusable when the outer element is already a button.
🧹 Nitpick comments (1)
packages/react-core/src/components/Label/Label.tsx (1)
341-365: ⚡ Quick winExtract the repeated
!!onLabelClick && !isEditable && !onClosecondition to a named variable.The sub-expression appears identically on lines 342, 363, and 365. A future edit that changes it in one place but not the others will silently break the feature.
♻️ Proposed refactor
+ const isOuterButtonClickable = !!onLabelClick && !isEditable && !onClose; + const LabelComponent = ( - isOverflowLabel || isAddLabel || (!!onLabelClick && !isEditable && !onClose) ? 'button' : 'span' + isOverflowLabel || isAddLabel || isOuterButtonClickable ? 'button' : 'span' ) as any;onClick={ - isOverflowLabel || isAddLabel || (!!onLabelClick && !isEditable && !onClose) + isOverflowLabel || isAddLabel || isOuterButtonClickable ? onLabelClick : undefined } {...(LabelComponent === 'button' && { type: 'button' })} - {...(isClickableDisabled && !!onLabelClick && !isEditable && !onClose && { disabled: true })} + {...(isClickableDisabled && isOuterButtonClickable && { disabled: true })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Label/Label.tsx` around lines 341 - 365, Introduce a named boolean (e.g., canTriggerLabelClick) to capture the repeated condition (!!onLabelClick && !isEditable && !onClose) and replace the three inline occurrences in LabelComponent, the onClick prop, and the disabled prop check with that variable; ensure LabelComponent selection still uses the combined condition (isOverflowLabel || isAddLabel || canTriggerLabelClick), onClick uses canTriggerLabelClick ? onLabelClick : undefined, and the disabled spread uses isClickableDisabled && canTriggerLabelClick to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-core/src/components/Label/Label.tsx`:
- Around line 299-302: The labelComponentChildProps currently adds tabIndex: 0
when isTooltipVisible, which creates a nested focusable element if the outer
LabelComponent is a button; update the guard so tabIndex: 0 is only added when
isTooltipVisible AND LabelComponent === 'span' (i.e., skip adding tabIndex when
LabelComponent !== 'span'), modifying the labelComponentChildProps construction
(referencing labelComponentChildProps, LabelComponent, isTooltipVisible, and
styles.labelContent) so the inner element is not focusable when the outer
element is already a button.
---
Nitpick comments:
In `@packages/react-core/src/components/Label/Label.tsx`:
- Around line 341-365: Introduce a named boolean (e.g., canTriggerLabelClick) to
capture the repeated condition (!!onLabelClick && !isEditable && !onClose) and
replace the three inline occurrences in LabelComponent, the onClick prop, and
the disabled prop check with that variable; ensure LabelComponent selection
still uses the combined condition (isOverflowLabel || isAddLabel ||
canTriggerLabelClick), onClick uses canTriggerLabelClick ? onLabelClick :
undefined, and the disabled spread uses isClickableDisabled &&
canTriggerLabelClick to preserve existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85a66cf1-0919-49b2-89e6-3b0b8402cd8d
📒 Files selected for processing (1)
packages/react-core/src/components/Label/Label.tsx
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Some comments that @mcoker or @jcmill may wanna weigh in on:
- The updates here do increase the horizontal clickable area, but the hover styling still only takes affect when hovering the inner element
- Link labels still have the issue raised
- Removable labels would still have the issue raised on the left/start side of the label. I don't think we should have the clickable area of the label itself be extended on the right/end side, because then it becomes an accessibility issue of the close button (which is already too small a clickable area when paired with clickable labels) being "within" the clickable area of the label itself. So it sort of becomes an inconsisten clickable area: basic clickable labels have a larger clickable area, link labels don't, and removable + clickable labels don't.
The last bullet point mainly has me wondering if this is something we need to/should fix. At some point we'll need to address the accessibility issue fo the close button size/proximity to a clickable label, which might mean increasing the space between the 2 a tad bit. If we do that, I'd wonder if the clickable area of the Label itself still ends up being an annoyance/looking odd. If the inconsistent clickable area isn't a huge deal, though, then the last bullet is rather moot.
3 participants
Description
Fixes patternfly/patternfly#8430
When a
<Label>has anonClickhandler, the outer<span class="pf-v6-c-label">has padding/border that is visually part of the label but clicking that region did
not trigger
onClick— only the inner<button class="pf-v6-c-label__content">was interactive.
Changes
onClickis provided and noonCloseis present, the outer element is promotedfrom a
<span>to a<button>, so the full visual area including padding and borderis part of the hit target. The inner element becomes a plain
<span>, removing thesemantically problematic nested button.
Before:
After:
disabledis now applied to the outer for the promoted clickable case. Previously it was applied to the inner via isClickableDisabled && isButton, but since the inner element is now a plain in this case, isButton is false and that guard no longer fires. This preserves the existing isDisabled behavior.Summary by CodeRabbit
onClosecallback, improving interaction behavior consistency when multiple event handlers are present.