From 9cdf4663b80ca7608b2abe3e0efc9279bb930f53 Mon Sep 17 00:00:00 2001 From: Rohan <45748283+r100-stack@users.noreply.github.com> Date: Fri, 8 Nov 2024 18:03:33 -0500 Subject: [PATCH] New and improved `useOverflow` algorithm (#2154) --- .changeset/sour-apes-visit.md | 5 + .../src/core/Breadcrumbs/Breadcrumbs.test.tsx | 64 +--- .../src/core/Breadcrumbs/Breadcrumbs.tsx | 4 +- .../src/core/ButtonGroup/ButtonGroup.test.tsx | 141 +------- .../src/core/Select/SelectTagContainer.tsx | 4 +- .../src/core/Table/Table.test.tsx | 36 -- .../src/core/Table/TablePaginator.test.tsx | 67 ---- .../components/MiddleTextTruncation.test.tsx | 83 ----- .../utils/components/OverflowContainer.tsx | 340 +++++++++++++++++- .../itwinui-react/src/utils/hooks/index.ts | 1 - .../src/utils/hooks/useOverflow.test.tsx | 216 ----------- .../src/utils/hooks/useOverflow.tsx | 109 ------ testing/e2e/app/routes/ButtonGroup/spec.ts | 4 + testing/e2e/app/routes/ComboBox/route.tsx | 61 +++- testing/e2e/app/routes/ComboBox/spec.ts | 23 ++ 15 files changed, 433 insertions(+), 725 deletions(-) create mode 100644 .changeset/sour-apes-visit.md delete mode 100644 packages/itwinui-react/src/utils/components/MiddleTextTruncation.test.tsx delete mode 100644 packages/itwinui-react/src/utils/hooks/useOverflow.test.tsx delete mode 100644 packages/itwinui-react/src/utils/hooks/useOverflow.tsx diff --git a/.changeset/sour-apes-visit.md b/.changeset/sour-apes-visit.md new file mode 100644 index 00000000000..d036a463958 --- /dev/null +++ b/.changeset/sour-apes-visit.md @@ -0,0 +1,5 @@ +--- +'@itwin/itwinui-react': patch +--- + +Fixed bugs and improved performance in components with overflow support (e.g. `ComboBox`, `Select`, `ButtonGroup`, `Breadcrumbs`, etc.) diff --git a/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.test.tsx b/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.test.tsx index eacaed76d28..c322feaa9f2 100644 --- a/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.test.tsx +++ b/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.test.tsx @@ -3,12 +3,10 @@ * See LICENSE.md in the project root for license terms and full copyright notice. *--------------------------------------------------------------------------------------------*/ import * as React from 'react'; -import { fireEvent, render, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import { Breadcrumbs } from './Breadcrumbs.js'; -import { SvgChevronRight, SvgMore } from '../../utils/index.js'; -import * as UseOverflow from '../../utils/hooks/useOverflow.js'; -import { IconButton } from '../Buttons/IconButton.js'; +import { SvgChevronRight } from '../../utils/index.js'; import { Button } from '../Buttons/Button.js'; import { userEvent } from '@testing-library/user-event'; @@ -56,13 +54,6 @@ const assertBaseElement = ( }); }; -const useOverflowMock = vi - .spyOn(UseOverflow, 'useOverflow') - .mockImplementation((itemsCount) => [vi.fn(), itemsCount]); -beforeEach(() => { - useOverflowMock.mockImplementation((itemsCount) => [vi.fn(), itemsCount]); -}); - it('should render all elements in default state', () => { const { container } = renderComponent(); assertBaseElement(container); @@ -181,57 +172,6 @@ it('should accept currentIndex prop', () => { assertBaseElement(container, { currentIndex: 1 }); }); -it('should overflow when there is not enough space', () => { - useOverflowMock.mockReturnValue([vi.fn(), 2]); - const { container } = renderComponent(); - - expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy(); - expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy(); - - const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item'); - expect(breadcrumbs.length).toEqual(3); - expect(breadcrumbs[0].textContent).toEqual('Item 0'); - expect(breadcrumbs[1].textContent).toEqual('…'); - expect(breadcrumbs[1].firstElementChild?.textContent).toContain('…'); - expect(breadcrumbs[2].textContent).toEqual('Item 2'); -}); - -it('should handle overflow when overflowButton is specified', () => { - const onClick = vi.fn(); - useOverflowMock.mockReturnValue([vi.fn(), 2]); - const { container } = renderComponent({ - overflowButton: (visibleCount) => ( - - - - ), - }); - - expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy(); - expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy(); - - const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item'); - expect(breadcrumbs.length).toEqual(3); - fireEvent.click(breadcrumbs[1]); - expect(onClick).toHaveBeenCalledTimes(1); - expect(onClick).toHaveBeenCalledWith(2); -}); - -it('should show the last item when only one can be visible', () => { - useOverflowMock.mockReturnValue([vi.fn(), 1]); - - const { container } = renderComponent(); - - expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy(); - expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy(); - - const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item'); - expect(breadcrumbs.length).toEqual(2); - expect(breadcrumbs[0].textContent).toEqual('…'); - expect(breadcrumbs[0].firstElementChild?.textContent).toContain('…'); - expect(breadcrumbs[1].textContent).toEqual('Item 2'); -}); - it('should support legacy api', async () => { const onClick = vi.fn(); diff --git a/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx b/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx index 1c09a82d759..caa4ae52287 100644 --- a/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx +++ b/packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx @@ -111,7 +111,7 @@ type BreadcrumbsProps = { * Current level * */ -const BreadcrumbsComponent = React.forwardRef((props, ref) => { +const BreadcrumbsComponent = React.forwardRef((props, forwardedRef) => { const { children: childrenProp, currentIndex = React.Children.count(childrenProp) - 1, @@ -130,7 +130,7 @@ const BreadcrumbsComponent = React.forwardRef((props, ref) => { diff --git a/packages/itwinui-react/src/core/ButtonGroup/ButtonGroup.test.tsx b/packages/itwinui-react/src/core/ButtonGroup/ButtonGroup.test.tsx index 44022720fb8..5f56f929178 100644 --- a/packages/itwinui-react/src/core/ButtonGroup/ButtonGroup.test.tsx +++ b/packages/itwinui-react/src/core/ButtonGroup/ButtonGroup.test.tsx @@ -2,13 +2,10 @@ * Copyright (c) Bentley Systems, Incorporated. All rights reserved. * See LICENSE.md in the project root for license terms and full copyright notice. *--------------------------------------------------------------------------------------------*/ -import { SvgMore, SvgClose as SvgPlaceholder } from '../../utils/index.js'; -import { fireEvent, render } from '@testing-library/react'; -import * as UseOverflow from '../../utils/hooks/useOverflow.js'; +import { render } from '@testing-library/react'; import { ButtonGroup } from './ButtonGroup.js'; import { Button } from '../Buttons/Button.js'; -import { IconButton } from '../Buttons/IconButton.js'; it('should render with two buttons', () => { const { container } = render( @@ -34,101 +31,6 @@ it('should render with four buttons', () => { expect(container.querySelectorAll('button').length).toBe(4); }); -it.each(['start', 'end'] as const)( - 'should handle overflow when overflowButton is specified (%s)', - (overflowPlacement) => { - const scrollWidthSpy = vi - .spyOn(HTMLElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(250) - .mockReturnValue(200); - const offsetWidthSpy = vi - .spyOn(HTMLElement.prototype, 'offsetWidth', 'get') - .mockReturnValue(200); - - const mockOnClick = vi.fn(); - - const OverflowGroup = () => { - const buttons = [...Array(3)].map((_, index) => ( - - - - )); - return ( - ( - mockOnClick(overflowStart)}> - - - )} - overflowPlacement={overflowPlacement} - > - {buttons} - - ); - }; - const { container } = render(); - - expect(container.querySelector('.iui-button-group')).toBeTruthy(); - - const buttons = container.querySelectorAll('.iui-button'); - expect(buttons).toHaveLength(2); - fireEvent.click(overflowPlacement === 'end' ? buttons[1] : buttons[0]); - expect(mockOnClick).toHaveBeenCalledWith(1); - - scrollWidthSpy.mockRestore(); - offsetWidthSpy.mockRestore(); - }, -); - -it.each(['start', 'end'] as const)( - 'should handle overflow when available space is smaller than one element (%s)', - (overflowPlacement) => { - const scrollWidthSpy = vi - .spyOn(HTMLElement.prototype, 'scrollWidth', 'get') - .mockReturnValue(200); - const offsetWidthSpy = vi - .spyOn(HTMLElement.prototype, 'offsetWidth', 'get') - .mockReturnValue(50); - - const OverflowGroup = () => { - const buttons = [...Array(3)].map((_, index) => ( - - - - )); - return ( - ( - - - - )} - overflowPlacement={overflowPlacement} - > - {buttons} - - ); - }; - const { container } = render(); - const { - container: { firstChild: moreIconButton }, - } = render( - - - , - ); - - expect(container.querySelector('.iui-button-group')).toBeTruthy(); - - const buttons = container.querySelectorAll('.iui-button'); - expect(buttons).toHaveLength(1); - expect(buttons[0]).toEqual(moreIconButton); - - scrollWidthSpy.mockRestore(); - offsetWidthSpy.mockRestore(); - }, -); - it('should work in vertical orientation', () => { const { container } = render( @@ -142,44 +44,3 @@ it('should work in vertical orientation', () => { expect(group).toBeTruthy(); expect(group.children).toHaveLength(2); }); - -it.each` - visibleCount | overflowStart | length | overflowPlacement - ${9} | ${1} | ${10} | ${'start'} - ${8} | ${2} | ${10} | ${'start'} - ${4} | ${6} | ${10} | ${'start'} - ${3} | ${7} | ${10} | ${'start'} - ${1} | ${9} | ${10} | ${'start'} - ${9} | ${8} | ${10} | ${'end'} - ${8} | ${7} | ${10} | ${'end'} - ${4} | ${3} | ${10} | ${'end'} - ${3} | ${2} | ${10} | ${'end'} - ${1} | ${0} | ${10} | ${'end'} -`( - 'should calculate correct values when overflowPlacement=$overflowPlacement and visibleCount=$visibleCount', - ({ visibleCount, overflowStart, length, overflowPlacement }) => { - const useOverflowMock = vi - .spyOn(UseOverflow, 'useOverflow') - .mockReturnValue([vi.fn(), visibleCount]); - - const buttons = [...Array(length)].map((_, index) => ( - - )); - - const { container } = render( - {startIndex}} - overflowPlacement={overflowPlacement} - > - {buttons} - , - ); - - expect(container.querySelectorAll('button')).toHaveLength(visibleCount - 1); - expect(container.querySelector('span')).toHaveTextContent( - `${overflowStart}`, - ); - - useOverflowMock.mockRestore(); - }, -); diff --git a/packages/itwinui-react/src/core/Select/SelectTagContainer.tsx b/packages/itwinui-react/src/core/Select/SelectTagContainer.tsx index 8311ab26465..a9292363c98 100644 --- a/packages/itwinui-react/src/core/Select/SelectTagContainer.tsx +++ b/packages/itwinui-react/src/core/Select/SelectTagContainer.tsx @@ -17,7 +17,7 @@ type SelectTagContainerProps = { /** */ -export const SelectTagContainer = React.forwardRef((props, ref) => { +export const SelectTagContainer = React.forwardRef((props, forwardedRef) => { const { tags: tagsProp, className, ...rest } = props; const tags = React.useMemo( @@ -29,7 +29,7 @@ export const SelectTagContainer = React.forwardRef((props, ref) => { diff --git a/packages/itwinui-react/src/core/Table/Table.test.tsx b/packages/itwinui-react/src/core/Table/Table.test.tsx index 3f7467632c0..482f111373c 100644 --- a/packages/itwinui-react/src/core/Table/Table.test.tsx +++ b/packages/itwinui-react/src/core/Table/Table.test.tsx @@ -30,7 +30,6 @@ import { } from '../../utils/index.js'; import { DefaultCell, EditableCell } from './cells/index.js'; import { TablePaginator } from './TablePaginator.js'; -import * as UseOverflow from '../../utils/hooks/useOverflow.js'; import { userEvent } from '@testing-library/user-event'; import { ActionColumn, @@ -2587,41 +2586,6 @@ it('should handle unwanted actions on editable cell', async () => { expect(onSelect).not.toHaveBeenCalled(); }); -it('should render data in pages', async () => { - vi.spyOn(UseOverflow, 'useOverflow').mockImplementation((itemsCount) => [ - vi.fn(), - itemsCount, - ]); - const { container } = renderComponent({ - data: mockedData(100), - pageSize: 10, - paginatorRenderer: (props) => , - }); - - let rows = container.querySelectorAll('.iui-table-body .iui-table-row'); - expect(rows).toHaveLength(10); - expect(rows[0].querySelector('.iui-table-cell')?.textContent).toEqual( - 'Name1', - ); - expect(rows[9].querySelector('.iui-table-cell')?.textContent).toEqual( - 'Name10', - ); - - const pages = container.querySelectorAll( - '.iui-table-paginator .iui-table-paginator-page-button', - ); - expect(pages).toHaveLength(10); - await userEvent.click(pages[3]); - rows = container.querySelectorAll('.iui-table-body .iui-table-row'); - expect(rows).toHaveLength(10); - expect(rows[0].querySelector('.iui-table-cell')?.textContent).toEqual( - 'Name31', - ); - expect(rows[9].querySelector('.iui-table-cell')?.textContent).toEqual( - 'Name40', - ); -}); - it('should change page size', async () => { const { container } = renderComponent({ data: mockedData(100), diff --git a/packages/itwinui-react/src/core/Table/TablePaginator.test.tsx b/packages/itwinui-react/src/core/Table/TablePaginator.test.tsx index 6291fdb1ecf..7eb5f2f96e3 100644 --- a/packages/itwinui-react/src/core/Table/TablePaginator.test.tsx +++ b/packages/itwinui-react/src/core/Table/TablePaginator.test.tsx @@ -4,7 +4,6 @@ *--------------------------------------------------------------------------------------------*/ import { fireEvent, render, screen } from '@testing-library/react'; import { TablePaginator, type TablePaginatorProps } from './TablePaginator.js'; -import * as UseOverflow from '../../utils/hooks/useOverflow.js'; import * as UseContainerWidth from '../../utils/hooks/useContainerWidth.js'; import { userEvent } from '@testing-library/user-event'; @@ -21,13 +20,6 @@ const renderComponent = (props?: Partial) => { ); }; -beforeEach(() => { - vi.spyOn(UseOverflow, 'useOverflow').mockImplementation((itemsCount) => [ - vi.fn(), - itemsCount, - ]); -}); - afterEach(() => { vi.restoreAllMocks(); }); @@ -190,38 +182,6 @@ it('should handle clicks', async () => { expect(onPageChange).toHaveBeenCalledWith(6); }); -it('should render truncated pages list', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 5]); - const { container } = renderComponent({ currentPage: 10 }); - - const pages = container.querySelectorAll('.iui-table-paginator-page-button'); - expect(pages).toHaveLength(7); - expect(pages[0].textContent).toEqual('1'); - expect(pages[1].textContent).toEqual('9'); - expect(pages[2].textContent).toEqual('10'); - expect(pages[3].textContent).toEqual('11'); - expect(pages[3]).toHaveAttribute('data-iui-active', 'true'); - expect(pages[4].textContent).toEqual('12'); - expect(pages[5].textContent).toEqual('13'); - expect(pages[6].textContent).toEqual('20'); - - const ellipsis = container.querySelectorAll('.iui-table-paginator-ellipsis'); - expect(ellipsis).toHaveLength(2); -}); - -it('should render only the current page when screen is very small', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 1]); - const { container } = renderComponent({ currentPage: 10 }); - - const pages = container.querySelectorAll('.iui-table-paginator-page-button'); - expect(pages).toHaveLength(1); - expect(pages[0].textContent).toEqual('11'); - expect(pages[0]).toHaveAttribute('data-iui-active', 'true'); - - const ellipsis = container.querySelectorAll('.iui-table-paginator-ellipsis'); - expect(ellipsis).toHaveLength(0); -}); - it('should handle keyboard navigation when focusActivationMode is auto', () => { const onPageChange = vi.fn(); const { container } = renderComponent({ @@ -272,33 +232,6 @@ it('should handle keyboard navigation when focusActivationMode is manual', () => expect(onPageChange).toHaveBeenCalledWith(0); }); -it('should render elements in small size', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 5]); - const { container } = renderComponent({ - size: 'small', - pageSizeList: [10, 25, 50], - currentPage: 10, - onPageSizeChange: vi.fn(), - }); - - const pageSwitchers = container.querySelectorAll('.iui-button'); - expect( - Array.from(pageSwitchers).every( - (p) => p.getAttribute('data-iui-size') === 'small', - ), - ).toBe(true); - - const pages = container.querySelectorAll( - '.iui-table-paginator-page-button[data-iui-size="small"]', - ); - expect(pages).toHaveLength(7); - - const ellipsis = container.querySelectorAll( - '.iui-table-paginator-ellipsis-small', - ); - expect(ellipsis).toHaveLength(2); -}); - it('should render with custom localization', async () => { vi.spyOn(UseContainerWidth, 'useContainerWidth').mockImplementation(() => [ vi.fn(), diff --git a/packages/itwinui-react/src/utils/components/MiddleTextTruncation.test.tsx b/packages/itwinui-react/src/utils/components/MiddleTextTruncation.test.tsx deleted file mode 100644 index 4393d666e8a..00000000000 --- a/packages/itwinui-react/src/utils/components/MiddleTextTruncation.test.tsx +++ /dev/null @@ -1,83 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Bentley Systems, Incorporated. All rights reserved. - * See LICENSE.md in the project root for license terms and full copyright notice. - *--------------------------------------------------------------------------------------------*/ -import { render } from '@testing-library/react'; -import { MiddleTextTruncation } from './MiddleTextTruncation.js'; -import * as UseOverflow from '../hooks/useOverflow.js'; - -it('should truncate the text', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 20]); - const { container } = render( -
- -
, - ); - - const containerSpan = container.querySelector('span') as HTMLSpanElement; - expect(containerSpan.textContent).toBe('This is some …lipsis'); -}); - -it('should truncate the text and leave 20 symbols at the end', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 50]); - const { container } = render( -
- -
, - ); - - const containerSpan = container.querySelector('span') as HTMLSpanElement; - expect(containerSpan.textContent).toBe( - 'This is some very long text t…f the truncated text', - ); -}); - -it('should leave original text (same length)', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 20]); - const { container } = render( -
- -
, - ); - - const containerSpan = container.querySelector('span') as HTMLSpanElement; - expect(containerSpan.textContent).toBe('This is a short text'); -}); - -it('should leave original text', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 20]); - const { container } = render( -
- -
, - ); - - const containerSpan = container.querySelector('span') as HTMLSpanElement; - expect(containerSpan.textContent).toBe('No trunc'); -}); - -it('should render custom text', () => { - vi.spyOn(UseOverflow, 'useOverflow').mockReturnValue([vi.fn(), 20]); - const text = 'This is some very long text to truncate and expect ellipsis'; - const { container } = render( - ( - - {truncatedText} - some additional text - - )} - />, - ); - - const containerSpan = container.querySelector('span') as HTMLSpanElement; - expect(containerSpan.textContent).toBe( - 'This is some …lipsis - some additional text', - ); - expect( - containerSpan.querySelector('[data-testid="custom-text"]'), - ).toBeTruthy(); -}); diff --git a/packages/itwinui-react/src/utils/components/OverflowContainer.tsx b/packages/itwinui-react/src/utils/components/OverflowContainer.tsx index 548b586195b..1efb25a4888 100644 --- a/packages/itwinui-react/src/utils/components/OverflowContainer.tsx +++ b/packages/itwinui-react/src/utils/components/OverflowContainer.tsx @@ -6,8 +6,10 @@ import React from 'react'; import { useMergedRefs } from '../hooks/useMergedRefs.js'; import type { PolymorphicForwardRefComponent } from '../props.js'; import { Box } from './Box.js'; -import { useOverflow } from '../hooks/useOverflow.js'; +import { useLayoutEffect } from '../hooks/useIsomorphicLayoutEffect.js'; import { useSafeContext } from '../hooks/useSafeContext.js'; +import { isUnitTest } from '../functions/dev.js'; +import { useResizeObserver } from '../hooks/useResizeObserver.js'; type OverflowContainerProps = { /** @@ -21,12 +23,43 @@ type OverflowContainerProps = { itemsCount: number; }; -const OverflowContainerComponent = React.forwardRef((props, ref) => { +/** + * Useful to handle overflow of items in a container. + * Also listens to resize changes along the overflowOrientation. + * + * API: + * - Use `OverflowContainer.useContext()` to get overflow related properties. + * - Wrap overflow content in `OverflowContainer.OverflowNode` to conditionally render it when overflowing. + * + * @example + * const items = Array(10) + * .fill(null) + * .map((_, i) => Item {i}); + * + * return ( + * + * + * + * ); + * + * const MyOverflowContainerContent = () => { + * const { visibleCount, itemsCount } = OverflowContainer.useContext(); + * + * return ( + * <> + * {items.slice(0, visibleCount)} + * + * +{itemsCount - visibleCount} more... + * + * + * ); + * }; + */ +const OverflowContainerMain = React.forwardRef((props, forwardedRef) => { const { itemsCount, children, overflowOrientation, ...rest } = props; const [containerRef, visibleCount] = useOverflow( itemsCount, - false, overflowOrientation, ); @@ -37,12 +70,17 @@ const OverflowContainerComponent = React.forwardRef((props, ref) => { return ( - + {children} ); -}) as PolymorphicForwardRefComponent<'div', OverflowContainerProps>; +}) as PolymorphicForwardRefComponent< + 'div', + OverflowContainerProps & { + overflowOrientation: Required['overflowOrientation']; + } +>; // ---------------------------------------------------------------------------- @@ -66,7 +104,33 @@ const OverflowContainerOverflowNode = ( // ---------------------------------------------------------------------------- +const OverflowContainerComponent = React.forwardRef((props, forwardedRef) => { + const { itemsCount, overflowOrientation = 'horizontal', ...rest } = props; + + const [size, setSize] = React.useState(null); + const [resizeRef] = useResizeObserver(setSize); + + const ref = useMergedRefs(resizeRef, forwardedRef); + + const key = + `${itemsCount}` + + `${overflowOrientation === 'vertical' ? size?.height : size?.width}`; + + return ( + + ); +}) as PolymorphicForwardRefComponent<'div', OverflowContainerProps>; + +// ---------------------------------------------------------------------------- + /** + * @private * Wrapper over `useOverflow`. * * - Use `OverflowContainer.useContext()` to get overflow related properties. @@ -96,6 +160,272 @@ if (process.env.NODE_ENV === 'development') { OverflowContainerContext.displayName = 'OverflowContainerContext'; } +/** + * @private + * Hook that returns the number of items that should be visible based on the size of the container element. + * + * The returned number should be used to render the element with fewer items. + * + * @param itemsCount Number of items that this element contains. + * @param orientation 'horizontal' (default) or 'vertical' + * @returns [callback ref to set on container, stateful count of visible items] + * + * @example + * const items = Array(10).fill().map((_, i) => Item {i}); + * const [ref, visibleCount] = useOverflow(items.count); + * ... + * return ( + *
+ * {items.slice(0, visibleCount)} + *
+ * ); + */ +const useOverflow = ( + itemsCount: number, + orientation: 'horizontal' | 'vertical' = 'horizontal', +) => { + const [guessState, dispatch] = React.useReducer( + overflowGuessReducer, + { itemsCount }, + overflowGuessReducerInitialState, + ); + + const { minGuess, maxGuess, isStabilized, visibleCount } = guessState; + + const containerRef = React.useRef(null); + const isGuessing = React.useRef(false); + + /** + * Call this function to guess the new `visibleCount`. + * The `visibleCount` is not changed if the correct `visibleCount` has already been found. + * + * The logic of finding the correct `visibleCount` is similar to binary search. + * Logic (without all edge cases): + * - Have a guess range for `visibleCount` of `(0, x]` (0 is exclusive and x is inclusive) + * - 0 is exclusive as the minimum `visibleItems` always has to be 1. + * - The only exception is when the `itemsCount` is itself 0. + * - x can be an any arbitrary number ≤ `itemsCount`. + * - Initial `visibleCount` = max guess. + * - We NEED an overflow in the beginning for the algorithm to work. + * - Because the max guess should always be `≥` the correct `visibleCount`. + * - So, if not overflow, shift the guess range forward by: + * - doubling the max guess: since we need to overflow + * - setting min guess to current visibleCount: since not overflow means correct visibleCount ≥ current visibleCount + * - setting visible count to the new max guess + * - Shift the guess range forward repeatedly until the container overflows. + * - After the first overflow, `visibleCount` = average of the two guesses. + * - Repeat the following (`guessVisibleCount()`): + * - If container overflows, new max guess = current `visibleCount`. + * - If container does not overflow, new min guess = current `visibleCount`. + * - new `visibleCount` = the average of the new min and max guesses. + * - Stop when the average of the two guesses is the min guess itself. i.e. no more averaging possible. + * - The min guess is then the correct `visibleCount`. + */ + const guessVisibleCount = React.useCallback(() => { + // If already stabilized, already guessing, or in unit test, do not guess. + if (isStabilized || isGuessing.current || isUnitTest) { + return; + } + + try { + isGuessing.current = true; + + // We need to wait for the ref to be attached so that we can measure available and required sizes. + if (containerRef.current == null) { + return; + } + + const dimension = orientation === 'horizontal' ? 'Width' : 'Height'; + const availableSize = containerRef.current[`offset${dimension}`]; + const requiredSize = containerRef.current[`scroll${dimension}`]; + + const isOverflowing = availableSize < requiredSize; + + if ( + // there are no items + itemsCount === 0 || + // overflowing when even 1 item is present + (visibleCount === 1 && isOverflowing) || + // no overflow when rendering all items + (visibleCount === itemsCount && !isOverflowing) || + // if the new average of the guess range will never change the visibleCount anymore (infinite loop) + (maxGuess - minGuess === 1 && visibleCount === minGuess) + ) { + dispatch({ type: 'stabilize' }); + return; + } + + // Before the main logic, the max guess MUST be ≥ the correct visibleCount for the algorithm to work. + // If not, should shift the guess range forward to induce the first overflow. + if (maxGuess === visibleCount && !isOverflowing) { + dispatch({ type: 'shiftGuessRangeForward' }); + return; + } + + if (isOverflowing) { + // overflowing = we guessed too high. So, decrease max guess + dispatch({ type: 'decreaseMaxGuess' }); + } else { + // not overflowing = maybe we guessed too low? So, increase min guess + dispatch({ type: 'increaseMinGuess' }); + } + } finally { + isGuessing.current = false; + } + }, [isStabilized, itemsCount, maxGuess, minGuess, orientation, visibleCount]); + + // Guess the visible count until stabilized. + // To prevent flicking, use useLayoutEffect to paint only after stabilized. + useLayoutEffect(() => { + if (!isStabilized) { + guessVisibleCount(); + } + }, [guessVisibleCount, isStabilized]); + + return [containerRef, visibleCount] as const; +}; + +// ---------------------------------------------------------------------------- + +type GuessState = ( + | { + isStabilized: true; + minGuess: null; + maxGuess: null; + } + | { + isStabilized: false; + minGuess: number; + maxGuess: number; + } +) & { + itemsCount: number; + visibleCount: number; +}; + +type GuessAction = + | { + /** + * - `"decreaseMaxGuess"`: When overflowing, do the following: + * - New `maxGuess` = current `visibleCount`. + * - New `visibleCount` = average of the `minGuess` and new `maxGuess`. + */ + type: 'decreaseMaxGuess'; + } + | { + /** + * - `"increaseMinGuess"`: When not overflowing, do the following: + * - New `minGuess` = current `visibleCount`. + * - New `visibleCount` = average of the `maxGuess` and new `minGuess`. + */ + type: 'increaseMinGuess'; + } + | { + /** + * - `"shiftGuessRangeForward"`: Useful to induce the first overflow to start guessing. + * - Shift the guess range forward by: + * - doubling the max guess: since we need to overflow + * - setting min guess to current visibleCount: since underflow means correct visibleCount ≥ current + * visibleCount + * - setting visible count to the new max guess + */ + type: 'shiftGuessRangeForward'; + } + | { + /** + * - `"stabilize"`: Stop guessing as `visibleCount` is the correct value. + */ + type: 'stabilize'; + }; + +/** Arbitrary initial max guess for `visibleCount`. We refine this max guess with subsequent renders. */ +const STARTING_MAX_ITEMS_COUNT = 32; + +const overflowGuessReducerInitialState = ({ + itemsCount, +}: Pick): GuessState => { + const initialVisibleCount = Math.min(itemsCount, STARTING_MAX_ITEMS_COUNT); + + // In unit test environments, always show all items + return isUnitTest + ? { + isStabilized: true, + minGuess: null, + maxGuess: null, + itemsCount, + visibleCount: itemsCount, + } + : { + isStabilized: false, + minGuess: 0, + maxGuess: initialVisibleCount, + itemsCount, + visibleCount: initialVisibleCount, + }; +}; + +const overflowGuessReducer = ( + state: GuessState, + action: GuessAction, +): GuessState => { + /** Ensure that the visibleCount is always ≤ itemsCount */ + const getSafeVisibleCount = (visibleCount: number) => + Math.min(state.itemsCount, visibleCount); + + switch (action.type) { + case 'decreaseMaxGuess': + case 'increaseMinGuess': + if (state.isStabilized) { + return state; + } + + let newMinGuess = state.minGuess; + let newMaxGuess = state.maxGuess; + + if (action.type === 'decreaseMaxGuess') { + newMaxGuess = state.visibleCount; + } else { + newMinGuess = state.visibleCount; + } + + // Next guess is always the middle of the new guess range + const newVisibleCount = Math.floor((newMinGuess + newMaxGuess) / 2); + + return { + ...state, + isStabilized: false, + minGuess: newMinGuess, + maxGuess: newMaxGuess, + visibleCount: getSafeVisibleCount(newVisibleCount), + }; + case 'shiftGuessRangeForward': + if (state.isStabilized) { + return state; + } + + const doubleOfMaxGuess = state.maxGuess * 2; + + return { + ...state, + isStabilized: false, + minGuess: state.maxGuess, + maxGuess: doubleOfMaxGuess, + visibleCount: getSafeVisibleCount(doubleOfMaxGuess), + }; + case 'stabilize': + return { + ...state, + isStabilized: true, + minGuess: null, + maxGuess: null, + }; + default: + return state; + } +}; + +// ---------------------------------------------------------------------------- + function useOverflowContainerContext() { const overflowContainerContext = useSafeContext(OverflowContainerContext); return overflowContainerContext; diff --git a/packages/itwinui-react/src/utils/hooks/index.ts b/packages/itwinui-react/src/utils/hooks/index.ts index 03d8878ed1d..c7b890b917b 100644 --- a/packages/itwinui-react/src/utils/hooks/index.ts +++ b/packages/itwinui-react/src/utils/hooks/index.ts @@ -4,7 +4,6 @@ *--------------------------------------------------------------------------------------------*/ export * from './useEventListener.js'; export * from './useMergedRefs.js'; -export * from './useOverflow.js'; export * from './useResizeObserver.js'; export * from './useContainerWidth.js'; export * from './useGlobals.js'; diff --git a/packages/itwinui-react/src/utils/hooks/useOverflow.test.tsx b/packages/itwinui-react/src/utils/hooks/useOverflow.test.tsx deleted file mode 100644 index 9b67c5dae48..00000000000 --- a/packages/itwinui-react/src/utils/hooks/useOverflow.test.tsx +++ /dev/null @@ -1,216 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Bentley Systems, Incorporated. All rights reserved. - * See LICENSE.md in the project root for license terms and full copyright notice. - *--------------------------------------------------------------------------------------------*/ -import { act, render, waitFor } from '@testing-library/react'; -import * as React from 'react'; -import { useOverflow } from './useOverflow.js'; -import * as UseResizeObserver from './useResizeObserver.js'; - -const MockComponent = ({ - children, - disableOverflow = false, - orientation = 'horizontal', -}: { - children: React.ReactNode[] | string; - disableOverflow?: boolean; - orientation?: 'horizontal' | 'vertical'; -}) => { - const [overflowRef, visibleCount] = useOverflow( - children.length, - disableOverflow, - orientation, - ); - return
{children.slice(0, visibleCount)}
; -}; - -afterEach(() => { - vi.restoreAllMocks(); -}); - -it.each(['horizontal', 'vertical'] as const)( - 'should overflow when there is not enough space (%s)', - async (orientation) => { - const dimension = orientation === 'horizontal' ? 'Width' : 'Height'; - vi.spyOn(HTMLDivElement.prototype, `scroll${dimension}`, 'get') - .mockReturnValueOnce(120) - .mockReturnValue(100); - vi.spyOn( - HTMLDivElement.prototype, - `offset${dimension}`, - 'get', - ).mockReturnValue(100); - vi.spyOn( - HTMLSpanElement.prototype, - `offset${dimension}`, - 'get', - ).mockReturnValue(25); - - const { container } = render( - - {[...Array(5)].map((_, i) => ( - Test {i} - ))} - , - ); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(4); - }); - }, -); - -it('should overflow when there is not enough space (string)', async () => { - vi.spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(50) - .mockReturnValue(28); - vi.spyOn(HTMLDivElement.prototype, 'offsetWidth', 'get').mockReturnValue(28); - - // 20 symbols (default value taken), 50 width - // avg 2.5px per symbol - const { container } = render( - This is a very long text., - ); - - // have 28px of a place - // 11 symbols can fit - await waitFor(() => { - expect(container.textContent).toBe('This is a v'); - }); -}); - -it('should overflow when there is not enough space but container fits 30 items', async () => { - vi.spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(300) - .mockReturnValueOnce(600) - .mockReturnValue(300); - vi.spyOn(HTMLDivElement.prototype, 'offsetWidth', 'get').mockReturnValue(300); - vi.spyOn(HTMLSpanElement.prototype, 'offsetWidth', 'get').mockReturnValue(10); - - const { container } = render( - - {[...Array(100)].map((_, i) => ( - Test {i} - ))} - , - ); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(30); - }); -}); - -it('should restore hidden items when space is available again', async () => { - let onResizeFn: (size: DOMRectReadOnly) => void = vi.fn(); - vi.spyOn(UseResizeObserver, 'useResizeObserver').mockImplementation( - (onResize) => { - onResizeFn = onResize; - return [vi.fn(), { disconnect: vi.fn() } as unknown as ResizeObserver]; - }, - ); - const scrollWidthSpy = vi - .spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(120) - .mockReturnValue(100); - const offsetWidthSpy = vi - .spyOn(HTMLDivElement.prototype, 'offsetWidth', 'get') - .mockReturnValue(100); - vi.spyOn(HTMLSpanElement.prototype, 'offsetWidth', 'get').mockReturnValue(25); - - const { container, rerender } = render( - - {[...Array(5)].map((_, i) => ( - Test {i} - ))} - , - ); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(4); - }); - - scrollWidthSpy.mockReturnValue(125); - offsetWidthSpy.mockReturnValue(125); - rerender( - - {[...Array(5)].map((_, i) => ( - Test {i} - ))} - , - ); - - act(() => onResizeFn({ width: 125 } as DOMRectReadOnly)); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(5); - }); -}); - -it('should not overflow when disabled', () => { - vi.spyOn(HTMLElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(120) - .mockReturnValue(100); - vi.spyOn(HTMLElement.prototype, 'offsetWidth', 'get').mockReturnValue(100); - - const { container } = render( - - {[...Array(50)].map((_, i) => ( - Test {i} - ))} - , - ); - - expect(container.querySelectorAll('span')).toHaveLength(50); -}); - -it('should hide items and then show them all when overflow is disabled', async () => { - vi.spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(300) - .mockReturnValueOnce(600) - .mockReturnValue(300); - vi.spyOn(HTMLDivElement.prototype, 'offsetWidth', 'get').mockReturnValue(300); - vi.spyOn(HTMLSpanElement.prototype, 'offsetWidth', 'get').mockReturnValue(10); - - const { container, rerender } = render( - - {[...Array(100)].map((_, i) => ( - Test {i} - ))} - , - ); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(30); - }); - - rerender( - - {[...Array(100)].map((_, i) => ( - Test {i} - ))} - , - ); - - await waitFor(() => { - expect(container.querySelectorAll('span')).toHaveLength(100); - }); -}); - -it('should return 1 when item is bigger than the container', () => { - vi.spyOn(HTMLDivElement.prototype, 'scrollWidth', 'get') - .mockReturnValueOnce(50) - .mockReturnValueOnce(100) - .mockReturnValue(50); - vi.spyOn(HTMLDivElement.prototype, 'offsetWidth', 'get').mockReturnValue(50); - vi.spyOn(HTMLSpanElement.prototype, 'offsetWidth', 'get').mockReturnValue(60); - - const { container } = render( - - {[...Array(5)].map((_, i) => ( - Test {i} - ))} - , - ); - - expect(container.querySelectorAll('span')).toHaveLength(1); -}); diff --git a/packages/itwinui-react/src/utils/hooks/useOverflow.tsx b/packages/itwinui-react/src/utils/hooks/useOverflow.tsx deleted file mode 100644 index 14aa58641ea..00000000000 --- a/packages/itwinui-react/src/utils/hooks/useOverflow.tsx +++ /dev/null @@ -1,109 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Bentley Systems, Incorporated. All rights reserved. - * See LICENSE.md in the project root for license terms and full copyright notice. - *--------------------------------------------------------------------------------------------*/ -import * as React from 'react'; -import { useMergedRefs } from './useMergedRefs.js'; -import { useResizeObserver } from './useResizeObserver.js'; -import { useLayoutEffect } from './useIsomorphicLayoutEffect.js'; - -const STARTING_MAX_ITEMS_COUNT = 20; - -/** - * Hook that observes the size of an element and returns the number of items - * that should be visible based on the size of the container element. - * - * The returned number should be used to render the element with fewer items. - * - * @private - * @param itemsCount Number of items that this element contains. - * @param disabled Set to true to disconnect the observer. - * @param dimension 'horizontal' (default) or 'vertical' - * @returns [callback ref to set on container, stateful count of visible items] - * - * @example - * const items = Array(10).fill().map((_, i) => Item {i}); - * const [ref, visibleCount] = useOverflow(items); - * ... - * return ( - *
- * {items.slice(0, visibleCount)} - *
- * ); - */ -export const useOverflow = ( - itemsCount: number, - disabled = false, - orientation: 'horizontal' | 'vertical' = 'horizontal', -) => { - const containerRef = React.useRef(null); - - const [visibleCount, setVisibleCount] = React.useState(() => - disabled ? itemsCount : Math.min(itemsCount, STARTING_MAX_ITEMS_COUNT), - ); - - const needsFullRerender = React.useRef(true); - - const [containerSize, setContainerSize] = React.useState(0); - const previousContainerSize = React.useRef(0); - const updateContainerSize = React.useCallback( - ({ width, height }: DOMRectReadOnly) => - setContainerSize(orientation === 'horizontal' ? width : height), - [orientation], - ); - const [resizeRef, observer] = useResizeObserver(updateContainerSize); - const resizeObserverRef = React.useRef(observer); - - useLayoutEffect(() => { - if (disabled) { - setVisibleCount(itemsCount); - } else { - setVisibleCount(Math.min(itemsCount, STARTING_MAX_ITEMS_COUNT)); - needsFullRerender.current = true; - } - }, [containerSize, disabled, itemsCount]); - - const mergedRefs = useMergedRefs(containerRef, resizeRef); - - useLayoutEffect(() => { - if (!containerRef.current || disabled) { - resizeObserverRef.current?.disconnect(); - return; - } - const dimension = orientation === 'horizontal' ? 'Width' : 'Height'; - - const availableSize = containerRef.current[`offset${dimension}`]; - const requiredSize = containerRef.current[`scroll${dimension}`]; - - if (availableSize < requiredSize) { - const avgItemSize = requiredSize / visibleCount; - const visibleItems = Math.floor(availableSize / avgItemSize); - /* When first item is larger than the container - visibleItems count is 0, - We can assume that at least some part of the first item is visible and return 1. */ - setVisibleCount(visibleItems > 0 ? visibleItems : 1); - } else if (needsFullRerender.current) { - const childrenSize = Array.from(containerRef.current.children).reduce( - (sum: number, child: HTMLElement) => sum + child[`offset${dimension}`], - 0, - ); - // Previous `useEffect` might have updated visible count, but we still have old one - // If it is 0, lets try to update it with items length. - const currentVisibleCount = - visibleCount || Math.min(itemsCount, STARTING_MAX_ITEMS_COUNT); - const avgItemSize = childrenSize / currentVisibleCount; - const visibleItems = Math.floor(availableSize / avgItemSize); - - if (!isNaN(visibleItems)) { - // Doubling the visible items to overflow the container. Just to be safe. - setVisibleCount(Math.min(itemsCount, visibleItems * 2)); - } - } - needsFullRerender.current = false; - }, [containerSize, visibleCount, disabled, itemsCount, orientation]); - - useLayoutEffect(() => { - previousContainerSize.current = containerSize; - }, [containerSize]); - - return [mergedRefs, visibleCount] as const; -}; diff --git a/testing/e2e/app/routes/ButtonGroup/spec.ts b/testing/e2e/app/routes/ButtonGroup/spec.ts index 9d7f0250300..2a8e0ebe67a 100644 --- a/testing/e2e/app/routes/ButtonGroup/spec.ts +++ b/testing/e2e/app/routes/ButtonGroup/spec.ts @@ -6,6 +6,8 @@ test.describe('ButtonGroup (toolbar)', () => { }) => { await page.goto('/ButtonGroup'); + await page.waitForTimeout(50); + await page.keyboard.press('Tab'); await expect(page.getByRole('button', { name: 'Button 1' })).toBeFocused(); @@ -36,6 +38,8 @@ test.describe('ButtonGroup (toolbar)', () => { }) => { await page.goto('/ButtonGroup?orientation=vertical'); + await page.waitForTimeout(50); + await page.keyboard.press('Tab'); await expect(page.getByRole('button', { name: 'Button 1' })).toBeFocused(); diff --git a/testing/e2e/app/routes/ComboBox/route.tsx b/testing/e2e/app/routes/ComboBox/route.tsx index 9a81c59a126..6d117bc2b1a 100644 --- a/testing/e2e/app/routes/ComboBox/route.tsx +++ b/testing/e2e/app/routes/ComboBox/route.tsx @@ -1,10 +1,13 @@ -import { Button, ComboBox } from '@itwin/itwinui-react'; +import { Button, ComboBox, Flex } from '@itwin/itwinui-react'; import { useSearchParams } from '@remix-run/react'; import * as React from 'react'; export default function ComboBoxTest() { const config = getConfigFromSearchParams(); + if (config.exampleType === 'overflow') { + return ; + } return ; } @@ -46,12 +49,66 @@ const Default = ({ ); }; +const Overflow = () => { + const data = new Array(15).fill(0).map((_, i) => ({ + label: `option ${i}`, + value: i, + })); + const widths = new Array(10).fill(0).map((_, i) => 790 + i * 3); + + const [ + selectTagContainersDomChangeCount, + setSelectTagContainersDomChangeCount, + ] = React.useState(0); + + React.useEffect(() => { + const observer = new MutationObserver(() => + setSelectTagContainersDomChangeCount((count) => count + 1), + ); + + const selectTagContainers = document.querySelectorAll( + "[role='combobox'] + div:first-of-type", + ); + selectTagContainers.forEach((container) => { + observer.observe(container, { + attributes: false, + childList: true, + subtree: false, + }); + }); + + return () => { + observer.disconnect(); + }; + }, []); + + return ( + +

+ Select tag containers DOM changes: {selectTagContainersDomChangeCount} +

+ {widths.slice(0, 10).map((width) => ( + x.value)} + /> + ))} +
+ ); +}; + // ---------------------------------------------------------------------------- function getConfigFromSearchParams() { const [searchParams] = useSearchParams(); - const exampleType = searchParams.get('exampleType') as 'default' | undefined; + const exampleType = searchParams.get('exampleType') as + | 'default' + | 'overflow' + | undefined; const virtualization = searchParams.get('virtualization') === 'true'; const multiple = searchParams.get('multiple') === 'true'; const clearFilterOnOptionToggle = diff --git a/testing/e2e/app/routes/ComboBox/spec.ts b/testing/e2e/app/routes/ComboBox/spec.ts index e2bd3554f9e..aac88597a5f 100644 --- a/testing/e2e/app/routes/ComboBox/spec.ts +++ b/testing/e2e/app/routes/ComboBox/spec.ts @@ -74,6 +74,20 @@ test('should select multiple options', async ({ page }) => { }); }); +test('should not have flickering tags (fixes #2112)', async ({ page }) => { + await page.goto('/ComboBox?exampleType=overflow'); + + // Wait for page to stabilize + await page.waitForTimeout(30); + + const stabilizedCount = await getSelectTagContainerDomChangeCount(page); + await page.waitForTimeout(100); + const newCount = await getSelectTagContainerDomChangeCount(page); + + // DOM should not change with time (i.e. no flickering) + expect(stabilizedCount).toBe(newCount); +}); + test(`should clear filter and set input value when an option is toggled and when focus is lost (multiple=false)`, async ({ page, }) => { @@ -444,3 +458,12 @@ const getSelectTagContainerTags = (page: Page) => { // See: https://github.com/iTwin/iTwinUI/pull/2151#discussion_r1684394649 return page.getByRole('combobox').locator('+ div > span'); }; + +const getSelectTagContainerDomChangeCount = async (page: Page) => { + const selectTagContainerDomChangeCounter = page.getByTestId( + 'select-tag-containers-dom-change-count', + ); + return (await selectTagContainerDomChangeCounter.textContent()) + ?.split(':')[1] + .trim(); +};