[TrapFocus] Fix portal support #21610

mnajdova · 2020-06-28T08:35:28Z

This PR enables portals support inside TrapFocus.

What is the problem?

When there are portals rendered inside the TrapFocus, they are rendered outside of the that elements dom tree. This creates the following problem: by nature the TrapFocus traps the focus whenever the focus is not inside it's dom tree. That means if the focus goes inside the portal, it is "stealed" by the TrapFocus and it's put on the root element (an example of this can be find in #15694)

Solution

The solution to this problem is, we need to somehow figure out if the focusing event is happening inside the React tree, not just the dom tree. I have experiment with different approaches of how this can be solved, but settled on the event propagation, as the most stable one (there was a different one that uses React's internal, you can find the link in the end of the PR description).

What we need to do is, add an onFocus event on the children of the TrapFocus and set some internal ref to the last target that was focused, so that we can check that it is indeed the element focused when the contain method is invoked - it is a dom listener on the document for the focus event. This is necessary because we may run up to some inconsistency, when some element outside of the TrapFocus' React tree is focused.

Fixes #15694

For people interested in an alternative that uses React's internals: d8c75e5.

docs/src/pages/components/dialogs/DialogWithPortalsInside.js

mui-pr-bot · 2020-06-28T08:51:27Z

Details of bundle changes

Generated by 🚫 dangerJS against 3eead64

oliviertassinari

The logic looks solid, I couldn't find anything better.

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.test.js

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

docs/src/pages/components/dialogs/dialogs.md

docs/src/pages/components/dialogs/DialogWithPortalsInside.js

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

….test.js Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

oliviertassinari · 2020-06-29T14:54:16Z

I think that it would be great to check that the changes solve this downstream issue mui/material-ui-pickers#1852.
We can do such by using the picker reproduction and replacing the used version by the one Codesandbox-CI publishes :). @dmtrKovalenko Could you check that out? :)

eps1lon

Not easy to hold different trees in your head where focus events propagate differently. I think it's important for knowledge sharing especially to go into more details for this change.

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

eps1lon · 2020-07-01T06:27:24Z

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

@@ -73,7 +75,7 @@ function Unstable_TrapFocus(props) {
      rootRef.current.focus();
    }

