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

Search modal closes when clicking the reset button #971

Closed
sarahdayan opened this issue May 16, 2022 · 3 comments · Fixed by #987
Closed

Search modal closes when clicking the reset button #971

sarahdayan opened this issue May 16, 2022 · 3 comments · Fixed by #987
Assignees

Comments

@sarahdayan
Copy link
Member

Description

In detached mode, even when openOnFocus=true, clearing the search input with the reset button closes the modal.

detached-desktop.mov

It also happens on desktop but the input receives the focus back, triggering the onFocus logic, so the panel reopens.

desktop-flicker.mov

In detached mode, this doesn't happen because the closing modal destroys the entire form, so there's no search input to focus.

Root cause and possible solution

Clicking the reset button triggers the blur event, which triggers logic that closes the search modal. This doesn't happen on touch devices because of some guard logic, but this logic isn't present on non-touch devices.

Having the same logic in both handlers should fix the issue, and avoid the close/reopen behavior on desktop.

Reproduction

Preview →

Steps

  1. Go to https://nt0gqu.csb.app/ on desktop
  2. Click on the search button
  3. Type a query (e.g., "apple") and wait for results
  4. Clear the query by clicking the reset button
  5. See the modal closing

To compare, you can reproduce either on mobile or on desktop with a simulated touch device. Doing the same steps doesn't close the modal.

detached-touch.mov

Expected behavior

The modal shouldn't close when hitting the reset button on a detached autocomplete with openOnFocus=true.

Additionally, it shouldn't "flicker" (close then reopen) when not detached. Instead, it should stay open.

Environment

  • OS: macOS Monterey 12.3.1
  • Browser: Firefox 100, Safari mobile
  • Autocomplete version: 1.6.3
@sarahdayan
Copy link
Member Author

Here's a breakdown of what is going on and some leads to get it fixed.

We have two ways of handling blur events on the search input:

  • On pointer devices, we listen to the onblur event on the search input
  • On touch devices, we delegate to the environment's mousedown event

This was initially designed to avoid closing the modal on blur on mobile devices: when users start scrolling with their finger (touchmove), so the search input is blurred. The onMouseDown handler is defined in environment props, which have access to every element (input, form, and panel) so it's easy to conditionally dispatch a blur event to the state reducer based on whether or not the tap was done within the autocomplete or not.

The logic isn't repeated in the onBlur handler of the search input:

  • This was likely missed, detached mode being primarily designed for mobile
  • The input props don't have access to the form or panel

The onBlur handler simply dispatch a blur event to the state reducer, which sets isOpen to false, destroying the modal before any other events bubble.

Because we can have detached mode on pointer devices, we need to handle the case just like we do on touch devices.

One solution would be to provide the form element to getInputProps and repeat the logic. This isn't trivial though, because the form props require access to the input. Also, this would force us to change prop getters types in autocomplete-core, which would likely be breaking for users who built a custom renderer.

Another solution is to avoid relying on blur on both pointer and touch devices, and instead set up an onMouseDown handler in the environment props that execute the same logic as onTouchStart, but for pointer devices.

This works, but breaks some tests:

  • Some can be deleted because they test the blur event directly
  • Hitting Enter without having any selected item should be handled—it normally triggers blur but if this is no longer handled, the logic there needs to go somewhere else.
  • Some tests are still failing but I didn't yet get a chance to understand why.

@absemetov
Copy link

Hey @sarahdayan !
Please check it out 636
Gotta fix this problem with Detached mode!

@andreyvolokitin
Copy link

The proposed solution of having uniform logic regardless of input type is very relevant, as touch might actually be not possible to detect reliably

(I also immediately used detached mode at desktop and never considered it a mobile-only thing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment