[Modal] Fix focus not being contained #16585
Conversation
eps1lon
added
bug 🐛
Details of bundle changes.Comparing: 3b6c822...e4d192c
|
| @@ -97,11 +97,11 @@ function TrapFocus(props) { | |||
| } | |||
| }; | |||
|
|
|||
| doc.addEventListener('focus', contain, true); | |||
There was a problem hiding this comment.
Huh the 3rd was capture, right? Do these events go through the capture phase?
eps1lon
force-pushed
the
fix/FocusTrap/containment
branch
from
11a0c2b to
17eca2d
Compare
eps1lon
force-pushed
the
fix/FocusTrap/containment
branch
from
17eca2d to
288672a
Compare
|
Hum, I'm out of idea for supporting Firefox (aside from setInternval…). I'm wondering if Firefox doesn't have an active bug on this matter: https://bugzilla.mozilla.org/show_bug.cgi?id=559561. |
|
Yeah I'll give it a spin tomorrow but I'm not spending too much time on it considering there's active work on this issue in the react core. |
|
@eps1lon 💯 for giving up if it takes more than x hours. Opportunity cost, opportunity cost! :) |
|
It seems that the fix only works on Chrome. I had no success in Edge and Firefox to find any event to leverage. I have tried to look at how the popular open-source project handles the problem: none seems to solve it, e.g. reactjs/react-modal#486. We have a few options, by order of personal preference:
|
|
Oh, just realized that the test on browserstack reports the same observation than I did, I should have checked first 🤦♂️. I have spent 15 minutes trying the interval approach out. Pushing to see the report of the CI. I'm really happy about your new test @eps1lon 👍. Let me know what you think. |
oliviertassinari
force-pushed
the
fix/FocusTrap/containment
branch
from
0db1dac to
095af44
Compare
| @@ -100,7 +100,13 @@ function TrapFocus(props) { | |||
| doc.addEventListener('focus', contain, true); | |||
| doc.addEventListener('keydown', loopFocus, true); | |||
|
|
|||
| const interval = setInterval(() => { | |||
| contain(); | |||
| }, 500); | |||
There was a problem hiding this comment.
500ms is really long. Shouldn't this be 50?
There was a problem hiding this comment.
I don't know. The value should be large enough not to put much pressure on the main thread but be small enough not to disturb user interactions. The average reaction time for a human is around 700ms, e.g. for a car accident. Maybe the value could be defined with a heuristic: pick the reproduction example, start from 50ms, increase the value up until we can no longer make the escape keypress, consequent to an enter keypress, work reliably?
There was a problem hiding this comment.
100ms is the magic number when it comes to noticing time passed. If 400ms puts already pressure on the main thread your app has other issues.
contain is currently expensive? Maybe just check if the focus is on the body (which is the case if it is lost like in the reported issue) and only then run it. This should allow us to reduce it to 50ms which is ~3 frames. That seems alot less likely to be noticeable.
There was a problem hiding this comment.
Yeah, sure, that makes sense to me. No matter what we pick, the cost of changing it is low. Can it have an impact of people perception if the see something that ticks every 50ms when debugging a perf issue on their app? Or, can it have an impact on the requestidlecallback API? I have no idea. The future will tell us 🤷♂️. +1 for trying 50ms.
|
For future reference: While working please prefer to branch of the work of others. If you need CI open a separate PR to this repository. If it's more about discussing implementation details a PR to the fork repository is more appropriate. If I want to continue my work I now have to checkout previous versions of my work or hard reset your work. I'm going open a new PR with my previous work this time but I'd hope we make a collaborative effort in the future. |
|
Confirmed bug in Firefox: The issue is that in Firefox no focus related events are fired when the focused area stops being a focused area. The whatwg spec defines how the browser should behave but does not explicitly mention any events (chrome and IE11 do fire blur events):
-- https://html.spec.whatwg.org/multipage/interaction.html#focus-fixup-rule Given that I would stick with the "containOnBlur" handler which handles spec compliant browsers immediately and add a polling based fallback with the note that it should be removed once firefox fixes that bug and we no longer support versions with that bug. |
|
Do you confirm that Edge and Safari have the same bug? |
I think that collaboration should happen, ideal, under a single centralized thread. This is what we do with comments. I wish we could do the same with code changes. Its work best when different paths can be explored, one at the time. However, there is one important limitation in practice with GitHub: asynchronicity. We can't push at the same time under the same branch while we can comment at the same time. I think that it should be encouraged when:
I'm aware you dislike when somebody pushes on your remote (you mentioned it at multiple occasion in the past), I will keep reducing my threshold for doing it with you (hopefully, never do it). Thanks for the feedback.
You can hard reset the path I have explored. It's fine, I don't mind at all. GitHub keeps track of the push forces now. It won't be lost. As for syncing the local environment with remote, when I have this problem, I personally use this approach. I don't know if there is a simpler one?
|
|
I know how to handle git and get my changes back. If you only care about your workload then keep on pushing over my implementations and I will put in the extra effort to work on my branches that you don't want to put in. Glad that we could finally talk open about it. If you want to explore different paths you can look into branches in git and how to track different remotes with them. Git offers tracking of changes not only sequentially with commits but also in parallel with branches. |
Can't even get Safari to tab into the dialog so I'll leave that alone. Edge has the same issue as firefox but will at least be fixed once they are chromium based. Guess we stick with the polling based approach. MutationObserver might do as well but I honestly don't want to put too much benchmarking effort into this so 50ms polling rate sounds fine. |
|
The interval still runs at the end of the suite, it sounds wrong. I believe one of the tests in our suite doesn't clean up correctly. 🔍 Looking into which test leak. |
|
Found it: diff --git a/packages/material-ui/src/Popover/Popover.test.js b/packages/material-ui/src/Popover/Popover.test.js
index 92f112dd1..3593fdc34 100644
--- a/packages/material-ui/src/Popover/Popover.test.js
+++ b/packages/material-ui/src/Popover/Popover.test.js
@@ -609,6 +609,7 @@ describe('<Popover />', () => {
let wrapper;
before(() => {
+ clock = useFakeTimers();
innerHeightContainer = window.innerHeight;
const mockedAnchor = document.createElement('div');
stub(mockedAnchor, 'getBoundingClientRect').callsFake(() => ({
@@ -629,8 +630,6 @@ describe('<Popover />', () => {
</Popover>,
);
element = handleEntering.args[0][0];
-
- clock = useFakeTimers();
});
after(() => {@eps1lon The strategy proposed in #16585 (comment) sounds great to me 👍. How do you want to move forward? |
|
@eps1lon I will assume that you have assigned me to the pull request so I push it one step further. I'm on it :). |
|
Ok, it should be good to go 🚢. I have restricted the changes to the addition of the interval to support Firefox, Edge and Safari. The other alternatives were breaking the focus ring in some way. Given that the long term plan is to get rid of this component, better not invest too much time on it. |
|
@oliviertassinari could you please apply this fix also for 3rd mui version? |
|
Sorry, we have no plan for that. |
Not sure yet if the test is properly set up but I have codesandbox as a proof of concept.
The issue was that DOM events were confused with React events. In React every event bubbles. In the DOM that's not necessarily the case. Specifically focus does not bubble. Focusout does.
Should close #16584 if done