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 13 commits
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
15 changes: 15 additions & 0 deletions cypress/integration/search/actions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
describe('Start', () => {
beforeEach(() => {
cy.visit(Cypress.config().baseUrl!);
// seems we could run into race condition
// if we don't wait for .DocSearch-Button-Key to render
cy.get('.DocSearch-Button-Key');
shortcuts marked this conversation as resolved.
Show resolved Hide resolved
});

it('Open modal on search button click', () => {
Expand Down Expand Up @@ -147,4 +150,16 @@ describe('Recent and Favorites', () => {
.trigger('click');
cy.contains('No recent searches').should('be.visible');
});

describe('A11y', () => {
beforeEach(() => {
cy.visit(Cypress.config().baseUrl!);
});
it('Restore focus to stored document.activeElement before modal is open', () => {
cy.get('[data-testid="btn"]').focus();
cy.get('body').type('{ctrl}k');
cy.closeModal();
cy.focused().should('have.data', 'testid', 'btn');
});
});
});
Binary file added examples/cypress/favicon.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions examples/cypress/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />

<link rel="shortcut icon" href="favicon.png" type="image/x-icon" />

<title>DocSearch v3 - React</title>
</head>

<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>

<script type="module" src="src/index.js"></script>
</body>
</html>
20 changes: 20 additions & 0 deletions examples/cypress/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"name": "@docsearch/cypress",
"description": "DocSearch v3 Cypress testing",
"version": "3.3.1",
"private": true,
"license": "MIT",
"scripts": {
"build": "parcel build index.html",
"start": "parcel index.html --port 3000 --no-cache"
},
"dependencies": {
"@docsearch/css": "3.3.1",
"@docsearch/react": "3.3.1",
"react": "^18.1.0",
"react-dom": "^18.1.0"
},
"devDependencies": {
"parcel": "2.7.0"
}
}
32 changes: 32 additions & 0 deletions examples/cypress/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';
import { DocSearch } from '@docsearch/react';

import './app.css';
import '@docsearch/css';

function App() {
return (
<div>
<h1>DocSearch v3 - React</h1>
<button data-testid="btn">A button</button>
<DocSearch
indexName="docsearch"
appId="R2IYF7ETH7"
apiKey="599cec31baffa4868cae4e79f180729b"
transformItems={
(items) => { // transform absolute url into relative ones to solve CORS problem in cypress
return items.map((item) => {
const { origin } = new URL(item.url);
return {
...item,
url: item.url.replace(new RegExp(`^${origin}`), ''),
};
})
}
}
/>
</div>
);
}

export default App;
4 changes: 4 additions & 0 deletions examples/cypress/src/app.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
body {
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica,
Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';
}
7 changes: 7 additions & 0 deletions examples/cypress/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import React from 'react';
import { createRoot } from 'react-dom/client';

import App from './App';

const root = createRoot(document.getElementById('root'));
root.render(<App />);
2 changes: 1 addition & 1 deletion examples/demo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"license": "MIT",
"scripts": {
"build": "parcel build index.html",
"start": "parcel index.html"
"start": "parcel index.html --port 3000 --no-cache"
shortcuts marked this conversation as resolved.
Show resolved Hide resolved
},
"dependencies": {
"@docsearch/css": "3.3.1",
Expand Down
2 changes: 1 addition & 1 deletion examples/js-demo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"license": "MIT",
"scripts": {
"build": "parcel build index.html",
"start": "parcel index.html"
"start": "parcel index.html --port 3000 --no-cache"
},
"dependencies": {
"@docsearch/css": "3.3.1",
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
"cy:run:chrome": "yarn run cy:run --browser chrome",
"cy:run:edge": "yarn run cy:run --browser edge",
"cy:run:firefox": "yarn run cy:run --browser firefox",
"cy:run": "start-server-and-test 'yarn website:test' http://localhost:3000 'cypress run --spec 'cypress/integration/**/*' --headless'",
"cy:run": "start-server-and-test 'yarn playground:cypress' http://localhost:3000 'cypress run --spec 'cypress/integration/**/*' --headless'",
"cy:verify": "cypress verify",
"lint:css": "stylelint **/src/**/*.css",
"lint": "eslint --ext .js,.ts,.tsx .",
"playground:cypress": "yarn workspace @docsearch/cypress start",
"playground:start": "yarn workspace @docsearch/react-example start",
"playground-js:start": "yarn workspace @docsearch/js-example start",
"release": "shipjs prepare",
Expand Down
16 changes: 14 additions & 2 deletions packages/docsearch-react/src/DocSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type {
} from '@algolia/autocomplete-core';
import type { SearchOptions } from '@algolia/client-search';
import type { SearchClient } from 'algoliasearch/lite';
import React from 'react';
import React, { useEffect } from 'react';
chenxsan marked this conversation as resolved.
Show resolved Hide resolved
import { createPortal } from 'react-dom';

import { DocSearchButton } from './DocSearchButton';
Expand Down Expand Up @@ -42,24 +42,36 @@ 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(() => {
setIsOpen(false);
}, [setIsOpen]);

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

const onInput = React.useCallback(
(event: KeyboardEvent) => {
setIsOpen(true);
Expand Down