enable lazy auto-complete search and fallback to input while loading

[Gregoor]

Inspired by #3719

Motivation

Every kb in the main bundle slows down every kind of interaction (due to added fetching, parsing, evaluation delay). We can lazy-load the autocomplete component and render a static input as a fallback. Users can already start typing into it or even press enter to submit a full-search.
If the auto-complete loads while they were typing, their input and focus state gets used inside of it so that it gets seemlessly swapped out.

Demo

Screen.Recording.2021-05-17.at.11.41.49.mov

What it does

It moves the input and focus state setting out of the auto-complete and into the root search component, that way it can share that state either with the dumb-input or the autocomplete, whichever is available.

How to test

Make the search lazy load slow and start typing in the search field before it has loaded.

a patch for slowing down lazy load

Index: client/src/ui/molecules/search/index.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/client/src/ui/molecules/search/index.tsx b/client/src/ui/molecules/search/index.tsx
--- a/client/src/ui/molecules/search/index.tsx	(revision 1935fc5c89a69959d71cde75f94e738330ff9eeb)
+++ b/client/src/ui/molecules/search/index.tsx	(date 1621244137814)
@@ -7,7 +7,12 @@
 import "./index.scss";
 
 import "./basic-search-widget.scss";
-const LazySearchNavigateWidget = lazy(() => import("../../../search"));
+const LazySearchNavigateWidget = lazy(
+  () =>
+    new Promise((resolve) => {
+      setTimeout(() => resolve(import("../../../search") as any), 3000);
+    })
+);
 
 function useQueryParamState() {
   const [searchParams] = useSearchParams();

[Gregoor]

Okay now I made it a little more controversial: I added some localStorage caching for the flex index as it was quite slow for me, and given that we don't do client-side navigation I'd expected users would have to reindex quite a lot.

I'm not super happy yet with that async function inside of the persisting effect, so maybe that would benefit from another think.

[peterbe]

For the record, I think what you have here, apart from the localStorage, is a great experiment.
We chatted on Slack and I mentioned that the localStorage thing might not be a great idea based on the fact that the XHR request can be so nicely browser-cached. Besides, if you remove that portion, your code will be a great experimentation variant between always injecting it and not having it at all.

My early measurements with Lighthouse clearly indicates a 7% decrease to the Lighthouse Performance score (when having the auto-complete part of the main JS bundle). So it's definitely ripe for more experimentation and tuning.

[peterbe]

Also, for the record, can you push your branch to mdn and then kick off a Dev build with the prefix search-lazy.
Then, we'll be able to compare:

1. https://main.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Global_Objects
2. https://search.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Global_Objects
3. https://search-lazy.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Global_Objects

[Gregoor]

Peter and I chatted about trying localStorage separately (what it did was not cache the json, but rather the flex index, which might still be worth it).

I've also pushed the branch onto this repo so we can deploy it.

A TODO I still have on this is maintaining the correct selection when switching out the search widget. Currently it would just jump to the end of the inputValue when the components get switched out.

[Gregoor]

Here goes the build: https://github.com/mdn/yari/actions/runs/856570307

[Gregoor]

Alrighty, I added selection-retaining, though I have to admit that I could race-condition it into being a bit flaky. Have not found out yet why that is.

I also made two other changes:

• moved useFocusOnSlash out so that the BasicSearchWidget can also use it, in case people try to use the shortcut to focus search before it has loaded. Hence they also now share their placeholder which makes the swapping out more seemless
• I changed the "nothing found" text to No quick results found. Click <Link to={searchPath}>here</Link> or press enter to do a full-search to give people a clearer mental model of the two different kinds of searches (and the possibility that a full-text search might yet return a result).

[Gregoor]

Here goes the build: https://github.com/mdn/yari/actions/runs/856570307

turns out I only got the prefix right there, not the branch. Rerunning it now!

[Gregoor]

You were right @peterbe, it is a downshift bug. Maybe more of a "bug" than a bug, because it's the good old issue of semi-controlled inputs again, where the same state lives in multiple places. I won't bore you with details, but in the thread I found a workaround which seems to work. So there should be no more cases of mis-selection on this branch.
With that, I might want to switch to only loading the autocomplete after on focus (the idea you had in our meeting). Wdyt?

[Gregoor]

(I just added a commit that does only load it onFocus but I'm only 65% convinced it's the right thing)

[peterbe]

only load auto-complete on focus

I haven't actually looked at your code but I thought we kinda concluded that it's not worth doing. If you wait for onFocus (or onMouseOver) the risk is slightly increased that something can go wrong with the cursor or something like that. Your branch already has the same performance Score as the main version (which is amazing by the way!).

[peterbe]

Do you think you can look to delete whatever was typed after a navigate()?

I think the screencast is clear.
https://user-images.githubusercontent.com/26739/120822115-1662cb80-c524-11eb-8f98-5a072303ddd3.mov
I.e. it would be nice if it could remove "padding" after it has navigated to that page.

BUT, I quite like that it stays if I don't pick a title but instead hit Enter to (full site-)search:
https://user-images.githubusercontent.com/26739/120822377-57f37680-c524-11eb-951a-e140798567c0.mov

[peterbe]

By the way, I've tested it a bunch in BrowserStack. Works in Edge and Opera.
Curious thing, when I tested with Firefox 68 (version for no particular reason) it did not keep what I typed after picking a result as demonstrated in the screecast in the comment above this.

[peterbe]

This is quite insane.

We go from a score of 85 to 84. Even though all the fancy lazy loading. I guess it just goes to show, that every little bit of JS counts.

BUT I do not think this is a blocker. The autocomplete is a powerful feature that users will genuinely appreciate. We've worked hard on this for many months and a lot of people who have tested it already in the local preview server really really like it. So I'm definitely willing to accept the little marginal web perf hit.

[schalkneethling]

@Gregoor This is getting soooooo close. Two things I just noticed.

When you type something that the title search does not find a result for, you get this UI

Unlike with the results list, there is no way to get to that link using the keyboard arrow keys for example. When you press the tab key, it moves focus to the submit button but, pressing enter(or clicking the button) does not actually trigger a site search either.

[schalkneethling]

Also, we need to add this little piece of SCSS for the input button’s active state:

.search-button {
  ...
  &:active {
    background-color: transparent;
  }
}

Thank you!

[Gregoor]

Updated to main and all the points addressed, except for....

Do you think you can look to delete whatever was typed after a navigate()?

Interesting! I thought this was a bug in the previous implementation, that the search term got deleted. But now that I think it more maybe the correct behavior is: delete when an item is clicked, but keep when it's a navigation to the /search endpoint?

[peterbe]

But now that I think it more maybe the correct behavior is: delete when an item is clicked, but keep when it's a navigation to the /search endpoint?

RIght. Agreed. Leave it on onSubmit and clear it on item.onClick :)

