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.

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

Fix ComboBox options' tag flicking #2120

Draft
wants to merge 74 commits into
base: main
Choose a base branch
from

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Jun 27, 2024

Changes

Fixes #2112. 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:

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.

This PR refactors useOverflow with a new algorithm that now takes the actual widths/heights into consideration.

New algorithm for useOverflow

Logic:

  • 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.
    • Each time the container does not overflow, new min guess is the average of the two 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.

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).

While working on useOverflow, it seemed hard to get it to work by using refs and also not knowing the children. So, this PR creates a new private component called OverflowContainer that calls useOverflow internally. useOverflow is this now no longer used by any other component.

OverflowContainer:

  • We just pass it the children, overflowTag and overflowTagPlacement (e.g. overflow from start or end). It automatically places the overflowTag at the overflowTagPlacement.
  • For custom cases (e.g. where overflow doesn't happen exactly in the start or end), a function could be passed ((visibleCount) => React.ReactNode) as the child of OverflowContainer. Now what to render based be customized based on the visibleCount. itemLength is required in this case.

I also tested that this bug does not regress some related prior bug fixes. Examples:

Testing

  • Since useOverflow now renders multiple times, having unit tests to mock rendered sizes is now unreasonable. So, I replaced all failing unit tests related to useOverflow with e2e tests.
Before/After

Before:

Enregistrement.de.l.ecran.2024-06-28.a.13.28.29.mov

After:

Enregistrement.de.l.ecran.2024-06-28.a.13.25.54.mov

Docs

  • Added JSDocs for the new algorithm, components, hooks, etc. to better explain this new algorithm.
  • Added changesets.

@r100-stack r100-stack self-assigned this Jun 27, 2024
@r100-stack r100-stack changed the base branch from main to r/overflow-add-resize-leftover July 5, 2024 21:38
…will need to remove containerRef as that's causing the effect to run twice
…but could investigate why two guesses happen with no change.
i.e. double when called the first time doesn't double the guess.
Base automatically changed from r/overflow-add-resize-leftover to main July 8, 2024 20:31
An error occurred while trying to automatically change base from r/overflow-add-resize-leftover to main July 8, 2024 20:31
@@ -20,6 +20,7 @@
list-style-type: none;
user-select: none;
block-size: 100%;
inline-size: 100%;
Copy link
Member Author

@r100-stack r100-stack Jul 16, 2024

Choose a reason for hiding this comment

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

Needed this to allow the .iui-breadcrumbs-list's visibleCount to update whenever .iui-breadcrumbs resizes.

@r100-stack r100-stack marked this pull request as ready for review July 17, 2024 19:50
@r100-stack r100-stack requested review from a team as code owners July 17, 2024 19:50
@r100-stack r100-stack requested review from mayank99 and Ben-Pusey-Bentley and removed request for a team July 17, 2024 19:50
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Is it possible to split this into multiple stacked PRs? It's extremely difficult to understand/review it right now (unless you're looking for a quick LGTM)

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there's some flakiness/unreliability in the e2e tests when running on the GitHub CI (action run).

Investigating why it keeps failing only in the CI and not locally despite having awaits with the Promise returning expects.


if (isOverflowing) {
// overflowing = we guessed too high. So, new max guess = half the current guess
newVisibleCountGuessRange = [visibleCountGuessRange[0], visibleCount];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that I understand how this part of the algorithm works. This is apparently halving the visibleCountGuesssRange, but I don't know how visibleCount is half of the max for the guess range. I must be missing something, since I am able to test the overflow container out with the deploy preview and see that it is working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ComboBox: Selected options jittering
3 participants