Skip to content
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

[WIP]: S2 treeview haschildnodes #7694

Draft
wants to merge 1 commit into
base: s2-treeview-api-update
Choose a base branch
from
Draft

[WIP]: S2 treeview haschildnodes #7694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/@react-aria/collections/src/Document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,18 @@ export class ElementNode<T> extends BaseNode<T> {
node.parentKey = this.parentNode instanceof ElementNode ? this.parentNode.node.key : null;
node.prevKey = this.previousSibling?.node.key ?? null;
node.nextKey = this.nextSibling?.node.key ?? null;
node.hasChildNodes = !!this.firstChild;

// Check if this node has any child nodes, but specifically any that are items.
Copy link
Member Author

@snowystinger snowystinger Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is this really a valid assumption? what about branch children? other types of items?

let child = this.firstChild;
let hasChildNodes = false;
while (!hasChildNodes && child && child?.nextSibling) {
child = child.nextSibling;
if (child.node.type === 'item') {
hasChildNodes = true;
}
}

node.hasChildNodes = hasChildNodes;
node.firstChildKey = this.firstChild?.node.key ?? null;
node.lastChildKey = this.lastChild?.node.key ?? null;
}
Expand Down
10 changes: 4 additions & 6 deletions packages/@react-aria/gridlist/src/useGridListItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ export interface AriaGridListItemOptions {
/** Whether the list row is contained in a virtual scroller. */
isVirtualized?: boolean,
/** Whether selection should occur on press up instead of press down. */
shouldSelectOnPressUp?: boolean
shouldSelectOnPressUp?: boolean,
/** Whether this item has children, even if not loaded yet. */
hasChildItems?: boolean
}

export interface GridListItemAria extends SelectableItemStates {
Expand Down Expand Up @@ -86,13 +88,9 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
};

let treeGridRowProps: HTMLAttributes<HTMLElement> = {};
let hasChildRows;
let hasChildRows = props.hasChildItems || node.hasChildNodes;
let hasLink = state.selectionManager.isLink(node.key);
if (node != null && 'expandedKeys' in state) {
// TODO: ideally node.hasChildNodes would be a way to tell if a row has child nodes, but the row's contents make it so that value is always
// true...
let children = state.collection.getChildren?.(node.key);
hasChildRows = [...(children ?? [])].length > 1;
if (onAction == null && !hasLink && state.selectionManager.selectionMode === 'none' && hasChildRows) {
onAction = () => state.toggleKey(node.key);
}
Expand Down
23 changes: 8 additions & 15 deletions packages/@react-spectrum/s2/src/TreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,24 +294,19 @@ const treeRowFocusIndicator = raw(`
}`
);

let TreeItemContext = createContext<{hasChildItems?: boolean}>({});

export const TreeViewItem = <T extends object>(props: TreeViewItemProps<T>) => {
let {
href
} = props;
let {isDetached, isEmphasized} = useContext(InternalTreeContext);

return (
// TODO right now all the tree rows have the various data attributes applied on their dom nodes, should they? Doesn't feel very useful
<TreeItemContext.Provider value={{hasChildItems: !!props.childItems}}>
<UNSTABLE_TreeItem
{...props}
className={(renderProps) => treeRow({
...renderProps,
isLink: !!href, isEmphasized
}) + (renderProps.isFocusVisible && !isDetached ? ' ' + treeRowFocusIndicator : '')} />
</TreeItemContext.Provider>
<UNSTABLE_TreeItem
{...props}
className={(renderProps) => treeRow({
...renderProps,
isLink: !!href, isEmphasized
}) + (renderProps.isFocusVisible && !isDetached ? ' ' + treeRowFocusIndicator : '')} />
);
};

Expand All @@ -320,7 +315,6 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
children
} = props;
let {isDetached, isEmphasized} = useContext(InternalTreeContext);
let {hasChildItems} = useContext(TreeItemContext);

