feat(TreeView): add support for disabled TreeViewListItems #12140
Conversation
|
Preview: https://pf-react-pr-12140.surge.sh A11y report: https://pf-react-pr-12140-a11y.surge.sh |
There was a problem hiding this comment.
Some general comments after testing and tinkering in the preview:
- For the "Single selectable" tree view (where the entire node toggles expansion rather than selection, if it expands), the disabled state isn't immediately announced. I have to 1) go into the tree view, 2) navigate to the tree item, then 3) go further into that tree item (rather than navigating forward/backward) to get to the button inside, which then announes a disabled state. I'm not sure if users would typically do that.
- For this, what we could do instead of putting
disabledon the button, putaria-disabled="true"on therole="treeitem"element
- For this, what we could do instead of putting
- For the separate expand/select buttons from "With separate selection and expansion" example, sort of the same issue as above, except the other issue is that the navigation as noted above is basically required with VO for this implementation. I have to go deeper into a tree item in order to know that there are separate expand/collapse and select buttons (which, idk if that's really a great implementation in the first place). Via plain keyboard it works okay as I can go to the separate expand and select buttons, but for VO it's not as straight forward.
I'm not sure what the best approach would be off the top of my head. For the more basic tree view, applying aria-disabeld to the treeitem would be better than putting disabled on the button. For an implementation where expansion and selection are separate actions within a tree item, that may not work, but at the same time it may not be clear that you have to go further into the tree item for additional/separate actions. I might want to say that allowing this mixed state of "one action is disabled, but not the other" isn't advisable - either the entire tree item is disabled or it isn't. That would mean we could just apply aria-disabled to the treeitem and have it exposed as disabled far easier for users.
| /** Flag indicating if the tree view item is disabled. */ | ||
| isDisabled?: boolean; | ||
| /** Flag indicating if the tree view item toggle is disabled. */ | ||
| isToggleDisabled?: boolean; |
There was a problem hiding this comment.
Would we want isDisabled to disable everything (selection and expansion)? Might just be me, just that's what I expected to happen when passing isDisabled, but that only disabled selection for the example where the selection is a separate action; for the default behavior isDisabled works as expected since there's only one button.
Or would we want to have isToggleDisabled and isSelectionDisabled props instead?
There was a problem hiding this comment.
Maybe a prop description update would work ("For tree view items with separated toggle and selection behaviors, this will disable selection.")? I think I had gone with a plain isDisabled because for tree view items where selection isn't separate, everything is disabled.
|
TLDR of this comment: how we currently have our TreeView component setup, the way this PR goes about disabling stuff might be one of the better ways to handle it. The longer story: So I tried testing out with NVDA a bit as well and looking a little more, and at least based on whatever settings I have set in both NVDA and VO, the interaction experiences are fairly different. In NVDA I can use arrow keys alone to navigate the tree view items, i.e. arrow keys will essentially focus the button elements within each tree item (whether it be the entire button that expands, or separately the expand button and selectable button). VoiceOver from what I can tell I need to use VO keyboard shortcuts to navigate since it's being announced as a table, in which case my previous comment of it being difficult to tell that there are separate expand/select actions still seems to stand (and in that case, the disabled state may not get announced since VO focus i on the tree item, not the button element within the tree item. Though NVDA can get a bit complicated as well when navigating via their shortcuts; if I use NVDA navigation within the treeview and try to move browser/system focus to the treeitem that has NVDA focus, I still have to press the Tab key to place focus on the actual button element to interact properly. I can't be sure if this is how a user would actually interact/navigate in a treeview, though. As an addendum to one of my previous comments, making the FWIW, WCAG has this navigation tree view example that has a separate selection/expansion sort of implementation. Essentially, the caret for expansion isn't in a semantic element, it looks to rely solely on mouse clicking the icon to expand it without selection while keyboard users need to use left/right arrow keys to expand/collapse. Also their expected keyboard interactions can e found on their tree view pattern page. |
| className={css( | ||
| styles.treeViewNode, | ||
| isSelected && styles.modifiers.current, | ||
| Component === 'button' && isDisabled && 'pf-m-disabled' |
There was a problem hiding this comment.
CSS update todo:
- Put disabled css class on overall node for text styling & toggle element for toggle styling.
- For typical nodes without separate behaviors, have
isDisabledapply both disabled css to node and toggle button elements.
kmcfaul
force-pushed
the
treeview-disabled
branch
from
f25dfcb to
30a649e
Compare
WalkthroughAdds two optional flags, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TreeView
participant TreeViewListItem
participant ToggleButton
participant AppCallbacks
User->>TreeView: click item / toggle
TreeView->>TreeViewListItem: render item (props include isDisabled, isToggleDisabled)
alt isDisabled true
TreeViewListItem-->>User: apply aria-disabled / pf-m-disabled, ignore select/click
else isToggleDisabled true and toggle clicked
TreeViewListItem->>ToggleButton: render disabled toggle (button[disabled])
ToggleButton-->>User: visual disabled, ignore expand/collapse
else normal
TreeViewListItem->>AppCallbacks: invoke onSelect/onExpand/onCollapse
AppCallbacks-->>TreeView: update state (expanded/selected)
TreeView-->>TreeViewListItem: re-render with updated props
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/TreeView/TreeViewListItem.tsx (1)
309-332: Memoization comparator missingisDisabledandisToggleDisabled.The memoization comparator does not include
isDisabledorisToggleDisabledin its prop comparisons. WhenuseMemois enabled, changes to these props won't trigger a re-render, resulting in stale disabled states.🔎 Proposed fix
prevProps.action !== nextProps.action || prevProps.parentItem !== nextProps.parentItem || - prevProps.itemData !== nextProps.itemData + prevProps.itemData !== nextProps.itemData || + prevProps.isDisabled !== nextProps.isDisabled || + prevProps.isToggleDisabled !== nextProps.isToggleDisabled ) { return false; }
🧹 Nitpick comments (2)
packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx (2)
419-427: MoveuserEvent.setup()inside tests orbeforeEach.Calling
userEvent.setup()at the describe block level means the same user instance is shared across all tests. This can lead to state leakage between tests. Best practice is to call it inside each test or inbeforeEach.🔎 Proposed fix
describe('isDisabled prop', () => { - const user = userEvent.setup(); const onSelectMock = jest.fn(); const onExpandMock = jest.fn(); const onCollapseMock = jest.fn(); beforeEach(() => { jest.clearAllMocks(); }); test('Renders button with disabled attribute and pf-m-disabled class when isDisabled is true', () => { render(<TreeViewListItem isDisabled {...requiredProps} />); // ... }); test('Does not call onSelect when isDisabled is true', async () => { + const user = userEvent.setup(); render(<TreeViewListItem isDisabled onSelect={onSelectMock} {...requiredProps} />); // ... });Apply the same pattern to the
isToggleDisableddescribe block (lines 530-537).
627-636: Test placed in potentially confusing location.This test verifies that
isDisabledappliespf-m-disabledto the toggle span in the default TreeViewListItem case, but it's located within theisToggleDisableddescribe block. Consider moving it to theisDisableddescribe block for clarity, or add a comment explaining the relationship.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-core/src/components/TreeView/TreeView.tsxpackages/react-core/src/components/TreeView/TreeViewListItem.tsxpackages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx (1)
packages/react-core/src/components/TreeView/TreeViewListItem.tsx (1)
TreeViewListItem(287-335)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (7)
packages/react-core/src/components/TreeView/TreeView.tsx (2)
38-41: LGTM! Clear prop documentation.The new
isDisabledandisToggleDisabledprops are well-documented and correctly added to theTreeViewDataIteminterface.
165-166: Props correctly forwarded to TreeViewListItem.The
isDisabledandisToggleDisabledvalues from item data are properly passed through to theTreeViewListItemcomponent.packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx (1)
418-659: Good test coverage for disabled functionality.The tests comprehensively cover various combinations of
isDisabledandisToggleDisabledwithhasCheckbox,isSelectable, andisExpandedstates. The verification ofaria-disabled, class applications, and callback prevention is thorough.packages/react-core/src/components/TreeView/TreeViewListItem.tsx (4)
49-52: LGTM! Props well-documented.The new
isDisabledandisToggleDisabledprops are properly typed and documented.
134-134: Disabled toggle class logic is correct.The
hasDisabledToggleClasscomputation correctly applies the disabled styling when:
isToggleDisabledis true, OR- The component is a button (default case) and
isDisabledis trueThis aligns with the expected behavior where the toggle inherits the disabled state in the default button case.
230-231:isFullyDisabledlogic is sound.The computed
isFullyDisabledcorrectly determines whenaria-disabledshould be applied to the treeitem:
- For button Component:
isDisabledalone makes it fully disabled- For label/div Component (hasCheckbox or isSelectable): requires both
isDisabledANDisToggleDisabledThis allows partial disabled states where selection is disabled but toggle remains functional.
248-268: Disabled state correctly applied to Component.The implementation properly:
- Applies
pf-m-disabledclass whenisDisabledis true- Gates
onSelectcallback with!isDisabledcheck- Gates expand/collapse behavior with
!isDisabledcheck- Sets
disabledattribute on button Component whenisDisabledis true
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Just one question below
| const renderToggle = (randomId: string) => ( | ||
| <ToggleComponent | ||
| className={css(styles.treeViewNodeToggle)} | ||
| className={css(styles.treeViewNodeToggle, hasDisabledToggleClass && 'pf-m-disabled')} |
There was a problem hiding this comment.
For instances we're using pf-m-disabled in the components and test file, is this class available on the styles.modifiers object?
There was a problem hiding this comment.
It will be with the core update.
|
@kmcfaul has this PR been updated with the changes to core yet? |
kmcfaul
force-pushed
the
treeview-disabled
branch
from
30a649e to
f3cafd1
Compare
|
cc @nicolethoen bumped core version |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-core/src/components/TreeView/TreeView.tsx`:
- Around line 165-166: The memo comparison for TreeViewListItem is missing
checks for isDisabled and isToggleDisabled, so updates to those booleans won’t
re-render when itemData reference is unchanged; update the custom comparison
function used in the memo wrapping of TreeViewListItem to also compare
prevProps.isDisabled !== nextProps.isDisabled and prevProps.isToggleDisabled !==
nextProps.isToggleDisabled (in addition to the existing checks against itemData
and other props) so the component re-renders when either flag changes.
🧹 Nitpick comments (3)
packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx (3)
419-427: Consider movinguserEvent.setup()insidebeforeEach.Calling
userEvent.setup()at the describe block level (line 420) creates a single user instance shared across all tests in this suite. While this often works, the recommended pattern is to call it insidebeforeEachor at the start of each test to ensure a fresh instance and avoid potential state leakage.Suggested refactor
describe('isDisabled prop', () => { - const user = userEvent.setup(); const onSelectMock = jest.fn(); const onExpandMock = jest.fn(); const onCollapseMock = jest.fn(); + let user: ReturnType<typeof userEvent.setup>; beforeEach(() => { + user = userEvent.setup(); jest.clearAllMocks(); });
529-537: SameuserEvent.setup()pattern issue.Consider the same refactor as suggested for the
isDisableddescribe block.
638-658: Add test verifyingonCheckis not called whenisDisabledis true.The test suite consistently verifies that
onSelect,onExpand, andonCollapseare not called whenisDisabled=true, but lacks a corresponding test for theonCheckcallback. For consistency and completeness, add a test case: "Does not call onCheck when isDisabled is true and hasCheckbox is true".
There was a problem hiding this comment.
Actionable comments posted: 1
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/TreeView/TreeViewListItem.tsx (1)
230-269: Disabled non‑selectable items can’t expand children
isDisabledboth disables the wrapper<button>and gates the expand/collapse path, so non‑selectable items with children become non‑expandable. That conflicts with the objective of allowing expansion while disabling selection. Consider lettingisToggleDisabledcontrol expand/collapse and only disabling the button when toggling is disabled (or there are no children).✅ Suggested fix
- const isFullyDisabled = - (Component === 'button' && isDisabled) || (Component !== 'button' && isDisabled && isToggleDisabled); + const isFullyDisabled = isDisabled && (!children || isToggleDisabled); ... - if (!isDisabled && !isSelectable && children && evt.isDefaultPrevented() !== true) { + if (!isToggleDisabled && !isSelectable && children && evt.isDefaultPrevented() !== true) { if (internalIsExpanded) { onCollapse && onCollapse(evt, itemData, parentItem); } else { onExpand && onExpand(evt, itemData, parentItem); } setIsExpanded(!internalIsExpanded); } ... - {...(Component === 'button' && { type: 'button', disabled: isDisabled })} + {...(Component === 'button' && { + type: 'button', + disabled: isDisabled && (!children || isToggleDisabled) + })}
🤖 Fix all issues with AI agents
In `@packages/react-core/src/components/TreeView/TreeViewListItem.tsx`:
- Around line 49-52: In TreeViewListItem, the checkbox input remains interactive
when the item is disabled; update the checkbox element to set its disabled
attribute to the combined value (isDisabled || checkProps.disabled) so it is
non-interactive when the row is disabled. Locate the checkbox rendering in the
TreeViewListItem component (where hasCheckbox is handled and checkProps is
spread onto the input) and replace/augment the input's disabled prop to use
disabled={isDisabled || checkProps.disabled} and ensure the onChange handler is
short-circuited/guarded accordingly.
| /** Flag indicating if the tree view item is disabled. */ | ||
| isDisabled?: boolean; | ||
| /** Flag indicating if the tree view item toggle is disabled. */ | ||
| isToggleDisabled?: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "TreeViewListItem.tsx" -path "*/react-core/*" | head -5Repository: patternfly/patternfly-react
Length of output: 137
🏁 Script executed:
fd "TreeViewListItem" --type fRepository: patternfly/patternfly-react
Length of output: 314
🏁 Script executed:
cat -n packages/react-core/src/components/TreeView/TreeViewListItem.tsx | head -200Repository: patternfly/patternfly-react
Length of output: 9070
🏁 Script executed:
cat -n packages/react-core/src/components/TreeView/TreeViewListItem.tsx | sed -n '200,250p'Repository: patternfly/patternfly-react
Length of output: 2174
🏁 Script executed:
cat -n packages/react-core/src/components/TreeView/TreeViewListItem.tsx | tail -100Repository: patternfly/patternfly-react
Length of output: 4652
🏁 Script executed:
cat -n packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx | grep -i "disabled\|isDisabled" -A 2 -B 2 | head -50Repository: patternfly/patternfly-react
Length of output: 2127
🏁 Script executed:
grep -n "isDisabled" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 2803
🏁 Script executed:
sed -n '502,511p' packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 430
🏁 Script executed:
sed -n '638,648p' packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 502
🏁 Script executed:
grep -n "hasCheckbox.*isDisabled\|isDisabled.*hasCheckbox" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx -A 10 | head -60Repository: patternfly/patternfly-react
Length of output: 1330
🏁 Script executed:
grep -n "hasCheckbox" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx | grep -i "check\|click\|oncheck"Repository: patternfly/patternfly-react
Length of output: 2982
🏁 Script executed:
sed -n '1,100p' packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx | grep -n "describe\|test"Repository: patternfly/patternfly-react
Length of output: 1306
🏁 Script executed:
grep -n "describe.*[Cc]heck\|test.*[Cc]heck" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 1877
🏁 Script executed:
sed -n '731,770p' packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 1590
🏁 Script executed:
grep -n "hasCheckbox.*isDisabled\|isDisabled.*hasCheckbox" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx -B 2 -A 15Repository: patternfly/patternfly-react
Length of output: 1647
🏁 Script executed:
grep -n "isDisabled.*hasCheckbox.*onCheck\|onCheck.*isDisabled.*hasCheckbox" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsx -B 5 -A 10Repository: patternfly/patternfly-react
Length of output: 53
🏁 Script executed:
sed -n '180,200p' packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 885
🏁 Script executed:
sed -n '160,180p' packages/react-core/src/components/TreeView/TreeViewListItem.tsxRepository: patternfly/patternfly-react
Length of output: 734
🏁 Script executed:
grep -n "onCheck.*isDisabled\|isDisabled.*onCheck" packages/react-core/src/components/TreeView/__tests__/TreeViewListItem.test.tsxRepository: patternfly/patternfly-react
Length of output: 53
Checkbox should be disabled when isDisabled is true
When hasCheckbox is enabled, the checkbox input element is not disabled when isDisabled=true. The onChange callback fires normally regardless of the disabled state. Apply disabled={isDisabled || checkProps.disabled} to the checkbox input to prevent interaction when the item is disabled, consistent with how onSelect, onExpand, and onCollapse are already guarded.
🤖 Prompt for AI Agents
In `@packages/react-core/src/components/TreeView/TreeViewListItem.tsx` around
lines 49 - 52, In TreeViewListItem, the checkbox input remains interactive when
the item is disabled; update the checkbox element to set its disabled attribute
to the combined value (isDisabled || checkProps.disabled) so it is
non-interactive when the row is disabled. Locate the checkbox rendering in the
TreeViewListItem component (where hasCheckbox is handled and checkProps is
spread onto the input) and replace/augment the input's disabled prop to use
disabled={isDisabled || checkProps.disabled} and ensure the onChange handler is
short-circuited/guarded accordingly.
4 participants
What: Closes #11893
Still needs some testing and definitely requires a styling update. Functionally the disabled props will work, but they will not look any different.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.