[fixed] Focus trap when reentering document (#742) #791

holloway · 2019-12-06T00:53:38Z

Fixes #742

Changes proposed:

Ensure modal focus trap when reentering document. This restores modal focus when navigating into the page by keyboard. See discussion on Modal doesn't trap focus when re-entering document #742 and Accessibility: focus trap improvements to avoid accessing underlying page #787

Upgrade Path (for changed or removed APIs):

N/A

Acceptance Checklist:

The commit message follows the guidelines in CONTRIBUTING.md.
~~[ ] Documentation (README.md) and examples have been updated as needed.~~ N/A I think
If this is a code change, a spec testing the functionality has been added.
~~[ ] If the commit message has [changed] or [removed], there is an upgrade path above.~~ N/A I think

diasbruno · 2019-12-07T15:13:21Z

Legit. @holloway Have you found what broke the tests?

diasbruno · 2019-12-07T15:16:37Z

@holloway Ping me if you need anything...not sure if I'm receiving the notifications from this repo or if I'm skipping without looking... :)

coveralls · 2019-12-08T05:47:29Z

Coverage increased (+0.8%) to 87.135% when pulling 30b406e on springload:fix/modal-body-focus into 98dd5be on reactjs:master.

holloway · 2019-12-08T07:51:27Z

@diasbruno Yeah the test bug was caused by making those trap elements in componentDidMount rather than the constructor (or, presumably, willMount).

So that's passing now.

src/components/ModalPortal.js

diasbruno · 2019-12-09T02:25:23Z

src/helpers/bodyTrap.js

+import portalOpenInstances from "./portalOpenInstances";
+// Body focus trap see Issue #742
+
+let before, after, instances;


Ýou can initialize instances with an empty array.

src/helpers/bodyTrap.js

src/helpers/portalOpenInstances.js

diasbruno · 2019-12-09T16:33:52Z

Thanks, @holloway. I merged your PR and will run a few tests on it.
Don't let me forget to make a release. :)

diasbruno · 2019-12-09T22:51:50Z

@holloway When the page gets the focus back - testing on firefox -, I'm seen that I need "a extra tab" because <html> is getting the focus. Is this the correct behavior or a difference between browsers?

holloway · 2019-12-10T01:53:08Z

@diasbruno hmm... I can't reproduce that behaviour in Firefox v71 (Linux), and I haven't heard of Firefox overriding JS-controlled focus in that way. What are the exact reproduction steps? For me, in Firefox and in Chrome, it restores focus to the button that it opened from. But you're seeing a change in behaviour compared to the ReactCommunity Codepen examples?

fyi I have tested focus management by running this to log the current element every second,

setInterval(function(){ console.log(document.activeElement); }, 1000);

And then interacting with the page to see how focus is moved around, and that part (when the page gets focus back) seems to behave as it was before this PR.

diasbruno · 2019-12-10T01:59:12Z

Thats look correct. Maybe this happens when I click on the browser's console and type document.activeElement.

Steps

Open modal
Click on the browser's address
Tab until it reaches the page view
Go to browser's console and type document.activeElement (maybe this is why I'm getting the <html>)

diasbruno · 2019-12-10T02:26:49Z

Firefox 71 here.

holloway · 2019-12-10T02:34:27Z

Ah then yes that might move focus to <html>. The trap elements are in <body> so if the focus is on <html>, before the trap, then they haven't walked into the focus trap yet.

There's a legitimate question about whether focusing <html> should move focus to the modal. I'm not sure of the answer to that, but I'll ask around with some accessibility experts that I know. If so, that would need a different approach than these trap elements, however in my testing it's basically harmless to allow <html> to be focused as the other content under the modal is still bypassed.

I guess the idea of using a setInterval was to poll the activeElement without needing to interact with the console which might change the activeElement and trigger a window blur event and react-modal changes (ie, I see focusManager.js responds to window blur).

diasbruno · 2019-12-10T02:38:46Z

LGTM. When we get more info about this, we can do something about this.

holloway · 2019-12-11T02:40:56Z

@diasbruno fyi I'm told that the extra tab in Firefox is expected behaviour that we wouldn't want to interfere with when a modal is open. FF users are in the habit of hitting tab twice, so this PRs' approach is still appropriate.

(@jasonkiss)

holloway · 2020-02-25T20:43:23Z

@diasbruno

Don't let me forget to make a release. :)