return (
<UNSTABLE_TreeItemContent>
Expand Down Expand Up @@ -348,7 +342,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
width: '[calc(calc(var(--tree-item-level, 0) - 1) * var(--indent))]'
})} />
{/* TODO: revisit when we do async loading, at the moment hasChildItems will only cause the chevron to be rendered, no aria/data attributes indicating the row's expandability are added */}
{(hasChildRows || hasChildItems) && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
{hasChildRows && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
<Provider
values={[
[TextContext, {styles: treeContent}],
Expand All @@ -357,8 +351,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
styles: style({size: fontRelative(20), flexShrink: 0})
}],
[ActionButtonGroupContext, {styles: treeActions}],
[ActionMenuContext, {styles: treeActionMenu, isQuiet: true, 'aria-label': 'Actions'}],
[TreeItemContext, {}]
[ActionMenuContext, {styles: treeActionMenu, isQuiet: true, 'aria-label': 'Actions'}]
]}>
{children}
</Provider>
Expand Down
28 changes: 9 additions & 19 deletions packages/@react-spectrum/tree/src/TreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import ChevronLeftMedium from '@spectrum-icons/ui/ChevronLeftMedium';
import ChevronRightMedium from '@spectrum-icons/ui/ChevronRightMedium';
import {DOMRef, Expandable, Key, SelectionBehavior, SpectrumSelectionProps, StyleProps} from '@react-types/shared';
import {isAndroid} from '@react-aria/utils';
import React, {createContext, JSX, JSXElementConstructor, ReactElement, ReactNode, useContext, useRef} from 'react';
import React, {createContext, JSX, JSXElementConstructor, ReactElement, ReactNode, useRef} from 'react';
import {SlotProvider, useDOMRef, useStyleProps} from '@react-spectrum/utils';
import {style} from '@react-spectrum/style-macro-s1' with {type: 'macro'};
import {useButton} from '@react-aria/button';
Expand All @@ -40,8 +40,6 @@ export interface SpectrumTreeViewProps<T> extends Omit<AriaTreeGridListProps<T>,
export interface SpectrumTreeViewItemProps<T extends object = object> extends Omit<TreeItemProps, 'className' | 'style' | 'value' | 'onHoverStart' | 'onHoverEnd' | 'onHoverChange'> {
/** Rendered contents of the tree item or child items. */
children: ReactNode,
/** Whether this item has children, even if not loaded yet. */
hasChildItems?: boolean,
/** A list of child tree item objects used when dynamically rendering the tree item children. */
childItems?: Iterable<T>
}
Expand Down Expand Up @@ -219,23 +217,18 @@ const treeRowOutline = style({
}
});

let TreeItemContext = createContext<{hasChildItems?: boolean}>({});

export const TreeViewItem = <T extends object>(props: SpectrumTreeViewItemProps<T>) => {
let {
href
} = props;

return (
// TODO right now all the tree rows have the various data attributes applied on their dom nodes, should they? Doesn't feel very useful
<TreeItemContext.Provider value={{hasChildItems: !!props.childItems || props.hasChildItems}}>
<UNSTABLE_TreeItem
{...props}
className={renderProps => treeRow({
...renderProps,
isLink: !!href
})} />
</TreeItemContext.Provider>
<UNSTABLE_TreeItem
{...props}
className={renderProps => treeRow({
...renderProps,
isLink: !!href
})} />
);
};

Expand All @@ -244,7 +237,6 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
let {
children
} = props;
let {hasChildItems} = useContext(TreeItemContext);

return (
<UNSTABLE_TreeItemContent>
Expand All @@ -260,7 +252,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
)}
<div style={{gridArea: 'level-padding', marginInlineEnd: `calc(${level - 1} * var(--spectrum-global-dimension-size-200))`}} />
{/* TODO: revisit when we do async loading, at the moment hasChildItems will only cause the chevron to be rendered, no aria/data attributes indicating the row's expandability are added */}
{(hasChildRows || hasChildItems) && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
{hasChildRows && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
<SlotProvider
slots={{
text: {UNSAFE_className: treeContent({isDisabled})},
Expand All @@ -278,9 +270,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
},
actionMenu: {UNSAFE_className: treeActionMenu(), UNSAFE_style: {marginInlineEnd: '.5rem'}, isQuiet: true}
}}>
<TreeItemContext.Provider value={{}}>
{children}
</TreeItemContext.Provider>
{children}
</SlotProvider>
<div className={treeRowOutline({isFocusVisible, isSelected})} />
</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/tree/test/TreeView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-label', 'Test');
// Until the row gets children, don't mark it with the aria/data attributes.
expect(rows[0]).not.toHaveAttribute('aria-expanded');
expect(rows[0]).not.toHaveAttribute('data-has-child-rows');
expect(rows[0]).toHaveAttribute('data-has-child-rows');
expect(chevron).toBeTruthy();
});