[peterbe]

I think you switched from navigate to a regular hard redirect with location.href.
If we're going to do this you'll need to change what URL gets preloaded. I.e. not the /index.json URL.
But I quite liked the client-side navigation myself. It's definitely snappier and you don't have the potential problem of having to scroll up.

[peterbe]

I think you switched from navigate to a regular hard redirect with location.href.
If we're going to do this you'll need to change what URL gets preloaded. I.e. not the /index.json URL.
But I quite liked the client-side navigation myself. It's definitely snappier and you don't have the potential problem of having to scroll up.

Bah. It's actually plenty fast as is even without navigate(). Sure, it's less fast with a full redirect but it's pretty darn good enough. But I do love, when using the localhost:5000 build and type slicing and then click the "Site search for 'slicing'" how it just more or less instantly loads the site-search. Even with throttled network in devtools.

[Gregoor]

Yeah I don't actually know why I went ahead with replacing navigate(). We did previously come to the conclusion that client-side-navigation is lightweight enough that we don't have to start phasing it out. So I just reverted that change and reset the input manually instead.

[peterbe]

We chatted about this at great length and we've concluded that the iOS Safari bug is real and it's blocking.
There are lots of stack overflow and blog posts about hacking around this limitation. Basically iOS Safari doesn't allow async triggering of .focus(). I.e. we can't trigger a synthetic focus event unless it's via a sync click event handle.

