fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: Combobox with virtualized options doesnt page down/up as expected

Open daric81 opened this issue 1 year ago • 2 comments
trafficstars

Library

React Components / v9 (@fluentui/react-components)

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (24) x64 13th Gen Intel(R) Core(TM) i7-13700
    Memory: 10.71 GB / 31.70 GB
  Browsers:
    Edge: Chromium (127.0.2651.86)
    Internet Explorer: 11.0.19041.4355

Are you reporting an Accessibility issue?

None

Reproduction

https://stackblitz.com/edit/vcdhme

Bug Description

Actual Behavior

When using the page down keys , the first time the focus moves 10 options down. subsequent page down key presses only move down 2 options. This can be visible in the fluentui documentation sample for the Combobox Virtualizer.

Expected Behavior

I'd expect options to page down by the same number (10 at a time) until the end is reached.

Logs

No response

Requested priority

Blocking

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

Validations

  • [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [X] The provided reproduction is a minimal reproducible example of the bug.

daric81 avatar Aug 05 '24 09:08 daric81

need a explain

swapanmandi avatar Sep 06 '24 19:09 swapanmandi

I'm uncertain whether this is a bug or if it's functioning as designed. When pressing the page down key, it moves down by a quarter of the list's length, as seen here:

https://github.com/microsoft/fluentui/blob/04dd27b05240312e586cb545c2345b66e980d301/packages/react-components/react-virtualizer/library/src/hooks/useVirtualizerMeasure.ts#L63

However, it seems we can alter this behavior using bufferItems, as demonstrated in this example: https://stackblitz.com/edit/q8ygjj?file=src%2Fexample.tsx

@Mitch-At-Work, could you confirm if this behavior is expected? If not, I'm considering submitting a PR with the following adjustment:

/**
 * Number of items to append at each end, i.e. 'preload' each side before entering view.
 * Minimum: 1
 */
- const newBufferItems = bufferItems ?? Math.max(Math.ceil(length / 4), 1);
+ const newBufferItems = bufferItems ?? Math.max(length, 1);

dmytrokirpa avatar Oct 14 '24 13:10 dmytrokirpa

@Mitch-At-Work, could you please check the previous comment and share your thoughts on it?

dmytrokirpa avatar Oct 22 '24 11:10 dmytrokirpa

Hi all,

The Virtualizer does not inject any methods for page down/up so it will default to browser behavior - as the list is Virtualized this will often cause it to scroll to the last current item in the DOM, which in the case of combobox is about 5 items ahead of it's current position.

Applications may override this functionality by implementing a custom listener for page down/up to scroll the desired amount via the Virtualizers imperative scroll hooks and assigning focus.

We would not want to increase the default buffer items as the intent of virtualization is to keep a minimal amount of items within the DOM - however applications may pass through a custom bufferItems value via the props if they feel it improves the experience or specifically to improve certain interactions like this.

Regards, Mitch.


Mitch-At-Work avatar Oct 22 '24 22:10 Mitch-At-Work

I've drafted a PR here to showcase how the buffer sizing can be modified to accommodate this scenario at the application level: https://github.com/microsoft/fluentui/pull/33114

Note that also the bufferSize must be modified alongside bufferItems to ensure that we will re-calculate virtualizer indexes when we reach ~10 items from the edges so that there is always at least 10 more items in the DOM in each direction. I would still recommend overriding key events to drive scroll imperatively, but given the simplicity of comboBox items this should be an acceptable increase.

This also helped me identify a bug with initializing custom buffer sizes so thank you for raising 👍 (Have fixed this in the above PR as part of changes).

Mitch-At-Work avatar Oct 23 '24 00:10 Mitch-At-Work

It seems that the Virtualizer hook's state is being reset when ComboBox opens causing it to be incorrectly set to 0 length, I will need to investigate futher here.

Mitch-At-Work avatar Oct 23 '24 00:10 Mitch-At-Work

Thank you for the update, @Mitch-At-Work!

dmytrokirpa avatar Oct 23 '24 09:10 dmytrokirpa

We've landed the underlying Virtualizer fixes to ensure the bufferItems/bufferSize is respected, this PR to update story should be ready to land now once approved: https://github.com/microsoft/fluentui/pull/33114

Mitch-At-Work avatar Oct 25 '24 20:10 Mitch-At-Work