Constrained tabbing: fix unstable behavior in firefox #42653
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +13.4 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
t-hamano
added
the
Browser Issues
ellatrix
reviewed
Sep 23, 2022
| @@ -64,7 +64,7 @@ function useConstrainedTabbing() { | |||
| node[ domAction ]( trap ); | |||
| trap.focus(); | |||
| // Remove after the browser moves focus to the next element. | |||
| timeoutId = setTimeout( () => node.removeChild( trap ) ); | |||
| timeoutId = setTimeout( () => node.removeChild( trap ), 10 ); | |||
There was a problem hiding this comment.
What if we add an event listener to the trap and remove the element only when focus was moved out of it?
There was a problem hiding this comment.
I have made the following changes and have confirmed that it works as expected:
// Remove itself when the trap loses focus.
trap.addEventListener( 'blur', () => node.removeChild( trap ) );
trap.focus();I couldn't figure out how to remove the event listener when the element itself is deleted, but is it also need to remove the event when the element is deleted?
There was a problem hiding this comment.
I'm not an expert but I think the element itself and the event listener are retained in memory because there's still a reference to it. To test: dump the trap element to the console after it's removed from the DOM, see that it's still in memory::
trap.addEventListener( 'blur', () => {
node.removeChild( trap );
console.log( trap );
} );
Since a new trap element and event listener will be created at each Tab key press on the last and first tabbable elements, I suspect this could lead to some memory leak. Again, I'm not an expert but I think that declaring the trap element with let and then assigning it to null after it's removed could be an option e.g. trap = null;.
Would appreciate feedback from someone with better knowledge than me.
There was a problem hiding this comment.
I couldn't figure out how to remove the event listener when the element itself is deleted, but is it also need to remove the event when the element is deleted?
It's my understanding that the listener does not need to be removed as long as there is no persisting reference to the element. It looks like in this case it'd be fine because each time the onKeyDown handler creates trap, the only closure around it is the blur listener itself. However, to be sure about it you could use the once option when adding the listener:
// Remove itself when the trap loses focus.
trap.addEventListener( 'blur', () => trap.remove(), { once: true } );That example also switches to remove instead of removeChild for saving a few bits. Since IE11 doesn't need supported that should be safe.
Also I couldn't quickly find an outside source to back up my statements but there is this bit from the the jsdoc of useRefEffect:
gutenberg/packages/compose/src/hooks/use-ref-effect/index.ts
Lines 20 to 23 in a12ad38
Which agrees and indicates that this hook itself could drop the cleanup function here:
gutenberg/packages/compose/src/hooks/use-constrained-tabbing/index.js
Lines 73 to 77 in a12ad38
afercia
reviewed
Nov 21, 2022
| trap.focus(); | ||
| // Remove after the browser moves focus to the next element. | ||
| timeoutId = setTimeout( () => node.removeChild( trap ) ); |
There was a problem hiding this comment.
Since this timeout is not used any longer, the variable declaration at line 36 and the clearTimeout at line 75 should be removed as well.
There was a problem hiding this comment.
Removed by 11a1cfa
|
Once this gets reviewed, it would be great to add some E2E tests with Playwright and run them on both Chrome and Firefox. Such an important usability and accessibility feature should never break and there should be tests in place to make sure it doesn't. Kind reminder that this is a regression present since WordPress 5.9 - Gutenberg 11.8, which is almost one year ago. |
|
Agreed. 👍 |
alexstine
added
the
[Focus] Accessibility (a11y)
|
Note: I'm working on a separate PR to move the existing a11y tests (which do have some tests for constrained tabbing) to Playwright so that we can run them with Firefox too. I'll also try to add some new tests. |
|
Thank you for the review, @afercia, @stokesman, @alexstine! @stokesman |
afercia
added
[Type] Bug
I don't think it’s needed. I largely bought it up to promote awareness of it in case it comes in handy elsewhere. |
|
Thanks for the fix. Highly appreciated :) |
|
🎉 ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #42652, #45903
Follow up on #34836
What?
This PR fixes a problem with "constrained tabbing" sometimes losing focus in Firefox.
"Constrained tabbing" means that the tab moving is constrained to the content of the wrapper element such as modal component.
As reported in #42652, I confirmed that in Firefox, the focus is sometimes lost from the modal when the following operation is performed:
tabkey is pressed on the last focusable element (the first element should be focused)tab + shiftkey is pressed on the first focusable element (the last element should be focused)Why?
I learned that the constrained tabbing useConstrainedTabbing hook.
For example, when the tab key is pressed on the last focused element, the following logic is used to move focus to the first element:
However, in Firefox, I found that the trap element is sometimes deleted while the focus remains on the trap element.
In other words, I believe that the focus is lost by deleting the trap element that remains in focus, and the browser is moving focus without considering the wrapper element.
How?
I haven't been able to find valid documentation, but I think we need to wait for the browser to properly move focus to the next element before removing the trap element.
So I added a small delay to the
setTimeoutfunction that removes the trap element.Testing Instructions
Screenshot
1d56267affde3a5df8713d9aaa28a5f2.mp4