[Modal] Tab key press causes scroll change

[mctep]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Change focused element through [Shift +] Tab should not change scroll position when some Modal is open by default.

Current Behavior 😯

Focusing through keyboard on whole page is active when there is any link on page.

Steps to Reproduce 🕹

On the Dialog Demo open Simple Dialog and press Shift + Tab. Page is scrolled to end.

On https://codesandbox.io/s/0y8n3r593v scroll to green button and press it. Press Tab three times or click to browser's url and then press Tab. Page is scrolled to start.

Context 🔦

Looks like <a> elements cause the problem.

Your Environment 🌎

Tech	Version
Material-UI	v3.5.1
React	16.4.1
Browser	Chrome/Firefox
TypeScript	n/a
etc.

[oliviertassinari]

I could reproduce the same issue on Bootstrap. Maybe related to twbs/bootstrap#21661.

[eps1lon]

I recently looked into that and recognized that react-bootstrap had the same issue too.

The official authoring practices trap it by putting up a tabIndex=0 wall around the modal content and let the browser pay for it.

I think we can fix this in one PR that implements dialogs according to the authoring practices (proper roles + focus trap).

[oliviertassinari]

@eps1lon I like how the official authoring practices and the antd version uses two sentinels DOM nodes to solve the problem. Gestalt has an interesting focus logic too.
The official authoring logic: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/js/dialog.js.

[mctep]

@oliviertassinari Looks like that recent PR is not enough to fix the issue for all cases.

If you

1. open the official authoring practices example,
2. make some scroll to hide the first links,
3. open the Dialog,
4. focus to address (URL) window bar,
5. and press Tab button

scroll position will be changed in the result.

For Material UI it is not reproduced because in the most cases we have a fixed AppBar in the page beginning with some focusable elements in it. And browser does not scroll this elements into the viewport on focus.

I could suggest to place also two sentinel fixed hidden buttons (or two divs with tabIndex="0") in the body when any modal is open through the ModalManager together with hiding scrollbar. So when user focuses on the window the first or the last focusable elements (through Shift+Tab) will be these sentinels and they will always be in the viewport. What do you think? I could try to make PR.

[eps1lon]

I could suggest to place also two sentinel fixed hidden buttons (or two divs with tabIndex="0") in the body when any modal is open through the ModalManager together with hiding scrollbar.

Are the authoring practices doing this as well? We essentially copied their strategy and place the sentinels around the dialog body not the document body.

Have you tried this on https://next.material-ui.com?

[mctep]

No, the problem is not with TrapFocus. The original issue is fixed well. The authoring practices does not take in account change focus from the browser address bar.

Have you tried this on https://next.material-ui.com?

Try to remove all fixed elements (header and two navs elements) through the Developer tools, close developer tools and change focus from browser address bar:

[eps1lon]

No, the problem is not with TrapFocus. The original issue is fixed well.

Please open a new issue and fill out the template. Especially something without removing elements with the devtools. If you break the page I would expect behavior to break as well.