Make sure useFocusOnMount runs when all the children tabbable elements have mounted #42187
Conversation
afercia
added
[Type] Bug
|
Size Change: +723 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
alexstine
added
the
[Focus] Accessibility (a11y)
|
In the second commit I changed a bit the code to use |
|
I restarted the job a few times but the |
afercia
force-pushed
the
fix/focus-on-mount-tabbables-added-later
branch
from
9d311c7 to
eaf3cc5
Compare
|
Latest commit fixes the failing
I changes the test by explicitly passing |
|
Latest commit refactors the previous approach to make
The simplest way to test is by altering Note: the I'd greatly appreciate a review from someone more familiar with e2e tests. |
afercia
force-pushed
the
fix/focus-on-mount-tabbables-added-later
branch
from
3810c08 to
483f605
Compare
|
Rebased on top of latest trunk to solve merge conflict and incorporate a change for the Popover test. Hopefully tests will pass now. |
|
Flaky tests detected in 4697327. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4262856582
|
|
There are still some snapshots to update. This PR fixes the focus on mount behavior while some tests appear to rely on the incorrect behavior. Actually, the snapshot failures surfaced a new bug. Specifically, it's about the For example, the format library text color By default, the However, it doesn't work as expected. Actually, it sets focus on the container. To reproduce on trunk:
Dumping to the console the
On this PR:
Dumping to the console the
|
|
It appears this needs some more work. With Safari, I'm seeing that when opening the text color picker, focus is moved back to the 'More' button and the popover opens in a wrong position. It happens only with Safari. Works well with Chrome and Firefox.
|
afercia
changed the title
afercia
force-pushed
the
fix/focus-on-mount-tabbables-added-later
branch
from
0b46c66 to
90b54b9
Compare
|
Rebased on top of latest trunk, which includes a relevant fix for the popover test. |
|
I was able to reproduce the popover mis-positioning also on trunk (see previous comment), so that appears unrelated to this PR. Will create a new issue. |
|
I think the snapshot failure is because, apparently, when the RichText Worth noting the test fails locally also on trunk. |
|
Screenshot from the CI failures-artifacts highlights the active theme on CI is Twenty Twenty-One, but the color palette is the default one:
Instead, locally Twenty Twenty-One renders its own color palette:
Thus, regardless of the amount of Yab key presses used on the test, the local test results will be different from the ones on CI, making impossible to correctly update the snapshots. I'm not sure why this happens. I'd greatly appreciate some help. Cc @kevin940726 🙇 |
|
The rich text tests haven't been migrated to Playwright yet, so I'm not sure if I'm the best person to ask here. Maybe @ellatrix would know better since she wrote the test? |
|
Thanks @kevin940726. The failing snapshot actually tests something unrelated to this PR. The issue is that this PR fixss initial focus on the color picker popover. Therefore, the number of Tab key presses in the test selects a different color. This could be fixable by just updating the snapshot. However, the actual color palette rendered on CI is different from the color palette rendered locally. No idea why. The color selected locally will always be different from the color selected on CI. |
|
I'm not sure why the palette is different. I just wrote a test to check if the palette works at all. Maybe you have a slightly different version of the theme or some plugin? |
|
I figured out. There's a difference between the Gutenberg My local setup is a standard Instead, when running the Gutenberg Will create a separate issue. |
|
Tests finally pass. I did my best to update the testing instructions. Given such an important accessibility feature like |
|
The popover position issue with Safari is unrelated to this PR. It can be reproduced also on trunk and I created an issue for that: #48364 All tests pass. I manually tested a few places where |
Fixes #42064
What?
Makes sure
useFocusOnMountruns when all the tabbable elements within the nodeuseFocusOnMountis applied to are actually mounted.Why?
When opening the block toolbar settings menus, initial focus goes to the second menu item. It should go to the first item.
How?
Restores the
SetTimeoutthat was removed in #27699I'm not that familiar with this implementation and maybe there are better ways to fix the issue. The point is
useFocusOnMountruns on a parent component that contains tabbable elements. The components that render the tabbable elements may be mounted slightly after the parent node is mounted so they're 'skipped'. To avoid this,useFocusOnMountneeds to run when all children tabbables are actually mounted.Updated Testing instructions.
The 'Show/Hide more settings' button was removed from the block toolbar Options menu in #46709. That cured the symptom and not the root cause of this issue.
useFocusOnMountstill fails when the children tabbable elements are mounted at a later time than thenodewhereuseFocusOnMountis used. To test:Additionally:
As a side effect, this PR fixes the initial focus on the Popover component. In the Popover, the default value of
focusOnMountisfirstElement. However, that doesn't work. Actually, initial focus goes to the Popover node. That's because the currentfocusOnMountimplementation sort-of fallbacks to the main node if no first tabbable is found. To test:Old Testing Instructions
useFocusOnMountis used, to make sure everything still works as expected. For example:Screenshots or screencast