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

Site editor: missing "Back" button in Document bar in focused editor coming from page #69016

Open
3 of 6 tasks
stokesman opened this issue Feb 3, 2025 · 11 comments · May be fixed by #69093 or #69103
Open
3 of 6 tasks

Site editor: missing "Back" button in Document bar in focused editor coming from page #69016

stokesman opened this issue Feb 3, 2025 · 11 comments · May be fixed by #69093 or #69103
Assignees
Labels
[Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@stokesman
Copy link
Contributor

stokesman commented Feb 3, 2025

Description

When editing a page in the Site editor and entering focused mode to edit a template or template part there is supposed to be a "Back" button in the Document bar.

Step-by-step reproduction instructions

  1. Edit a page in the Site editor
  2. Either:
    • Select a template part and click "Edit" in its toolbar
    • Click "Edit template" from the "Template options" menu in the Document summary panel
  3. Note that there is no "Back" button in the Document bar

Screenshots, screen recording, code snippet

The following demonstrates the issue for both templates and template parts and at the 19 second mark also demonstrates how the "Back" button does appear if you edit a template part when already editing the template.

missing-back-button-doc-bar.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 [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended labels Feb 3, 2025
@SainathPoojary
Copy link
Contributor

Hey @stokesman,

I tried reproducing the issue using WP 6.7.1 and the Gutenberg plugin, as well as the Gutenberg trunk. However, I wasn’t able to reproduce the bug. Is there a step I might have missed?

2025-02-04.08-07-59.mp4

@stokesman stokesman changed the title Missing "Back" button in Document bar in focused editor coming from page Site editor: missing "Back" button in Document bar in focused editor coming from page Feb 4, 2025
@stokesman
Copy link
Contributor Author

Hi @SainathPoojary, thank you for testing. It looks like you tested the Post editor and not the Site editor. I had that in the instructions but now I’ve updated the issue title as well in hopes no one else misses that detail.

@SainathPoojary
Copy link
Contributor

Thank you for the clarification @stokesman . You’re right I initially tested in the Post editor. After switching to the Site editor, I was able to reproduce the issue as described.

@Mayank-Tripathi32
Copy link
Contributor

Mayank-Tripathi32 commented Feb 4, 2025

This looks like a possible regression from recent PR, I will investigate it to find the bad commit.

Update: this issue is regression, I was able to locate some good commits: 9da58a749e2958f65c51352afd2f469379057fc4

But taking a bit of time to locate the exact commit that introduced this bug.

I will keep working on this.

@hbhalodia
Copy link
Contributor

Hi @Mayank-Tripathi32 @SainathPoojary, I tried to debug the issue and found this issue which is related to this - #67199, but not sure if this had cause the regression to be happened and to be more specific - c9a5cab, this commit may have cause this issue.

Thank You,

@Mayank-Tripathi32
Copy link
Contributor

@hbhalodia Thank you for identifying the faulty commit. I'll investigate further to determine the root cause of the bug and create a PR with a fix.

@t-hamano
Copy link
Contributor

t-hamano commented Feb 5, 2025

From my research, this issue was caused by #66851. It's strange because it seems like an unrelated PR at first glance.

The console error when you click the "Go back" link in the snack bar should be a clue:

Image

By the way, I noticed that the following console error also occurs in WP 6.7. It would be good if we could fix this too.

  • Open any page in the site editor.
  • Click the "Edit template".
  • Click the "Back" button.
  • Click the "Go back" link in the snack bar.
  • The error will be displayed in the browser console.
3f05d1753661ab6b4f28d6527c4c6cf2.mp4

@t-hamano
Copy link
Contributor

t-hamano commented Feb 6, 2025

I haven't been able to solve the problem yet, but I have found that making the following changes temporarily fixes the problem:

Diff
diff --git a/packages/edit-site/src/components/site-editor-routes/template-item.js b/packages/edit-site/src/components/site-editor-routes/template-item.js
index 22726f6a5a..8ad3ab2b69 100644
--- a/packages/edit-site/src/components/site-editor-routes/template-item.js
+++ b/packages/edit-site/src/components/site-editor-routes/template-item.js
@@ -1,7 +1,7 @@
 /**
  * Internal dependencies
  */
-import { MaybeEditor } from '../maybe-editor';
+import Editor from '../editor';
 import SidebarNavigationScreenTemplatesBrowse from '../sidebar-navigation-screen-templates-browse';
 
 export const templateItemRoute = {
@@ -9,7 +9,7 @@ export const templateItemRoute = {
        path: '/wp_template/*postId',
        areas: {
                sidebar: <SidebarNavigationScreenTemplatesBrowse backPath="/" />,
-               mobile: <MaybeEditor />,
-               preview: <MaybeEditor />,
+               mobile: <Editor />,
+               preview: <Editor />,
        },
 };

Perhaps this comment might give us a clue:

I've worked it out. In order to decide whether to show the back button, we rely on usePrevious, which stores a ref. Because the areas.preview component changes between the home route and the template-part-item route, the ref gets reset, so there's no previous and the back button doesn't get shown.

@stokesman
Copy link
Contributor Author

stokesman commented Feb 6, 2025

Thanks Aki, that comment is helpful. It’s interesting that it was in reply to your finding on how an e2e test failure was due to a missing back button #66851 (comment). It seems like there’s a lack of coverage for a similar scenario with pages and this could have been caught.

More from that same comment:

We could potentially use HomeViewPreview on every route that uses the editor and can be accessed from the canvas, so the component doesn't change, and call it something generic like MaybeEditor. I'm not sure if there's a better solution, open to ideas!

It looks like that idea was used for the solution and I found that applying the same idea for the page item route fixed this in my testing.

Diff
diff --git a/packages/edit-site/src/components/site-editor-routes/page-item.js b/packages/edit-site/src/components/site-editor-routes/page-item.js
index c20720316b..67738c5048 100644
--- a/packages/edit-site/src/components/site-editor-routes/page-item.js
+++ b/packages/edit-site/src/components/site-editor-routes/page-item.js
@@ -6,9 +6,9 @@ import { __ } from '@wordpress/i18n';
 /**
  * Internal dependencies
  */
-import Editor from '../editor';
 import DataViewsSidebarContent from '../sidebar-dataviews';
 import SidebarNavigationScreen from '../sidebar-navigation-screen';
+import { MaybeEditor } from '../maybe-editor';
 
 export const pageItemRoute = {
 	name: 'page-item',
@@ -21,7 +21,7 @@ export const pageItemRoute = {
 				content={ <DataViewsSidebarContent postType="page" /> }
 			/>
 		),
-		mobile: <Editor />,
-		preview: <Editor />,
+		mobile: <MaybeEditor />,
+		preview: <MaybeEditor />,
 	},
 };

I'm not sure this is the ideal solution or if there are potentially some other routes where it ought to be applied.

@t-hamano
Copy link
Contributor

t-hamano commented Feb 7, 2025

Thanks for your detailed investigation, I think I'm starting to understand what's causing this.

It looks like the best approach would probably be to centralize all the logic into the base editor (EditSiteEditor) to avoid rendering different components depending on the route.

I've started trying this approach in #69093.

@t-hamano t-hamano self-assigned this Feb 7, 2025
@t-hamano t-hamano moved this to 🏗️ In Progress in WordPress 6.8 Editor Tasks Feb 7, 2025
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Feb 7, 2025
@stokesman
Copy link
Contributor Author

I've started trying this approach in #69093.

That makes sense. I came up with another approach that I think might be favorable #69103. It makes the code concerned with the back button more robust (remounts won’t affect it). So the branching for the classic theme preview can be kept to just the home route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: 🏗️ In Progress
5 participants