react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Button in ComboBox not displaying Popup when value in ListBoxItem is to long

Open tordsta opened this issue 1 year ago • 34 comments

Provide a general summary of the issue here

When you have a ListBoxItem with a value longer than the Input/ComboBox component, the Button component inside ComboBox stops displaying the Popover when clicked with mouse. For example: <ListBoxItem>Dooooooooooooooooooooooooooooooooog</ListBoxItem>

🤔 Expected Behavior?

When a long ListBoxItem is selected, the Popover should open when the "<Button>▼</Button>" button is clicked.

😯 Current Behavior

When a long ListBoxItem is selected, the Popover does not open when the "<Button>▼</Button>" button is clicked.

💁 Possible Solution

I dont know what the underlying issue is.

🔦 Context

I just want to be able to have longer values in ListBoxItem.

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/elated-rumple-c5q8r5?file=/src/App.js:30,17

Version

1.0.0

What browsers are you seeing the problem on?

Other

If other, please specify.

Arc

What operating system are you using?

macOS Sonoma 14.1

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

tordsta avatar Jan 04 '24 11:01 tordsta

I'm seeing this in Chrome, but not Safari, so it seems like a browser feature where it focuses the text input if you click over where the overflowing text is. I'm not sure that this is a RAC bug, but more something that needs to be solved with styling.

reidbarber avatar Jan 04 '24 15:01 reidbarber

@reidbarber @tordsta just going to chime in and say that onSelectionChange is being called when the text overflows. Still think some CSS should solve it?

p1pah avatar Jan 29 '24 04:01 p1pah

I'm not seeing onSelectionChange get called when the button is clicked over the overflowed text:

https://github.com/adobe/react-spectrum/assets/8961049/e1e25b4d-5a2f-4a07-aa1d-18bbbc0a0ecb

reidbarber avatar Jan 29 '24 15:01 reidbarber

Also running into this issue and wanted to add some additional info I've found when trying to debug.

First I don't think the issue is that the menu doesn't open at all, it's that it opens and closes very quickly. In certain environments this is so quick you don't see it, but you'll notice it more clearly in other environments, check out the below videos.

Second is that it seems to be related to if the input is scrolled over or not. See the below video where you can see when the input is scrolled all the way to the left to the beginning of the word it works just fine, but when the cursor is on the far right and the word is scrolled over it breaks.

https://github.com/adobe/react-spectrum/assets/21112335/5c1e0241-7bea-4210-9bb4-31c7ba10a53e

This is not only at the very beginning or end cursor placements, you're cursor can be anywhere in the input and you will see the same behaviors purely based on if the input is scrolled or not.

