feat: restore focus after closing modal

[chenxsan]

Related issue #1370

cc @patrickhlauke if you don't mind please help review this one.

Basically the code would store the document.activeElement when modal is opened, and call focus on that stored document.activeElement when modal is closed.

I've added a button to the examples so we can test if the focus can be retained.

[netlify]

✅ Deploy Preview for docsearch canceled.

Name	Link
🔨 Latest commit	715e120
🔍 Latest deploy log	https://app.netlify.com/sites/docsearch/deploys/63aefd2b2e5af50008198490

[patrickhlauke]

is there an easy way for me to check this live without having to work out how to run it locally?

from looking over the code itself, this looks correct though.

[shortcuts]

Thanks a lot for the PR!

is there an easy way for me to check this live without having to work out how to run it locally?

For a concrete test you'd have to run de playground locally, it should only require installing deps and run yarn playground:start

[shortcuts]

Thanks for tackling this, would you mind adding a cypress test for this case?

[chenxsan]

Thanks a lot for the PR!

is there an easy way for me to check this live without having to work out how to run it locally?

For a concrete test you'd have to run de playground locally, it should only require installing deps and run yarn playground:start

I can see there's a netlify deploy preview, but it's canceled, any chance to make it work so anyone can test it online instead of running it locally?

[chenxsan]

Thanks for tackling this, would you mind adding a cypress test for this case?

Sure, I'll add one.

[shortcuts]

Thanks a lot for the PR!

is there an easy way for me to check this live without having to work out how to run it locally?

For a concrete test you'd have to run de playground locally, it should only require installing deps and run yarn playground:start

I can see there's a netlify deploy preview, but it's canceled, any chance to make it work so anyone can test it online instead of running it locally?

You can remove the ignore build command of the netlify.toml file, but I don't know if we have a button to re-focus on the website.

[chenxsan]

Converted into draft until I figure out the cypress test here.

[chenxsan]

@shortcuts Seems we can't test this feature against the website as docusaurus customizes its search function based on DocSearch per https://github.com/facebook/docusaurus/blob/0985fa0af338068b4906f409a3f874a319f5359c/packages/docusaurus-theme-search-algolia/src/theme/SearchBar/index.tsx#L148.

Hence I would suggest running cypress against the example site, either react-example or js-example should be ok. Testing against a customized search function based on DocSearch where we have no control does't make sense to me.

[chenxsan]

is there an easy way for me to check this live without having to work out how to run it locally?

from looking over the code itself, this looks correct though.

FYI, if you're going to run locally, you would need Node 16. 18 won't work as far as I see. Here's how you would run the project locally:

1. clone
2. run yarn
3. run yarn build
4. run yarn playground:start

[shortcuts]

Hence I would suggest running cypress against the example site, either react-example or js-example should be ok. Testing against a customized search function based on DocSearch where we have no control does't make sense to me.

Makes a lot of sense indeed! Changing the website:test here https://github.com/algolia/docsearch/blob/main/package.json#L24 with playground:start should solve this, no?

[shortcuts]

(again thanks a lot for the time spent here!)

[chenxsan]

(again thanks a lot for the time spent here!)

It's my pleasure to improve a project I use every day :)

I think it's ready for review now, I've added some comments to explain why I made such change.

[shortcuts]

Looks great! Thanks a lot

[shortcuts]

last comment!

[chenxsan]

Ooops, CI failed due to https://support.circleci.com/hc/en-us/articles/115014359648-Exit-Code-137-Out-of-Memory

[shortcuts]

Ooops, CI failed due to https://support.circleci.com/hc/en-us/articles/115014359648-Exit-Code-137-Out-of-Memory

oops D: you can update https://github.com/algolia/docsearch/blob/main/package.json#L18 to prevent building the playground, we don't need it on the CI anyway

I think adding --ignore @docsearch/(cypress|react-example|js-example) should do it

[francoischalifour]

Thank you @chenxsan!

[francoischalifour]

Is this working in server environments where document isn't defined?

[chenxsan]

Interesting, I've never thought of rendering DocSearch in server side.

I guess it won't work.

Any suggestion?

[chenxsan]

How about this:

I've reran the tests locally, seems fine, though we need to apply type assertion for the initial object.

[francoischalifour]

Isn't the effect covered by the onOpen() callback?

I think we better have activeElementRef accept null as initial value, and update its value in the onOpen() and onClose() callbacks. This way, the effect can also be removed, and there's no document in the initial render path.

 export function DocSearch(props: DocSearchProps) {
   const searchButtonRef = React.useRef<HTMLButtonElement>(null);
+  const activeElementRef = React.useRef<Element | null>(null);
   const [isOpen, setIsOpen] = React.useState(false);
   const [initialQuery, setInitialQuery] = React.useState<string | undefined>(
     props?.initialQuery || undefined
   );

   const onOpen = React.useCallback(() => {
     setIsOpen(true);
+    activeElementRef.current = document.activeElement || document.body;
   }, [setIsOpen]);

   const onClose = React.useCallback(() => {
     setIsOpen(false);
+
+    if (activeElementRef.current) {
+      activeElementRef.current.focus();
+      activeElementRef.current = null;
+    }
   }, [setIsOpen]);

-  React.useEffect(() => {
-    if (
-      !isOpen &&
-      activeElementRef.current &&
-      activeElementRef.current instanceof HTMLElement
-    ) {
-      activeElementRef.current.focus();
-    }
-  }, [isOpen]);

   // ...
 }

[chenxsan]

Unfortunately this won't work,

   const onClose = React.useCallback(() => {
     setIsOpen(false);
+
+    if (activeElementRef.current) {
+      activeElementRef.current.focus();
+      activeElementRef.current = null;
+    }
   }, [setIsOpen]);

I believe I tried this in the beginning, and it would cause two tests to fail:

I.e., clicking the button to open dialog, then invoking shortcut to close the dialog would fail. I haven't yet figured out what could be wrong. Guess operating DOM right after calling setIsOpen(false) doesn't fit well with React. Introducing another useEffect would let React schedule the jobs.