fix(autocomplete-js): leave the modal open on reset on pointer devices #987

sarahdayan · 2022-06-16T14:47:03Z

This PR fixes a bug in detached mode where clicking the reset button would also close the modal, even with openOnFocus=true. This bug appeared only on pointer devices, not on touch devices.

Preview

Here are a gew scenarios that this PR fixes. You can test it out in this sandbox. To compare, here's the same sandbox without the fix.

Before	After
detached-desktop.mov The modal closes on reset.	detached-desktop.mov The modal stays open on reset.
desktop-flicker.mov The panel "flickers": it closes then reopens on focus.	desktop-no-flicker.mov The panel stays open, there's no flicker.
detached-touch.mov The modal stays open in detached mode on a touch device	detached-touch.mov The modal still stays open in detached mode on a touch device

Root cause

Clicking the reset button triggers the DOM blur event on the <input>. Before this fix, we had an onBlur handler that triggered logic that closed the search modal. This didn't happen on touch devices because of some guard logic that only applied to touch devices.

This is a problem, because detached mode isn't touch-specific: it's the default mode on screens below 500px, and users can enable it with manually.

More context in the issue →

Solution

This PR fixes the problem by no longer relying on the blur input event, and instead using the mousedown event at the environment level. This is a similar approach to what we're doing with the touchstart event, but for pointer devices. Both handlers reuse the same logic, and are colocated.

The blur event isn't only triggered by "clicks" or "touches" outside of the focused element, it also happens when we hit Enter or Escape—the difference being that these are intentional blurs that we don't need to guard. The relevant part of the onBlur handler for these events (canceling all pending requests) was added to these event's handlers, in onKeyDown.

The rationale behind choosing this path is the following:

We should avoid conditionals as much as we can on large pieces of logic, especially for core behaviors.
The current logic needs access to the formElement and panelElement. Keeping the onBlur handler would mean making these elements available in getInputProps, which would be complicated, and a breaking change.
It's easier to respond to direct events (e.g., reset, keydown, etc.) than consequential events, particularly because we know exactly what caused them. Tracing the origin on a blur isn't always possible.

Output

This PR fixes the issue, and solves other problems in the process:

When resetting the form, but you're not in detached mode, you no longer have a flash of the panel closing then reopening (see preview at the top).
Triggering Escape in detached mode on touch devices was still vulnerable to stale promises due to some outdated logic that was removed in this PR. This was an unlikely scenario anyway, since you don't use Escape on most mobile and tablet devices. Still, it could technically happen on some computers with touch screens.

Next steps

We might not have enough high-level tests, and it's difficult to cover all scenarios on all devices. In the future, it would be highly beneficial to be able to write high-level test suites and run them on multiple devices. We could probably use our BrowserStack account for this.
We should ensure our behavioral tests interact with the library like a user would do—users don't call blur() on an input, they click on things or press keys. They don't call reset() on a form, they click a reset button, etc. We do this irregularly in our tests, which causes false negatives and positives. We should do a pass to straighten this out.

fixes #971

…ually

codesandbox-ci · 2022-06-16T14:49:00Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 22ff478:

Sandbox	Source
clever-violet-hxw44i	Issue #971

sarahdayan · 2022-06-17T07:36:08Z

packages/autocomplete-js/src/elements/Input.ts

-      if (onDetachedEscape && event.key === 'Escape') {
-        event.preventDefault();
-        onDetachedEscape();
-        return;
-      }


Same as https://github.com/algolia/autocomplete/pull/987/files#r899847862.

sarahdayan · 2022-06-17T07:36:33Z

packages/autocomplete-js/src/createAutocompleteDom.ts

-    onDetachedEscape: isDetached
-      ? () => {
-          autocomplete.setIsOpen(false);
-          setIsModalOpen(false);
-        }
-      : undefined,


This was no longer needed because the logic in onDetachedEscape has been covered by this since #556.

This is likely stale code from the first Autocomplete Touch (now Detached Mode) PR.

sarahdayan · 2022-06-17T07:39:00Z

packages/autocomplete-js/src/__tests__/detached.test.ts

+    await waitForElementToBeRemoved(
+      document.querySelector('.aa-DetachedOverlay')
+    ).catch(() => {});


We want to check that the modal stays open, but the following assertions in waitFor will always pass since Testing Library just waits for this scenario to hold true. If the modal was later closing, the test would still pass, so we need to also make sure the element is never removed.

I couldn't find a cleaner way to do it, I'm open to suggestions.

sarahdayan · 2022-06-17T07:41:58Z

packages/autocomplete-core/src/__tests__/getInputProps.test.ts

@@ -1893,81 +1918,6 @@ describe('getInputProps', () => {
    });
  });