Here are some interesting mentions of solutions:

• https://stackoverflow.com/questions/12204571/mobile-safari-javascript-focus-method-on-inputfield-only-works-with-click
• Allow autofocus to work inside <amp-lightbox> on iOS ampproject/amphtml#14676
• https://gist.github.com/cathyxz/73739c1bdea7d7011abb236541dc9aaa

Unfortunately, we have not been able to simulate the bug in any other way other than to test on an actual iPhone which is complicating things because you need to use desktop Safari to get access to web console logging from within the phone.

One curious thing we discovered was that:

  useEffect(() => {
    if (isFocused && inputRef.current) {
      console.log("FORCING FOCUS");
      inputRef.current.focus();
    }
  }, [isFocused]);

Did not work, but:

  useEffect(() => {
    if (isFocused && inputRef.current) {
      console.log("FORCING FOCUS");
      inputRef.current.focus();
      alert("FORCED FOCUS"); // THIS silly debugging hack
    }
  }, [isFocused]);

Perhaps the next action is to go through some of the hacks mentioned on that stack overflow post and see if it can be made to work with confidence.

[peterbe]

Because there's no other great place to discuss this; we should still consider to rewrite the whole thing. The reason we're doing these lazy on-demand loading is because downshift and Flexsearch and the XHR to /en-US/search.json are so slow that they would block TTI and other web perf measures.
The alternative would be to ditch client-side auto-complete and instead build something bespoke, or find a lightweight alternative to downshift, and instead do autocomplete XHR. Similar to https://www.peterbe.com/plog/blogitem-040601-1
Perhaps /api/v1/search?q=js&locale=en-US&autocomplete=1. To make that work, on the backend we'd need to index to Elasticsearch an Autocomplete field on the title etc.
The disadvantage with a solution like that would be that it would need to be intercepted in local development (e.g. localhost:5000 for yarn start in mdn/content)

[Gregoor]

Thanks for the summary Peter! I also want to mention the dirty-quick-fix: For mobile Safari we could start immediately loading the FancySearchWidget thereby making the chance of encountering this as small as you network is fast (and your fingers, you have to open a hamburger menu first). Imo this is not a terrible option, since TTI is still better than not lazy-loading it and data-restricted users tend not to be on iPhones.

[peterbe]

Thanks for the summary Peter! I also want to mention the dirty-quick-fix: For mobile Safari we could start immediately loading the FancySearchWidget thereby making the chance of encountering this as small as you network is fast (and your fingers, you have to open a hamburger menu first). Imo this is not a terrible option, since TTI is still better than not lazy-loading it and data-restricted users tend not to be on iPhones.

I like this a lot. It's less of a hack and more of a smart solution.
If you're on a responsive screen and you click the hamburger menu to expand the chances are huge that you're doing that because you're going to want to do a search and if you're going to do a search it's only a good thing that we can start the downloading stuff earlier anyway.
Yeah, the bug can still manifest itself but the chances will be vastly reduced.

Would you mind taking a quick look at the merge conflict we have now, before you leave, so the team after you (Florian, Schalk, myself) can give that one a shot in the next couple of weeks?

[schalkneethling]

Agreed, sounds like a reasonable assumption and solution. Could we merge this behind a flag and then work on the change in main?

[peterbe]

Agreed, sounds like a reasonable assumption and solution. Could we merge this behind a flag and then work on the change in main?

It's already behind a feature flag. In a sense. But this PR would change that a bit. I'd rather make the PR "good" before we consider merging it.

[peterbe]

Let's do it!!