Expand Down
5 changes: 3 additions & 2 deletions packages/@react-stately/grid/src/useGridState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,10 @@ export function useGridState<T extends object, C extends GridCollection<T>>(prop
}
}
if (newRow) {
const childNodes = newRow.hasChildNodes ? [...getChildNodes(newRow, collection)] : [];
let hasChildNodes = newRow.firstChildKey === undefined ? newRow.hasChildNodes : newRow.firstChildKey != null;
const childNodes = hasChildNodes ? [...getChildNodes(newRow, collection)] : [];
const keyToFocus =
newRow.hasChildNodes &&
hasChildNodes &&
parentNode !== node &&
node &&
node.index < childNodes.length ?
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-stately/selection/src/SelectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ export class SelectionManager implements MultipleSelectionManager {
}

// Add child keys. If cell selection is allowed, then include item children too.
if (item?.hasChildNodes && (this.allowsCellSelection || item.type !== 'item')) {
if (item && (item?.firstChildKey === undefined ? item?.hasChildNodes : item?.firstChildKey != null) && (this.allowsCellSelection || item.type !== 'item')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

have to account for old collections which don't have firstChildKey on their nodes

addKeys(getFirstItem(getChildNodes(item, this.collection))?.key ?? null);
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/@react-types/shared/src/collections.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ export interface Node<T> {
prevKey?: Key | null,
/** The key of the node after this node. */
nextKey?: Key | null,
/** The key of the first child node. */
firstChildKey?: Key | null,
Copy link
Member Author

Choose a reason for hiding this comment

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

ok to expose these? doesn't match old collections

/** The key of the last child node. */
lastChildKey?: Key | null,
/** Additional properties specific to a particular node type. */
props?: any,
/** @private */
Expand Down
2 changes: 1 addition & 1 deletion packages/react-aria-components/src/Table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class TableCollection<T> extends BaseCollection<T> implements ITableCollection<T
switch (node.type) {
case 'column':
columnKeyMap.set(node.key, node);
if (!node.hasChildNodes) {
if (node.firstChildKey == null) {
Copy link
Member Author

@snowystinger snowystinger Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this is technically what the old check was, is this still actually what we want to check, it's a bit of a strange check
but things break if I update it to the new hasChildNodes

node.index = this.columns.length;
this.columns.push(node);

Expand Down
6 changes: 3 additions & 3 deletions packages/react-aria-components/src/Tree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/

import {AriaTreeGridListProps, useTreeGridList, useTreeGridListItem} from '@react-aria/tree';
import {AriaTreeGridListItemOptions, AriaTreeGridListProps, useTreeGridList, useTreeGridListItem} from '@react-aria/tree';
import {ButtonContext} from './Button';
import {CheckboxContext} from './RSPContexts';
import {Collection, CollectionBuilder, CollectionNode, createBranchComponent, createLeafComponent, useCachedChildren} from '@react-aria/collections';
Expand Down Expand Up @@ -302,7 +302,7 @@ export const UNSTABLE_TreeItemContent = /*#__PURE__*/ createLeafComponent('conte

export const TreeItemContentContext = createContext<TreeItemContentRenderProps | null>(null);

export interface TreeItemProps<T = object> extends StyleRenderProps<TreeItemRenderProps>, LinkDOMProps, HoverEvents {
export interface TreeItemProps<T = object> extends StyleRenderProps<TreeItemRenderProps>, LinkDOMProps, HoverEvents, Pick<AriaTreeGridListItemOptions, 'hasChildItems'> {
/** The unique id of the tree row. */
id?: Key,
/** The object value that this tree item represents. When using dynamic collections, this is set automatically. */
Expand All @@ -325,7 +325,7 @@ export const UNSTABLE_TreeItem = /*#__PURE__*/ createBranchComponent('item', <T
// eslint-disable-next-line @typescript-eslint/no-unused-vars
let {rowProps, gridCellProps, expandButtonProps, descriptionProps, ...states} = useTreeGridListItem({node: item}, state, ref);
let isExpanded = rowProps['aria-expanded'] === true;
let hasChildRows = [...state.collection.getChildren!(item.key)]?.length > 1;
let hasChildRows = props.hasChildItems || item.hasChildNodes;
let level = rowProps['aria-level'] || 1;

let {hoverProps, isHovered} = useHover({
Expand Down
6 changes: 2 additions & 4 deletions packages/react-aria-components/test/Table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,10 +668,8 @@ describe('Table', () => {

let tableTester = testUtilUser.createTester('Table', {root: getByRole('grid')});
await user.tab();
fireEvent.keyDown(document.activeElement, {key: 'ArrowDown'});
fireEvent.keyUp(document.activeElement, {key: 'ArrowDown'});
fireEvent.keyDown(document.activeElement, {key: 'ArrowRight'});
fireEvent.keyUp(document.activeElement, {key: 'ArrowRight'});
await user.keyboard('{ArrowDown}');
await user.keyboard('{ArrowRight}');

let gridRows = tableTester.rows;
expect(gridRows).toHaveLength(4);
Expand Down