bump! 😄

diasbruno · 2020-02-25T21:37:55Z

🤦‍♂️

Thanks, @holloway.
I'll make a release today.

diasbruno · 2020-02-26T00:50:54Z

Release 3.11.2.

holloway · 2020-02-26T00:55:00Z

No probs, thanks @diasbruno !

* [added] Focus trap when reentering document (reactjs#742)

[fixed] Focus trap when reentering document (#742)

13208dd

holloway mentioned this pull request Dec 6, 2019

Modal doesn't trap focus when re-entering document #742

Closed

Matthew Holloway added 2 commits December 8, 2019 18:37

Trigger TravisCI

ec76c29

Moving focus trap init to constructor

bd080b6

Matthew Holloway added 2 commits December 8, 2019 20:46

Debugging Travis bug

31519fd

Debugging Travis bug

7f2482d

diasbruno reviewed Dec 8, 2019

View reviewed changes

src/components/ModalPortal.js Outdated Show resolved Hide resolved

Matthew Holloway added 3 commits December 9, 2019 14:54

Singleton pattern for tracking instances of open modals

d13ec84

Removing console

4c5425b

Adding eslint ignore console and 'React-Modal' prefix

864ce0a

diasbruno reviewed Dec 9, 2019

View reviewed changes

Only console logging when in dev mode

30b406e

diasbruno merged commit eb20444 into reactjs:master Dec 9, 2019

snyk-bot mentioned this pull request Mar 10, 2020

[Snyk] Upgrade react-modal from 3.10.1 to 3.11.2 FDMediagroep/fdmg-ts-react-ab#21

Merged

snyk-bot mentioned this pull request Mar 20, 2020

[Snyk] Upgrade react-modal from 3.11.1 to 3.11.2 Build-Week-International-S-S-W/Front-end#79

Merged

snyk-bot mentioned this pull request Apr 24, 2020

[Snyk] Upgrade react-modal from 3.1.0 to 3.11.2 electrode-io/car-buying#6

Open

programmer4web pushed a commit to programmer4web/react-modal that referenced this pull request Oct 5, 2020

[fixed] Focus trap when reentering document (reactjs#742) (reactjs#791)

e35279b

* [added] Focus trap when reentering document (reactjs#742)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fixed] Focus trap when reentering document (#742) #791

[fixed] Focus trap when reentering document (#742) #791

holloway commented Dec 6, 2019

diasbruno commented Dec 7, 2019

diasbruno commented Dec 7, 2019

coveralls commented Dec 8, 2019 •

edited

Loading

holloway commented Dec 8, 2019

diasbruno Dec 9, 2019

diasbruno commented Dec 9, 2019

diasbruno commented Dec 9, 2019 •

edited

Loading

holloway commented Dec 10, 2019 •

edited

Loading

diasbruno commented Dec 10, 2019 •

edited

Loading

diasbruno commented Dec 10, 2019

holloway commented Dec 10, 2019

diasbruno commented Dec 10, 2019

holloway commented Dec 11, 2019

holloway commented Feb 25, 2020

diasbruno commented Feb 25, 2020

diasbruno commented Feb 26, 2020

holloway commented Feb 26, 2020

[fixed] Focus trap when reentering document (#742) #791

[fixed] Focus trap when reentering document (#742) #791

Conversation

holloway commented Dec 6, 2019

diasbruno commented Dec 7, 2019

diasbruno commented Dec 7, 2019

coveralls commented Dec 8, 2019 • edited Loading

holloway commented Dec 8, 2019

diasbruno Dec 9, 2019

Choose a reason for hiding this comment

diasbruno commented Dec 9, 2019

diasbruno commented Dec 9, 2019 • edited Loading

holloway commented Dec 10, 2019 • edited Loading

diasbruno commented Dec 10, 2019 • edited Loading

diasbruno commented Dec 10, 2019

holloway commented Dec 10, 2019

diasbruno commented Dec 10, 2019

holloway commented Dec 11, 2019

holloway commented Feb 25, 2020

diasbruno commented Feb 25, 2020

diasbruno commented Feb 26, 2020

holloway commented Feb 26, 2020

coveralls commented Dec 8, 2019 •

edited

Loading

diasbruno commented Dec 9, 2019 •

edited

Loading

holloway commented Dec 10, 2019 •

edited

Loading

diasbruno commented Dec 10, 2019 •

edited

Loading