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

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

Open
tordsta opened this issue Jan 4, 2024 · 34 comments · May be fixed by #6461
Open

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

tordsta opened this issue Jan 4, 2024 · 34 comments · May be fixed by #6461
Assignees
Labels

Comments

@tordsta
Copy link

tordsta commented Jan 4, 2024

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: Dooooooooooooooooooooooooooooooooog

🤔 Expected Behavior?

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

😯 Current Behavior

When a long ListBoxItem is selected, the Popover does not open when the "▼" 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

@reidbarber
Copy link
Member

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.

@ben-hapip
Copy link

@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?

@reidbarber
Copy link
Member

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

Screen.Recording.2024-01-29.at.9.37.38.AM.mov

@max-two
Copy link

max-two commented Feb 2, 2024

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.

Screen.Recording.2024-02-01.at.8.43.15.PM.mov

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.

Screen.Recording.2024-02-01.at.8.47.54.PM.mov

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.

Screen.Recording.2024-02-01.at.8.52.05.PM.mov

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

@max-two
Copy link

max-two commented Feb 2, 2024

@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
Copy link

max-two commented Feb 2, 2024

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.

Screen.Recording.2024-02-01.at.9.07.52.PM.mov

@sookmax
Copy link
Contributor

sookmax commented Feb 2, 2024

@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:

let onScroll = (e: MouseEvent) => {
// Ignore if scrolling an scrollable region outside the trigger's tree.
let target = e.target;
// window is not a Node and doesn't have contain, but window contains everything
if (!triggerRef.current || ((target instanceof Node) && !target.contains(triggerRef.current))) {
return;
}
let onCloseHandler = onClose || onCloseMap.get(triggerRef.current);
if (onCloseHandler) {
onCloseHandler();
}
};

Since in our case, onClose() is passed eventually to useCloseOnScroll() by usePopover() only when popover is non-modal:

onClose: isNonModal ? state.close : null

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

combobox_longitem_isnonmodal_true_1080.mov

isNonModal: false

combobox_longitem_isnonmodal_false_1080.mov

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)

@LFDanLu
Copy link
Member

LFDanLu commented Feb 2, 2024

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.

@staticshock
Copy link

staticshock commented Mar 22, 2024

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?

@sookmax
Copy link
Contributor

sookmax commented Mar 22, 2024

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:

let onResize = () => {
isResizing.current = true;
clearTimeout(timeout);
timeout = setTimeout(() => {
isResizing.current = false;
}, 500);
updatePosition();
};

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

let close = useCallback(() => {
if (!isResizing.current) {
onClose();
}
}, [onClose, isResizing]);
// When scrolling a parent scrollable region of the trigger (other than the body),
// we hide the popover. Otherwise, its position would be incorrect.
useCloseOnScroll({
triggerRef: targetRef,
isOpen,
onClose: onClose && close
});

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:

let onScroll = (e: MouseEvent) => {
// Ignore if scrolling an scrollable region outside the trigger's tree.
let target = e.target;
// window is not a Node and doesn't have contain, but window contains everything
if (!triggerRef.current || ((target instanceof Node) && !target.contains(triggerRef.current))) {
return;
}
let onCloseHandler = onClose || onCloseMap.get(triggerRef.current);
if (onCloseHandler) {
onCloseHandler();
}
};
window.addEventListener('scroll', onScroll, true);

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?

@staticshock
Copy link

staticshock commented Mar 22, 2024

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

@sookmax
Copy link
Contributor

sookmax commented Mar 22, 2024

long_listbox_ex.mov

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:

combobox_longitem_story.mov

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

@LFDanLu
Copy link
Member

LFDanLu commented Mar 22, 2024

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

@staticshock
Copy link

staticshock commented Mar 22, 2024

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?

@LFDanLu
Copy link
Member

LFDanLu commented Mar 22, 2024

Adding isNonModal={false} is non-desirable here because it because it has accessibility implications due to

useLayoutEffect(() => {
if (state.isOpen && !isNonModal && popoverRef.current) {
return ariaHideOutside([popoverRef.current]);
}
}, [isNonModal, state.isOpen, popoverRef]);
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?

@staticshock
Copy link

2021 MacBook Pro w/M1 Pro chip

@sookmax
Copy link
Contributor

sookmax commented Mar 23, 2024

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

@staticshock
Copy link

Chrome 123.0.6312.59

@snowystinger
Copy link
Member

snowystinger commented Mar 24, 2024

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?

@staticshock
Copy link

staticshock commented Mar 24, 2024

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

@snowystinger
Copy link
Member

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.

@sookmax
Copy link
Contributor

sookmax commented Mar 25, 2024

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

@staticshock
Copy link

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
Copy link

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

@LFDanLu
Copy link
Member

LFDanLu commented Mar 25, 2024

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.

@psirenny
Copy link

psirenny commented Apr 25, 2024

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.

@LFDanLu
Copy link
Member

LFDanLu commented Apr 25, 2024

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 LFDanLu added the RAC label Apr 25, 2024
@staticshock
Copy link

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

@sookmax
Copy link
Contributor

sookmax commented May 3, 2024

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

Screen.Recording.2024-05-03.at.11.13.47.PM.mov

@snowystinger
Copy link
Member

I can reproduce now following instructions in #6322

@danielweck
Copy link

I can reproduce this 100% with Chromium/Chrome/Edge etc. but not with Firefox and Safari (all latest versions available, MacOS)

@danielweck
Copy link

Another data point: the long text MUST cause an offset of the first character (left hand side of the text clipped), otherwise the bug doesn't occur. In other words, if the text is longer than the input field but it is left-aligned (right hand side of the text clipped) then the bug doesn't occur.

@danielweck
Copy link

Another find: setting the isNonModal prop to true seems to fix the problem ... but of course we need to be aware of unintended consequences, notably with respect to accessibility (tab cycling for keyboard users continues to work as normal, but screen reader users might encounter issues with the inert background behind the pop over ... I haven't tested yet)

* Whether the popover is non-modal, i.e. elements outside the popover may be
* interacted with by assistive technologies.
*
* Most popovers should not use this option as it may negatively impact the screen
* reader experience. Only use with components such as combobox, which are designed
* to handle this situation carefully.
*/
isNonModal?: boolean,

  /**
   * Whether the popover is non-modal, i.e. elements outside the popover may be
   * interacted with by assistive technologies.
   *
   * Most popovers should not use this option as it may negatively impact the screen
   * reader experience. Only use with components such as combobox, which are designed
   * to handle this situation carefully.
   */
  isNonModal?: boolean,

@psirenny
Copy link

psirenny commented May 6, 2024

Another find: setting the isNonModal prop to true seems to fix the problem

The most frustrating unintended consequence (for us), is that it disables click-outside to dismiss.

@snowystinger snowystinger linked a pull request May 29, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 👀 In Review
Development

Successfully merging a pull request may close this issue.

10 participants