Size Change: +18 B (0%)

Total Size: 1.84 MB

_{compressed-size-action}

The e2e test I added is failing in Firefox yet testing manually in Firefox works as expected. In the e2e focus is ending up on the "Publish" button like it somehow skips the "Options" button "Open publish panel" button 😕. I’m at loss for how to resolve that at the moment.

UPDATE: I can reproduce testing manually. It only happens when the canvas is iframed and the post is new. The "Open publish panel" button is the last tab stop in the page. It’s like Firefox is moving focus as if the active element were the body.

UPDATE: I can reproduce testing manually. It only happens when the canvas is iframed and the post is new. The "Open publish panel" button is the last tab stop in the page. It’s like Firefox is moving focus as if the active element were the body.

I've discovered a few clues regarding this. It seems that the tab focus doesn't work well if the block inserter button is displayed in Firefox:

But I don't know why this problem only occurs in Firefox 🤷

I figured out the issue in Firefox. It stems from two event listeners moving focus—first on keydown then focusout. In Firefox, the second one actually works whereas other browsers aren’t honoring it. The first focus movement is the desired/expected one but it’s overridden by the second.

Like Aki noted, this corresponds with when the default block appender button is visible, though merely because the second focus move is conditioned to happen only if no blocks are present.

The solution I’ve tried in 1e40320 was to remove the second movement. From my testing the conditions where it actually happened are very rare and I haven’t yet seen what good it actually does. On the contrary it causes a focus loss in some. I have tried to figure it out by looking into the history, it’s from ac07149 which doesn’t make it obvious. It’s clearly intended for an undo related concern so that pretty much means it should not be happening from a tab navigation. An alternative to removing it would be refining the conditions so won’t interfere with tab navigation.

Now the e2e I added is passing but many other tests aren’t. They seem unrelated failures and they’re passing locally. I noticed some of the recent PRs are experiencing the same. I’ll keep this drafted anyway until that’s sorted out.

All e2e tests seem to pass now, so let's undraft it.

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

The solution I’ve tried in 1e40320 was to remove the second movement. From my testing the conditions where it actually happened are very rare and I haven’t yet seen what good it actually does.

This may be very rare, but I found that the focus might be lost in the following situations:

• Disable Paragraph block.
• Open the new post.
• Insert a block of some kind.
• Delete the block.
• Focus is lost? (Confirm that the screen reader reads "document" when the block is deleted in the following video).

If I revert 1e40320, the focus isn't lost as the writing flow wrapper gets focus (Confirm that the screen reader reads "section" when the block is deleted in the following video).

Is there a another approach to solve this problem?

I'm thinking about adding the Tab key check to the conditional statement:

function onFocusOut( event ) {
	// ...
	if (
		! event.relatedTarget &&
		ownerDocument.activeElement === ownerDocument.body &&
		getBlockCount() === 0 &&
		event.keyCode !== TAB
	) {
		node.focus();
	}

If pressing the Tab key causes focus to be lost, it should mean the focus is moved outside of the editor, which is what should be expected.

Thanks for testing Aki. I am glad you found this context where this logic seems suitable. I’ve pushed d057813 that restores it with an added condition that stops it from interfering with shift+tab from the post title in Firefox.

Still, I want to touch on a point regarding how this worked with the logic removed.

If I revert 1e40320, the focus isn't lost as the writing flow wrapper gets focus

When I test the same scenario, after deleting the last block the activeElement is the wrapper whether or not 1e40320 is reverted. Next, I find that tabbing from that point works the same with or without reverting. So, for browsers and tab order, focus does not seem lost. That’s not to disregard the result you had with the screen reader. I don’t get how that comes about. If the browser already has the wrapper as the activeElement and in iframed contexts (as your test appeared to be) the body is the wrapper, then how does it label that a "section"?

Aside from that, there is also another "hole" in the implementation. In Firefox, deleting a block does not cause a focusout event. While that seems like a browser bug it also means this logic doesn’t run there.

One more general point about this. It does not seem to belong in the tab nav hook. Tabbing never makes the block count change. So while I believe something like this is a useful idea, I feel like it should be implemented elsewhere. Additionally, I think we can do better than focus the wrapper in most cases by focusing a child (either the appender button or the post title) since one will almost always be available. This would be akin to how if you delete a single block and there is a previous sibling then focus goes to that sibling.

If pressing the Tab key causes focus to be lost, it should mean the focus is moved outside of the editor, which is what should be expected.

Thanks for the suggestion 🙇 The catch is that since it’s a focusout the event won’t include key data. It could still work by keeping a ref updated from keydown that stores whether the key was tab. I briefly tested it and found that it still allowed this logic to run in some cases it shouldn’t. E.g. when no blocks are present and the default block appender button is pressed. In most browsers that didn’t cause trouble but in Firefox prevents the popover from getting focus.

Flaky tests detected in c21a8b8.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13960634244
📝 Reported issues:

• [Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js

@stokesman Would it be ok to merge/backport this PR?

It's not a big problem, so maybe we could include it in RC1 instead of Beta3. cc @Mamaduka

Yes, let's sync this in RC1. I've already released packages for beta 3.

@t-hamano, @stokesman, is this good to merge?

As far as I can tell this is good to merge.

For further reference, this early return was removed in #65204 previous to this PR

It occurred to me that this seems it could change behavior for consumers but it’s been shipping that way in Gutenberg for some time so perhaps it’s fine.

The scenario in which I'm thinking of would be a consumer that’s adding some tabbable elements as children of BlockCanvas but that are not within blocks. The post title is an example of such a thing. With this condition for no block selection causing an early return it means that tabbing would navigate between such elements. Without this condition tabbing is going to be exiting the canvas.

I'm unaware of consumers for which this would be applicable. I did get concerned about WooCommerce because I know their beta product editor uses the canvas and has various tabbable elements in it. I tested that out and it looks like they are always wrapped in blocks so nothing should change. Nor did I find any changes from my quick testing.

That's interesting. I was testing that behavior on WP 6.7, and it seems broken.

1. Create a new post.
2. The title gets focus.
3. The tab moves to the default inserter "paragraph".
4. I would expect Shift+Tab to move focus back to the Title, previously focused element.
5. Instead, the focus moves to the "Add Block" button.

I think this is a good improvement, so let's go with this fix in WP 6.8.

The scenario in which I'm thinking of would be a consumer that’s adding some tabbable elements as children of BlockCanvas but that are not within blocks. The post title is an example of such a thing. With this condition for no block selection causing an early return it means that tabbing would navigate between such elements. Without this condition tabbing is going to be exiting the canvas.

In my experience as well, I have never heard of a plugin that injects such elements.

I think it’s safe to merge this PR and backport it to 6.8.

I also think this is an improvement and should be safe. Thanks for the reviews and consideration.

I was testing that behavior on WP 6.7, and it seems broken.

Yes, that particular scenario is confusing. I think it’d be nice to follow up on that and see if we can make the tab order match the visual order. The "Add block" button appears to be after the paragraph but it’s in a popover that precedes the entire canvas.

I just cherry-picked this PR to the wp/6.8 branch to get it included in the next release: 4b49562