The third thing I've noticed is that it doesn't necessarily have be a selected list box item. You can just type directly in the input long enough for it to scroll and you will experience the same issue. Here is an example from the React Aria Components example storybook itself (https://react-spectrum.adobe.com/react-aria-tailwind-starter/index.html?path=/docs/combobox--docs). In this video it best demonstrates the opening and closing, its very delayed in this storybook for some reason. Also, on this one you will notice that it clears out the input as well; this seems to happen when you are manually typing out an option that doesn't exist in the list box.

https://github.com/adobe/react-spectrum/assets/21112335/1833c506-51ee-455c-88e0-cf1cc95fb3cd

And lastly, this isn't only an issue with pushing the dropdown button. It also happens when typing in the box with the exact same behaviours as described above. At the beginning of this video you will see me move my cursor to the end of the word and just hold backspace. The backspacing keeps resetting and the menu spams open and close. Then I move the cursor to near the front of the word and it works fine. You will also see that when I type gibberish backspacing works fine no matter where you are, for it to break it has to be an actual item in the options list.

https://github.com/adobe/react-spectrum/assets/21112335/8a4ca038-1ed6-41f9-ba9a-c0e264b22ffc

Please let me know if there's anything else I can do to help!

max-two avatar Feb 02 '24 01:02 max-two

@LFDanLu You mentioned in another combobox issue this PR about closing comboboxes on scroll.

I don't know this code well enough to say one way or the other, but it sounds potentially related to this where the input being scrolled seems to close the dropdown menu immediately.

max-two avatar Feb 02 '24 02:02 max-two

Decided to check the down arrow behaviour because of the other thread I linked above and it is also wonky. If you type out a bunch of characters long enough to get input box scrolling and then hit down arrow sometimes it will open / close immediately or sometimes it will stay open. But even if it stay open, it will close and reset input as soon as you try to type another character.

https://github.com/adobe/react-spectrum/assets/21112335/d55327b2-2af6-4eff-add5-954a991a3df6

max-two avatar Feb 02 '24 02:02 max-two

@LFDanLu You mentioned in another combobox issue this PR about closing comboboxes on scroll.

I don't know this code well enough to say one way or the other, but it sounds potentially related to this where the input being scrolled seems to close the dropdown menu immediately.

It indeed seems to have something to do with a scroll listener.

In particular, I've noticed the following onScroll() is being called when I click the trigger button while a long item is selected, closing the popover that was just open by the button action: https://github.com/adobe/react-spectrum/blob/bd477d20344f491144dc34b9bd114070c2e9b10b/packages/%40react-aria/overlays/src/useCloseOnScroll.ts#L37-L49

Since in our case, onClose() is passed eventually to useCloseOnScroll() by usePopover() only when popover is non-modal: https://github.com/adobe/react-spectrum/blob/bd477d20344f491144dc34b9bd114070c2e9b10b/packages/%40react-aria/overlays/src/usePopover.ts#L101

If I force ComboBox to pass the PopoverConext with isNonModal: false in my local dev environment, I see the popover behave correctly when the trigger button is pressed:

<Provider
  values={[
    [ComboBoxStateContext, state],
    [LabelContext, {...labelProps, ref: labelRef}],
    [ButtonContext, {...buttonProps, ref: buttonRef, isPressed: state.isOpen}],
    [InputContext, {...inputProps, ref: inputRef}],
    [OverlayTriggerStateContext, state],
    [PopoverContext, {
      ref: popoverRef,
      triggerRef: inputRef,
      placement: 'bottom start',
--      isNonModal: true,
++     isNonModal: false,
      trigger: 'ComboBox',
      style: {'--trigger-width': menuWidth} as React.CSSProperties
    }],
    [ListBoxContext, {...listBoxProps, ref: listBoxRef}],
    [ListStateContext, state],
    [TextContext, {
      slots: {
        description: descriptionProps,
        errorMessage: errorMessageProps
      }
    }],
    [GroupContext, {isInvalid: validation.isInvalid, isDisabled: props.isDisabled || false}],
    [FieldErrorContext, validation]
  ]}>

isNonModal: true

https://github.com/adobe/react-spectrum/assets/71210554/d99b07b9-264d-4f3e-ad52-26e3f856cfa2

isNonModal: false

https://github.com/adobe/react-spectrum/assets/71210554/2f444376-dda4-4fcd-9c08-699fda8dc757

Now don't get me wrong though, I'm not suggesting isNonModal: false as a possible fix for this issue 😅. (just wanted to show you the difference in popover behavior)

sookmax avatar Feb 02 '24 14:02 sookmax

Thanks for the additional information @max-two and @sookmax! The combobox's close on scroll behavior being triggered due to a scroll event from a long input value is definitely not what was intended, will have to see if we can special case this scenario.

LFDanLu avatar Feb 02 '24 17:02 LFDanLu

Another observation:

  • If the caret is at the beginning of the long input, ArrowDown fails to open the Popover.
  • If the caret is at the end of the long input, clicking the button fails to open the Popover.

This means that no single caret positioning strategy successfully circumvents this issue. The only thing I've seen kind of work is disabling the input before the popover opens.

Might there be some variation of @sookmax's isNonModal observation that could be used as a workaround while this issue is open?

staticshock avatar Mar 22 '24 04:03 staticshock

Hey guys, is this issue somehow magically fixed..?

I've notice the above example at https://codesandbox.io/p/sandbox/elated-rumple-c5q8r5?file=/src/App.js:30,17 working correctly now?

And I'm no longer able to reproduce it locally either.. I'm not sure what was the case before, but now every time I open the popover via the trigger button, the following onResize() that is registered as visualViewport?.addEventListener('resize', onResize); runs:

https://github.com/adobe/react-spectrum/blob/f6a664ba900c3e6307a0f9461241aa3b2397efeb/packages/%40react-aria/overlays/src/useOverlayPosition.ts#L195-L204

It sets isResizing.current to true until the timeout runs after 500ms, which has an effect on whether onClose() (provided by usePopover()) is called:

https://github.com/adobe/react-spectrum/blob/f6a664ba900c3e6307a0f9461241aa3b2397efeb/packages/%40react-aria/overlays/src/useOverlayPosition.ts#L222-L234

What I've noticed is that by the time useCloseOnScroll() calls onCloseHandler(), now isResizing.current is true, not invoking onClose() and hence not closing the popover as a result:

https://github.com/adobe/react-spectrum/blob/f6a664ba900c3e6307a0f9461241aa3b2397efeb/packages/%40react-aria/overlays/src/useCloseOnScroll.ts#L37-L51

Has there been a change in VisualViewport API that might have altered when 'resize' event is called? Or was it always like this and the root cause of this lies somewhere else?

sookmax avatar Mar 22 '24 12:03 sookmax

I'm still reproducing the original problem via your codesandbox link, @sookmax.

staticshock avatar Mar 22 '24 14:03 staticshock

https://github.com/adobe/react-spectrum/assets/71210554/9983fdd4-118e-4e7f-9a23-490486326682

Hm.. guess it's just my environment then? I just recorded a video clicking around on the example, and should I not be able to open the popover while the long item is being selected? The problem is.. I just can?

Another video on the story:

https://github.com/adobe/react-spectrum/assets/71210554/d78d521e-b79d-418b-bf57-cf66c3aebc58

Tested on MacBook Air M2, 2022 Sonoma 14.4 & Windows 11 Home OS build 22631.3296 (Chrome only)

  • Chrome: Version 123.0.6312.59 (Official Build) (arm64)
  • FireFox: 124.0.1
  • Safari: Version 17.2.1

Please someone tell me I'm not the only one seeing this in their local environment.. I'm not insane.. 😭

sookmax avatar Mar 22 '24 15:03 sookmax

I'm unable to reproduce as well any more. Not quite sure what changed MacOS 14.1 Chrome: 122.0.6261.129 Firefox: 123.0.1 Safari: 17.1

Windows 11 Chrome: 123.0.6312.59 Firefox: 124.0.1 Edge: 122.0.2365.92

@staticshock As for a workaround, you could revert the changes from https://github.com/adobe/react-spectrum/pull/5453/files perhaps if absolutely need be, the combobox had existed for quite some time without closing its popover on scrolling and I think reverting that temporarily in a local patch package for your app/project isn't the worst. Alternatively, I've been meaning to look into a workaround of sorts where we either ignore scroll events that don't affect the trigger's actual position in useCloseOnScroll or to just special case isNonModal=true cases where we further delay the close on scroll listening

LFDanLu avatar Mar 22 '24 18:03 LFDanLu

Thanks, @LFDanLu. And thanks for the diagnosis, @sookmax!

For the record, adding isNonModal={false} to the Popover fixes the issue for me:

<ComboBox>
  <Group>
    <Input />
    <Button>Open</Button>
  </Group>
  <Popover isNonModal={false}>
    <ListBox>
      <ListBoxItem>
        Something long enough that it overflows the box
      </ListBoxItem>
    </ListBox>
  </Popover>
</ComboBox>

With isNonModal={true} the issue becomes reproducible again.

Any chance this is a race condition, where the difference in performance between our machines could be a factor?

staticshock avatar Mar 22 '24 21:03 staticshock

Adding isNonModal={false} is non-desirable here because it because it has accessibility implications due to https://github.com/adobe/react-spectrum/blob/f6a664ba900c3e6307a0f9461241aa3b2397efeb/packages/%40react-aria/overlays/src/usePopover.ts#L108-L112 which will hide the combobox's input from screen readers when the modal is open. For normal modals, hiding all elements other than the modal from screenreaders is nice since it provides a focused context for screen reader users. However, for a combobox, the input controls what items appear in the popover while it is open so the screen reader needs to be able see/interact with the input element still.

I don't quite recall why isNonModal is also tied to page scrollability and why we don't simply freeze the ComboBox page scrolling like other popovers (which is probably why the issue is resolved when setting isNonModal={false}), will have to dig up the discourse on those changes since that goes quite a ways back.

Any chance this is a race condition, where the difference in performance between our machines could be a factor?

Given the 500ms timeout that @sookmax has unearthed, its entirely possible. I'm on a 2019 MacBook so not super new, what machine are you seeing this issue with?

LFDanLu avatar Mar 22 '24 22:03 LFDanLu

2021 MacBook Pro w/M1 Pro chip

staticshock avatar Mar 23 '24 03:03 staticshock

@staticshock Can you also share us the version of the browser that you're using?

sookmax avatar Mar 23 '24 08:03 sookmax

Chrome 123.0.6312.59

staticshock avatar Mar 23 '24 17:03 staticshock

I'm unable to reproduce with Chrome 123.0.6312.59 on MacOS 14.4 and 2021 M1 Max

I'm also unable to reproduce with trackpad tap to click or press to click Screenshot 2024-03-24 at 1 07 42 PM

What input device are you using? do you have any special settings for your input device?

snowystinger avatar Mar 24 '24 02:03 snowystinger

I'm using a regular (external) trackpad. I'm kind of assuming that my inputs are registering as regular mouse inputs.

Screenshot 2024-03-23 at 8 16 39 PM

staticshock avatar Mar 24 '24 03:03 staticshock

Maybe, just trying to rule out anything associated with cross browser and cross input normalization we do. Any sort of a lead or difference so we can reproduce as well because I do believe you're seeing the issue.

snowystinger avatar Mar 24 '24 04:03 snowystinger

@staticshock

When 'Tap to click' is enabled in Trackpad options, state.pointerType in usePress() would become 'virtual' as opposed to null, which is the case for a physical trackpad press and a mouse click.

Not that I'm saying it's related to you observing the issue, but just wanted to mention it just in case. (I use 'Tap to click' as well albeit mine is the built-in trackpad, and I'm not able to reproduce the bug anymore with it.)

sookmax avatar Mar 25 '24 15:03 sookmax

Good thought. I just disabled tap to click, and that did not make a difference. I'm still consistently reproducing the bug via the original sandbox.

staticshock avatar Mar 25 '24 16:03 staticshock

I can also reproduce the issue on browserstack. Can one of you try loading up Chrome 123 on browserstack and trying it there?

staticshock avatar Mar 25 '24 19:03 staticshock

I just tried on browerstack on Chrome 123, wasn't able to reproduce, albiet the trial only gave me like a minute to try haha. Seeing if there is a corp account that I can use to play around with some more.

LFDanLu avatar Mar 25 '24 19:03 LFDanLu

I can also reproduce the issue on browserstack. Can one of you try loading up Chrome 123 on browserstack and trying it there?

Same, I can also reliably reproduce the issue on Chrome 124 and Brave 165. Setting the popover's isNonModal property to false resolves the issue in my local project.

psirenny avatar Apr 25 '24 09:04 psirenny

I can now reproduce it reliably via the original sandbox in Chrome 124, will see about if we can get some time to debug in an upcoming sprint.

LFDanLu avatar Apr 25 '24 17:04 LFDanLu

Take a snapshot of your entire universe down to the atoms—reproducibility is a precious gift!

staticshock avatar Apr 25 '24 17:04 staticshock

Looks like I'm the only outlier here that still cannot reproduce.. 😭 on Chrome Version 124.0.6367.119 (Official Build) (arm64)

https://github.com/adobe/react-spectrum/assets/71210554/82c80706-f502-4847-8ec0-ab2abbd7b096

sookmax avatar May 03 '24 14:05 sookmax

I can reproduce now following instructions in https://github.com/adobe/react-spectrum/issues/6322

snowystinger avatar May 03 '24 16:05 snowystinger