-  describe('onBlur', () => {


The test cases from this sub-suite are now split across multiple tests in the onKeyDown sub-suite and the getEnvironmentProps suite.

dhayab

👏

packages/autocomplete-core/src/getPropGetters.ts

Haroenv · 2022-06-20T11:11:44Z

packages/autocomplete-core/src/onKeyDown.ts

+      // If requests are still pending when the panel closes, they could reopen
+      // the panel once they resolve.
+      // We want to prevent any subsequent query from reopening the panel
+      // because it would result in an unsolicited UI behavior.


maybe here it should be qualified that the requests are still happening, but the handlers ignored, what do you think?

Since we call cancelAll in multiple places, do you think it would make more sense to comment the method directly?

I'll do this in a separate PR to keep this one focused.

francoischalifour

Great refactoring! I suspect some more dead code because this behavior was the original nature of Autocomplete.

There's one behavioral change that I think we shouldn't do: hitting Tab on the input doesn't close the panel anymore. Can you think of a way to support this?

Also, a future possible improvement is to make the results keyboard navigable in Detached Mode. This is related to focus trap.

sarahdayan · 2022-06-20T22:07:04Z

@francoischalifour Do you think hitting Tab should close the panel? This seems odd to me, I expect it to circle through focusable elements like it does right now, this seems more accessible.

francoischalifour · 2022-06-21T08:56:38Z

@sarahdayan This is the behavior Google, YouTube, Amazon, etc. are doing. So I think this is desirable.

sarahdayan · 2022-06-21T09:10:32Z

@francoischalifour It's the case on YouTube and Google indeed, but not on Amazon. Still, the combobox spec examples seem to abide by this: https://w3c.github.io/aria-practices/examples/combobox/combobox-select-only.html

Should we do this only in dropdown mode so we give ourselves the possibility to implement focus trap later on in detached mode?

francoischalifour · 2022-06-21T09:11:54Z

Should we do this only in dropdown mode so we give ourselves the possibility to implement focus trap later on in detached mode?

Yes, only in dropdown mode.

packages/autocomplete-js/src/createAutocompleteDom.ts

sarahdayan added 6 commits June 15, 2022 14:43

test: perform actual user action instead of triggering blur event man…

6b9667b

…ually

test: make concurrency tests more exhaustive and precise

3257bc5

fix: move blur handling to environment mousedown event

cb71f9c

feat: expose onMouseDown on GetEnvironmentProps

157424a

test: test clearing in detached mode scenarios

f236fe0

fix: avoid triggering custom logic on escape on detached mode

ac73f63

sarahdayan commented Jun 17, 2022

View reviewed changes

sarahdayan added 3 commits June 17, 2022 09:43

refactor: no need to inline

77bb200

chore: move comment where more relevant

c5002da

test: explicitly set parameter

fa20108

sarahdayan marked this pull request as ready for review June 17, 2022 08:20

sarahdayan requested review from francoischalifour, dhayab and Haroenv June 17, 2022 08:20

dhayab approved these changes Jun 17, 2022

View reviewed changes

packages/autocomplete-core/src/getPropGetters.ts Show resolved Hide resolved

Haroenv approved these changes Jun 20, 2022

View reviewed changes

francoischalifour reviewed Jun 20, 2022

View reviewed changes

sarahdayan added 2 commits June 21, 2022 11:23

test: assume element exists

80b799a

feat: trigger a blur when hitting Tab or Shift+Tab

4b80cc1

sarahdayan requested a review from francoischalifour June 21, 2022 12:42

francoischalifour reviewed Jun 21, 2022

View reviewed changes

packages/autocomplete-js/src/createAutocompleteDom.ts Outdated Show resolved Hide resolved

francoischalifour reviewed Jun 21, 2022

View reviewed changes

packages/autocomplete-js/src/createAutocompleteDom.ts Outdated Show resolved Hide resolved

refactor: forward detached status to input component

22ff478

sarahdayan requested a review from francoischalifour June 21, 2022 13:16

francoischalifour approved these changes Jun 21, 2022

View reviewed changes

sarahdayan merged commit 3e387e6 into next Jun 21, 2022

sarahdayan deleted the fix/detached-reset branch June 21, 2022 13:37

sarahdayan mentioned this pull request Jun 21, 2022

docs: add TSDoc for cancelable promise list methods #988

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(autocomplete-js): leave the modal open on reset on pointer devices #987

fix(autocomplete-js): leave the modal open on reset on pointer devices #987

sarahdayan commented Jun 16, 2022 •

edited

Loading

codesandbox-ci bot commented Jun 16, 2022 •

edited

Loading

sarahdayan Jun 17, 2022 •

edited

Loading

sarahdayan Jun 17, 2022

sarahdayan Jun 17, 2022

sarahdayan Jun 17, 2022

dhayab left a comment

Haroenv Jun 20, 2022

sarahdayan Jun 20, 2022

sarahdayan Jun 21, 2022

francoischalifour left a comment •

edited

Loading

sarahdayan commented Jun 20, 2022

francoischalifour commented Jun 21, 2022

sarahdayan commented Jun 21, 2022

francoischalifour commented Jun 21, 2022

fix(autocomplete-js): leave the modal open on reset on pointer devices #987

fix(autocomplete-js): leave the modal open on reset on pointer devices #987

Conversation

sarahdayan commented Jun 16, 2022 • edited Loading

Preview

Root cause

Solution

Output

Next steps

codesandbox-ci bot commented Jun 16, 2022 • edited Loading

sarahdayan Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

sarahdayan Jun 17, 2022

Choose a reason for hiding this comment

sarahdayan Jun 17, 2022

Choose a reason for hiding this comment

sarahdayan Jun 17, 2022

Choose a reason for hiding this comment

dhayab left a comment

Choose a reason for hiding this comment

Haroenv Jun 20, 2022

Choose a reason for hiding this comment

sarahdayan Jun 20, 2022

Choose a reason for hiding this comment

sarahdayan Jun 21, 2022

Choose a reason for hiding this comment

francoischalifour left a comment • edited Loading

Choose a reason for hiding this comment

sarahdayan commented Jun 20, 2022

francoischalifour commented Jun 21, 2022

sarahdayan commented Jun 21, 2022

francoischalifour commented Jun 21, 2022

sarahdayan commented Jun 16, 2022 •

edited

Loading

codesandbox-ci bot commented Jun 16, 2022 •

edited

Loading

sarahdayan Jun 17, 2022 •

edited

Loading

francoischalifour left a comment •

edited

Loading