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.

Sign up for GitHub

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

feat: restore focus after closing modal #1735

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
+115 −5
Open

feat: restore focus after closing modal #1735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4a97117
feat: restore focus after closing modal
chenxsan Dec 28, 2022
4f7e72a
add e2e test
chenxsan Dec 29, 2022
730f271
log
chenxsan Dec 29, 2022
abf6724
Revert "log"
chenxsan Dec 29, 2022
1f293a3
test again @docsearch/js-example
chenxsan Dec 30, 2022
09a3981
use port 3000
chenxsan Dec 30, 2022
ef95b7e
transform search results
chenxsan Dec 30, 2022
fd179ad
revert import
chenxsan Dec 30, 2022
66737a8
fix tests
chenxsan Dec 30, 2022
4b5fb04
fix tests
chenxsan Dec 30, 2022
094680d
apply reviews
chenxsan Dec 30, 2022
04fe620
revert style change
chenxsan Dec 30, 2022
c2340ea
revert change
chenxsan Dec 30, 2022
7f986a6
force ci
chenxsan Dec 30, 2022
81de7e8
use React.useEffect
chenxsan Dec 30, 2022
2e89569
ignore playground
chenxsan Dec 30, 2022
be4575f
apply review
chenxsan Dec 30, 2022
715e120
apply review
chenxsan Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/demo/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ function App() {
return (
<div>
<h1>DocSearch v3 - React</h1>
<button>A button</button>
<DocSearch
indexName="docsearch"
appId="R2IYF7ETH7"
Expand Down
2 changes: 1 addition & 1 deletion examples/js-demo/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<body>
<div class="container">
<h1>DocSearch v3 - Vanilla JavaScript</h1>

<button>A button</button>
<div id="docsearch"></div>
</div>

Expand Down
9 changes: 8 additions & 1 deletion packages/docsearch-react/src/DocSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,28 @@ export interface DocSearchProps {
initialQuery?: string;
navigator?: AutocompleteOptions<InternalDocSearchHit>['navigator'];
translations?: DocSearchTranslations;
getMissingResultsUrl?: ({ query: string }) => string;
getMissingResultsUrl?: ({ query }: { query: string }) => string;
shortcuts marked this conversation as resolved.
Show resolved Hide resolved
}

export function DocSearch(props: DocSearchProps) {
const searchButtonRef = React.useRef<HTMLButtonElement>(null);
const activeElementRef = React.useRef<Element>(document.body);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I guess it won't work.

Any suggestion?

Copy link
Contributor Author

@chenxsan chenxsan Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

   // ...
 }

Copy link
Contributor Author

@chenxsan chenxsan Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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(() => {
if (activeElementRef.current) {
if (activeElementRef.current instanceof HTMLElement) {
activeElementRef.current.focus();
}
}
setIsOpen(false);
}, [setIsOpen]);

Expand Down