-    const contain = () => {
+    const contain = (event) => {


Is this a native DOM event or a synthetic React event? As long as we're untyped I'd suggest using *event for react events and *nativeEvent for native DOM events to make it clear what we're dealing with.

From past experience, such distinction is also useful for using event.key vs nativeEvent.keyCode in regard to how React polyfill IE 11.

Renamed to nativeEvent 👍

eps1lon · 2020-07-01T06:29:27Z

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

@@ -85,6 +87,18 @@ function Unstable_TrapFocus(props) {
      }

      if (rootRef.current && !rootRef.current.contains(doc.activeElement)) {
+        // if the focus event is different than the last syntheticEvent from the children, reset


Having a hard time understanding this branch. Could you explain with JSX and an event order when we reach this branch? Ideally point to an existing test or add a new one for this branch.

The condition is trying to capture whether between the time we saved the last focused event target and this invocation of the function, it has changed. We have two scenarios we need to check:

the focus event was invoked with different target (that is the first part of the condition)

the element that was focused was removed and the interval for the function run out (that is the second part of the condition)

The 2 condition is captured already in the test where an element is removed and the focus moves to body, I will add a test for the other use-case (it may be hard to make sure which exact of these two condition will be caught because the function is invoked in an interval, but let me try to deal with it)

Does this clarifies the use case? I will also try to update the comment to better reflect this.

On this note, I would like to test whether just the second part of the condition will be enough, as the interval would anyway invoke this function, but it may have a bit of delay, so it may not be the best idea..

The newly added trest together with the previous covers all use casees I believe - focusing an dom outisde the react tree, as well focusing nodes inside portal

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

mnajdova · 2020-07-01T19:28:41Z

Not easy to hold different trees in your head where focus events propagate differently. I think it's important for knowledge sharing especially to go into more details for this change.

@eps1lon I've updated the PR description, hopefully this clarifies things. Let me know what do you think

joshwooding

Really clean!

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

oliviertassinari · 2020-07-01T22:56:31Z

I think that it would be great to check that the changes solve this downstream issue mui/material-ui-pickers#1852.

Tested in mui/material-ui-pickers#1852 (comment), we are good 👌

chore: Refactor to single variable

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

mnajdova added 9 commits June 27, 2020 09:48

wip

78a5b0c

invoked props handlers

2151d1d

wip

47d1726

fixed issue

10a67bb

replaced relatedTarget with target

5eae936

removed console log

17f0606

added example

246081a

tests

c4f4369

test

7b09e77

mnajdova commented Jun 28, 2020

View reviewed changes

docs/src/pages/components/dialogs/DialogWithPortalsInside.js Outdated Show resolved Hide resolved

prettier

3055b69

github-actions bot mentioned this pull request Jun 28, 2020

Update snapshots for pr/21610 eps1lon/mui-scripts-incubator#463

Closed

mnajdova added 2 commits June 28, 2020 12:13

test

3dd24bf

reverted test changes

248734a

oliviertassinari added the component: FocusTrap The React component. label Jun 28, 2020

oliviertassinari changed the title ~~Bug/portal dialog trapfocus~~ [TrapFocus] Fix portal support Jun 28, 2020

mnajdova added 2 commits June 28, 2020 13:11

fixed issue

beba567

prettier

662d6ca

mnajdova marked this pull request as ready for review June 28, 2020 11:49

mnajdova requested a review from oliviertassinari June 28, 2020 11:49

oliviertassinari reviewed Jun 28, 2020

View reviewed changes

oliviertassinari added the bug 🐛 Something doesn't work label Jun 28, 2020

oliviertassinari mentioned this pull request Jun 28, 2020

[TrapFocus] Fix disableAutoFocus prop #21612

Merged

mnajdova and others added 5 commits June 28, 2020 20:22

Update packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

0637674

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Update packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus…

9338196

….test.js Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Update packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

1c5aedf

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

renamed refs

8f21445

removed demo

7ad7a62

oliviertassinari approved these changes Jun 28, 2020

View reviewed changes

oliviertassinari requested a review from eps1lon June 28, 2020 21:51

oliviertassinari requested a review from dmtrKovalenko June 29, 2020 14:54

eps1lon requested changes Jul 1, 2020

View reviewed changes

mnajdova added 2 commits July 1, 2020 21:39

addressed comments

e514baf

nativeEvent

982f7a0

mnajdova requested a review from eps1lon July 1, 2020 20:00

joshwooding reviewed Jul 1, 2020

View reviewed changes

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js Outdated Show resolved Hide resolved

oliviertassinari mentioned this pull request Jul 1, 2020

Desktop pickers don't work inside modals mui/material-ui-pickers#1852

Closed

eps1lon and others added 2 commits July 2, 2020 13:13

chore: Refactor to single variable

a79cb09

Merge pull request #1 from eps1lon/chore/TrapFocus/single-react-tree-var

cb69b48

chore: Refactor to single variable

mnajdova commented Jul 2, 2020

View reviewed changes

packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js Outdated Show resolved Hide resolved

Update packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js

3eead64

oliviertassinari approved these changes Jul 2, 2020

View reviewed changes

eps1lon approved these changes Jul 2, 2020

View reviewed changes

mnajdova merged commit b50793d into mui:next Jul 2, 2020

This was referenced Jul 4, 2020

[docs] Batch small changes #21669

Merged

Fix resize mui/mui-x#59

Merged

oliviertassinari mentioned this pull request Mar 27, 2021

[TrapFocus] Improve props #25528

Closed

1 task

mnajdova mentioned this pull request Sep 9, 2023

[FocusTrap] Refactor & cleanup #38878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TrapFocus] Fix portal support #21610

[TrapFocus] Fix portal support #21610

mnajdova commented Jun 28, 2020 •

edited

Loading

mui-pr-bot commented Jun 28, 2020 •

edited

Loading

oliviertassinari left a comment

oliviertassinari commented Jun 29, 2020

eps1lon left a comment

eps1lon Jul 1, 2020

oliviertassinari Jul 1, 2020

mnajdova Jul 1, 2020

eps1lon Jul 1, 2020

mnajdova Jul 1, 2020

mnajdova Jul 1, 2020

mnajdova Jul 1, 2020

mnajdova commented Jul 1, 2020

joshwooding left a comment

oliviertassinari commented Jul 1, 2020

[TrapFocus] Fix portal support #21610

[TrapFocus] Fix portal support #21610

Conversation

mnajdova commented Jun 28, 2020 • edited Loading

What is the problem?

Solution

mui-pr-bot commented Jun 28, 2020 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 29, 2020

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon Jul 1, 2020

Choose a reason for hiding this comment

oliviertassinari Jul 1, 2020

Choose a reason for hiding this comment

mnajdova Jul 1, 2020

Choose a reason for hiding this comment

eps1lon Jul 1, 2020

Choose a reason for hiding this comment

mnajdova Jul 1, 2020

Choose a reason for hiding this comment

mnajdova Jul 1, 2020

Choose a reason for hiding this comment

mnajdova Jul 1, 2020

Choose a reason for hiding this comment

mnajdova commented Jul 1, 2020

joshwooding left a comment

Choose a reason for hiding this comment

oliviertassinari commented Jul 1, 2020

mnajdova commented Jun 28, 2020 •

edited

Loading

mui-pr-bot commented Jun 28, 2020 •

edited

Loading