New and improved useOverflow algorithm

[r100-stack]

Changes

Pulled out of #2120 as PR 4 in the PR stack (PR stack order).

As mentioned in #2112 (comment), I believe the flickering is because of an infinite loop:

visibleCount from our useOverflow internal hook changes on each resize of iui-select-tag-container and each resize changes the visibleCount.

I think this is because our useOverflow hook makes the assumption that all items are around the same size and thus we can take an average:

iTwinUI/packages/itwinui-react/src/utils/hooks/useOverflow.tsx

Line 79 in 4f431e5

const avgItemSize = requiredSize / visibleCount;

While this may work for cases like the TablePaginator, it doesn't seem to work for the ComboBox case where tags could have different widths.

Thus, useOverflow should be refactored to use real sizes.

---

Logic of new algorithm for useOverflow:

• Have a guess range for visibleCount. e.g. [0, 32] (32 is an arbitrary choice).
• Keep doubling the max guess until the container overflows.
  i.e. the max guess should always be ≥ the correct visibleCount.
  • With each such doubling, the new min guess is the current max guess (since underflow = we guessed low).
• Set visibleCount to the maxGuess.
• Repeat the following by calling guessVisibleCount() (keep re-rendering but not painting):
  • Each time the container overflows, new max guess is the average of the two guesses (since overflow = we guessed high).
  • Each time the container does not overflow, new min guess is the average of the two guesses (since underflow = we guessed low).
• 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.

Why this works:

• This approach takes the real widths/heights into account. So, the previous flickering is no longer there.
• Even if there are 10k items (for example), the maximum number of items we render starts from 32 and keeps doubling until we reach the first overflow. After the first overflow, the number of items we render always goes down. So, performance is not affected because we don't render any more items than needed.
• Since this approach involves halving the guess in each render, I believe this approach is of the order of log(n).

---

Others:

• useOverflow removes disabled argument since we can just conditionally render OverflowContainer. (New OverflowContainer wrapper around useOverflow #2152 (comment))
• useOverflow returns the same values that is used to return before this PR. I didn't re-implement the WIP logic where useOverflow returned one more than the total number of items when all items are visible. (New OverflowContainer wrapper around useOverflow #2152 (comment))

Testing

• Remove failing unit tests since mocking scrollWidth and offsetWidth was no longer reasonable with the new algorithm implementation. Equivalent e2e already added in e2e tests for useOverflow logic #2151 from the PR chain.
• Testing overflowStart is correct:
  • e2e test
  • Comparing the storybook overflowStart with a playground from 3.13.1 with the same code.
• Added an e2e to confirm that ComboBox: Selected options jittering #2112 is indeed fixed.

When testing, I realized that in addition to fixing this bug, this PR also might improve performance. Specifically, the old algorithm made infinite changes in the DOM even when there is no flickering visually. But the new algorithm doesn't make these continuous DOM updates.

Code

import { ComboBox, Flex } from '@itwin/itwinui-react';
import React from 'react';

const App = () => {
  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);

  const observer = new MutationObserver(() =>
    setSelectTagContainersDomChangeCount((count) => count + 1),
  );

  React.useEffect(() => {
    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 (
    <Flex flexDirection='column' alignItems='flex-start'>
      <p data-testid='dom-changes-counter'>
        Select tag containers DOM changes: {selectTagContainersDomChangeCount}
      </p>
      {widths.slice(0, 10).map((width) => (
        <ComboBox
          key={width}
          style={{ width: `${width}px`, maxWidth: '80vw' }}
          multiple={true}
          options={data}
          value={data.map((x) => x.value)}
        />
      ))}
    </Flex>
  );
};

export default App;

Result

Before:

Halfway through the video, I increased the throttling percentage.

Enregistrement.de.l.ecran.2024-10-01.a.11.50.13.reduced.mov

After:

Enregistrement.de.l.ecran.2024-10-01.a.11.09.56.mov

Docs

• Added necessary JSDocs to explain the logic of the new algorithm and to the new props.
• Added changeset since ComboBox: Selected options jittering #2112 is now fixed.

[mayank99]

Before I review this, could you list out somewhere (in order) all PRs in the stack? It's a bit difficult to tell at a glance which PR goes where. I'm also not sure of the current status of #2152 (it's been open for 2 months).

Also it would be good to start merging some of the earlier PRs in the stack, unless there is a strong reason to keep them around. Long-lived branches can be difficult to keep up-to-date.

[r100-stack]

Before I review this, could you list out somewhere (in order) all PRs in the stack?

Sounds good, I mentioned it in #2120 (comment).

I'm also not sure of the current status of #2152 (it's been open for 2 months).

There's one unresolved comment that I had already replied to. If it resolves your comment, you can mark it as resolved. There's nothing else that I plan to add to that PR, unless we realize we need to change something in this PR when reviewing other PRs in the stack that merge into this one.

Also it would be good to start merging some of the earlier PRs in the stack, unless there is a strong reason to keep them around. Long-lived branches can be difficult to keep up-to-date.

Sounds good, will merge those PRs soon if it seems like there will likely be no more changes needed in them.