Skip to content

Commit

Permalink
fix(stickyTree): hidden item + focus loss could happen when the last …
Browse files Browse the repository at this point in the history
…sticky header is collapsed (#16341)

#### Pull request checklist

- [ ] Addresses an existing issue: Fixes #0000
- [ ] Include a change request file using `$ yarn change`

#### Description of changes

Fixing 2 focus related issues in sticky tree:

### issue 1: when navigate using keyboard, item can be hidden behind sticky header

- before the fix:
![image](http://g.recordit.co/EbMtKgXykK.gif)

- after:
![image](http://g.recordit.co/xynV3iSANR.gif)

### issue 2: focus lost when the entire list is scrolled to the top, and the last sticky header is clicked.
React profiler shows that, when the focus loss happens, the sticky header component is recreated, instead of updated.
This is because:
the sticky header is created by `createTreeItem`, which came from the prop of the 1st children in the virtual list. But the 1st children in the virtual list can be undefined, causing the rendered list to be null, and sticky header was unmounted.
Why the 1st children can be undefined: 
react-window uses startIndex/stopIndex to render visible items (children of list). When an item collapsed, the number of total items changes, causing the 'stopIndex' to be smaller than 'startIndex' during re-rendering, which result in the `children[0]` being undefined.
The fix is to pass a stable `createTreeItem` thru context, which always renders sticky headers, so they don't get unmounted. 

- before the fix, when the last sticky header was clicked, focus lost to the entire list:
![image](http://g.recordit.co/y6CnxUAeWR.gif)

- after:
![image](http://g.recordit.co/ApWkGtIxhm.gif)


#### Focus areas to test

(optional)
  • Loading branch information
YuanboXue-Amber authored Jan 4, 2021
1 parent 5902550 commit 62d455a
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 29 deletions.
1 change: 1 addition & 0 deletions packages/fluentui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Added `TvIcon`. @TanelVari ([#16207](https://github.com/microsoft/fluentui/pull/16207))

### Documentation
- Fix sticky tree's hidden item + focus loss when last sticky header collapsed @yuanboxue-amber ([#16341](https://github.com/microsoft/fluentui/pull/16341))
- Add `VirtualizedStickyTreePrototype` @yuanboxue-amber ([#16221](https://github.com/microsoft/fluentui/pull/16221))
- split prototypes into its own package `@fluentui/react-northstar-prototypes` @yuanboxue-amber ([#16183](https://github.com/microsoft/fluentui/pull/16183))
- `VirtualizdedTreePrototype`: change to use `useVirtualTree` hook and `react-window` @yuanboxue-amber ([#16080](https://github.com/microsoft/fluentui/pull/16080))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export interface InnerElementContextType {
stickyItemIds: string[];
stickyItemPusherHeights: number[];
stickyItemSize: number;
createTreeItem: (id: string, style: React.CSSProperties) => React.ReactElement<TreeItemProps> | null;
}

export interface VirtualItemData {
Expand Down Expand Up @@ -80,13 +81,54 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
getItemById,
registerItemRef,
activeItemIds,
focusItemById: baseFocusItemById,
toggleItemActive: baseToggleItemActive,
focusItemById,
expandSiblings,
listRef,
getItemRef,
} = useVirtualTree({ ...props, defaultActiveItemIds: stickyItemIds });

const getItemSize = React.useCallback(
(index: number) => {
const id = visibleItemIds[index];

const item = getItemById(id);
if (item?.level === 1) {
return stickyItemSize;
}
return itemSize;
},
[getItemById, itemSize, stickyItemSize, visibleItemIds],
);

const scrollToMakeItemVisible = React.useCallback(
itemOffset => {
const scrollOffset = Math.max(0, itemOffset - (height as number) / 2); // try to position item in the middle of container
listRef.current.scrollTo(scrollOffset);
},
[height, listRef],
);

const focusItemById = React.useCallback(
(id: string) => {
baseFocusItemById(id);

// item is not mounted yet, scroll to make item visible
if (getItemRef(id) == null) {
const getItemOffset = index => {
let result = 0;
for (let i = 0; i < index; ++i) {
result += getItemSize(i);
}
return result;
};
const focusIndex = visibleItemIds.indexOf(id);
scrollToMakeItemVisible(getItemOffset(focusIndex));
}
},
[baseFocusItemById, getItemRef, getItemSize, scrollToMakeItemVisible, visibleItemIds],
);

// get height of the pusher for each sticky item, based on number of their visible descendants
const stickyItemPusherHeights: number[] = React.useMemo(() => {
const result = new Array(stickyItemIds.length).fill(0);
Expand Down Expand Up @@ -135,7 +177,7 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
// When using keyboard, and navigate to non-sticky items, they could be hidden behind sticky headers.
// Scroll to make the focused non-sticky items always visible
const makeVisibleOnFocus = React.useCallback(
(id: string, level: number) => {
(id: string, level: number, style: React.CSSProperties) => {
if (level === 1) {
return; // focused sticky items are always visible, so no need to deal with them
}
Expand All @@ -155,12 +197,10 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
};

if (isOverlappingWithSticky(id)) {
// known issue:
// this scroll cannot guarantee non-sticky items' visibility. It would work when sticky titles are relatively small compare to non-sticky items.
listRef.current.scrollToItem(visibleItemIds.indexOf(id), 'center'); // scroll to item
scrollToMakeItemVisible(style.top || 0); // react-window provides style.top, fall back to 0 just in case
}
},
[getItemRef, listRef, stickyItemIds, visibleItemIds],
[getItemRef, scrollToMakeItemVisible, stickyItemIds],
);

// When using keyboard, and navigate to stickyItems, arrow up/down should navigate to previous item's last child/current Item's first child.
Expand Down Expand Up @@ -203,7 +243,7 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
index,
treeSize,
selectable: false,
onFocus: () => makeVisibleOnFocus(id, level),
onFocus: () => makeVisibleOnFocus(id, level, style),
...(level === 1 && expanded && childrenIds.length && { onKeyDown: handleArrowUpDownOnSticky(id, item) }),
},
});
Expand All @@ -217,25 +257,13 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
stickyItemIds,
stickyItemPusherHeights,
stickyItemSize,
createTreeItem,
}),
[getItemById, stickyItemIds, stickyItemPusherHeights, stickyItemSize],
[getItemById, stickyItemIds, stickyItemPusherHeights, stickyItemSize, createTreeItem],
);

const getItemKey = React.useCallback((index: number, data: VirtualItemData) => data.visibleItemIds[index], []);

const getItemSize = React.useCallback(
(index: number) => {
const id = visibleItemIds[index];

const item = getItemById(id);
if (item?.level === 1) {
return stickyItemSize;
}
return itemSize;
},
[getItemById, itemSize, stickyItemSize, visibleItemIds],
);

React.useLayoutEffect(() => {
listRef.current.resetAfterIndex(0);
}, [listRef, visibleItemIds]); // when item collapsed/expanded (visibleItemIds change), refresh react-window itemSize cache
Expand Down Expand Up @@ -272,7 +300,11 @@ export const VirtualStickyTree: ComponentWithAs<'div', VirtualStickyTreeProps> =
return element;
};

const getStickyItemStyle = (indexAmoungStickyItems: number, stickyItemNums: number, stickyItemSize: number) => ({
const getStickyItemStyle = (
indexAmoungStickyItems: number,
stickyItemNums: number,
stickyItemSize: number,
): React.CSSProperties => ({
height: stickyItemSize,
zIndex: teamsTheme.siteVariables.zIndexes.overlay,
position: 'sticky',
Expand All @@ -283,15 +315,10 @@ const getStickyItemStyle = (indexAmoungStickyItems: number, stickyItemNums: numb

const InnerElementType = ({ children, style }, ref) => {
const context = React.useContext(InnerElementContext);
const { stickyItemIds, stickyItemPusherHeights, stickyItemSize, getItemById } = context;
const { stickyItemIds, stickyItemPusherHeights, stickyItemSize, getItemById, createTreeItem } = context;

const renderContent = React.useCallback(
(virtualItems: React.ReactElement<ListChildComponentProps>[]) => {
const createTreeItem = virtualItems[0]?.props.data?.createTreeItem;
if (!createTreeItem) {
return null;
}

const result: Record<
string,
{
Expand Down Expand Up @@ -341,7 +368,7 @@ const InnerElementType = ({ children, style }, ref) => {

return flattenedResult;
},
[getItemById, stickyItemIds, stickyItemPusherHeights, stickyItemSize],
[createTreeItem, getItemById, stickyItemIds, stickyItemPusherHeights, stickyItemSize],
);

return (
Expand Down

0 comments on commit 62d455a

Please sign in to comment.