Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nonfatal error tabbing away from preview frame in Site Editor #69037

Open
3 of 6 tasks
stokesman opened this issue Feb 5, 2025 · 8 comments · May be fixed by #69079
Open
3 of 6 tasks

Nonfatal error tabbing away from preview frame in Site Editor #69037

stokesman opened this issue Feb 5, 2025 · 8 comments · May be fixed by #69079
Assignees
Labels
[Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@stokesman
Copy link
Contributor

Description

In the Site editor tabbing away from the preview frame causes an error. I expect it to be error free.

This doesn’t happen in 6.7.1 without Gutenberg plugin. I'm not sure if the latest Gutenberg plugin releases have this issue or not.

Step-by-step reproduction instructions

  1. Open the Site editor
  2. Tab through the UI until the preview frame is focused
  3. Tab again (forward or backward)
  4. Note the error in the console

Screenshots, screen recording, code snippet

site-editor-tab-away-from-preview-error.mp4

Environment info

  • WP 6.7.1 with Gutenberg plugin at trunk
  • Chrome
  • macOS

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@stokesman stokesman added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor labels Feb 5, 2025
@stokesman
Copy link
Contributor Author

The error is in the useTabNav hook. It’s possible something should be fixed there. However, the issue could seemingly be avoided and some optimization be gained by preventing that hook and any of the others composed by useWritingFlow from being active when the Iframe is in a preview mode. The writing flow is not needed because nothing in the frame is interactive.

@singhakanshu00
Copy link
Contributor

Hi, @stokesman
I was thinking of implementing optional chaining in here which will not throw any undefined error:

next.current.focus( { preventScroll: true } );

which in my case solved the issue. Let me know your thoughts.

Screen.Recording.2025-02-05.at.10.20.31.AM.mov

@Mamaduka
Copy link
Member

Mamaduka commented Feb 5, 2025

@singhakanshu00, while that would fix the symptom, that doesn't fix the root cause @stokesman mentions here.

Sometimes, it's okay to fix errors with optional chaining, but generally, fixing the root cause of the issues should be preferred.

@t-hamano
Copy link
Contributor

t-hamano commented Feb 5, 2025

I identified this issue as being caused by #65204.

This PR contains a lot of code changes, but it would be a good idea to investigate why next.current becomes undefined here, or whether it is correct logic that this code is executed when focus is lost from the site editor.

It may not be an urgent issue, but let's see if we can fix it in WP 6.8.

@stokesman
Copy link
Contributor Author

It turns out there is at least one other closely related issue with the preview frame focus and keyboarding. The "Select All" shortcut is active in preview mode (screen recording disclosed)
site-editor-preview-unintended-keyboard-editable.mp4

I’ll see about making an issue/PR for that. I mention it here because the fix I have in mind would avoid/obscure this issue and I think pinpointing the cause, per Aki’s last comment, would be ideal.

@stokesman
Copy link
Contributor Author

stokesman commented Feb 6, 2025

it would be a good idea to investigate why next.current becomes undefined here

Here’s what I’ve gleaned. In the context of the Site editor and the preview canvas, the undefined value looks to have been a thing since #59317 but not a problem until this early return was removed in #65204.

I confirmed this by testing after making it return early for the same conditions:

diff --git a/packages/block-editor/src/components/writing-flow/use-tab-nav.js b/packages/block-editor/src/components/writing-flow/use-tab-nav.js
index 46c40d56fe..83063ee9e3 100644
--- a/packages/block-editor/src/components/writing-flow/use-tab-nav.js
+++ b/packages/block-editor/src/components/writing-flow/use-tab-nav.js
@@ -111,7 +111,10 @@ export default function useTabNav() {
 
 	const ref = useRefEffect( ( node ) => {
 		function onKeyDown( event ) {
-			if ( event.defaultPrevented ) {
+			if (
+				event.defaultPrevented ||
+				( ! hasMultiSelection() && ! getSelectedBlockClientId() )
+			) {
 				return;
 			}
 

@t-hamano
Copy link
Contributor

t-hamano commented Feb 6, 2025

@stokesman Thank you for your research.

I confirmed this by testing after making it return early for the same conditions:

This diff looks like it works fine for me. Do we submit a PR and see if it passes CI?

@Mayank-Tripathi32
Copy link
Contributor

I will open up a PR to run the tests, it seems to be working fine with the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging a pull request may close this issue.

5 participants