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

Options popover opens behind sidebar panels unless you first interact with Editor content [6.2] #47923

Closed
tatasha2004 opened this issue Feb 9, 2023 · 24 comments
Labels
[Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@tatasha2004
Copy link

Description

Options sidebar opened under Yoast sidebar (WP version nightly 6.2-beta1-55300)

Step-by-step reproduction instructions

  1. WP Beta Tester and Yoast SEO 20.1 are installed and activated
  2. WP updated to 6.2 nightly
  3. Edit post
  4. Click on Options (WP settings sidebar should be opened) or open the Yoast sidebar and then try to open the Options sidebar
    Actual result: Options sidebar opened under Yoast sidebar

Note:
With Gutenberg trunk:
-No issues.
Without Gutenberg:
The panel is under the Yoast sidebar

Screenshots, screen recording, code snippet

image

Environment info

mac Os 13.1
Chrome Version 110.0.5481.77
nginx
PHP 8.1.9
WP 6.2-beta1-55300
Beta tester 3.2.7
Yoast SEO 20.1

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

@Mamaduka Mamaduka added the [Type] Plugin Interoperability Incompatibilities between a specific plugin and the block editor. Close with workaround notes. label Feb 9, 2023
@ndiego
Copy link
Member

ndiego commented Feb 9, 2023

I have been able to replicate it, but the issue is not unique to the Yoast sidebar. The options panel seems to appear behind all sidebars unless you first interact with the Editor.

Image

@ndiego ndiego added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Feb 9, 2023
@ndiego ndiego changed the title Options sidebar opened under Yoast sidebar (WP version nightly 6.2-beta1-55300) Options popover opens behind sidebar panels unless you first interact with Editor content [6.2] Feb 9, 2023
@t-hamano
Copy link
Contributor

I have confirmed that this problem is not reproduced in the latest Gutenberg trunk.
Maybe some backport is needed.

@t-hamano
Copy link
Contributor

This problem seems to be occurring with all header area popovers, not just option popovers:

  • Option popover hides behind
  • Preview popover hides behind
  • Tools popover hides behind
  • Button tooltip hides behind
  • Focusing the block on any one of the blocks in the editor will fix this problem.
  • Not reproduced if the latest Gutenberg is enabled
  • Tested with WordPress 6.2-beta1-55295

I would mark this as high priority.

fc0b1b4a8b0d17ae8b71e0edd5c04555.mp4

@t-hamano t-hamano added [Priority] High Used to indicate top priority items that need quick attention and removed [Type] Plugin Interoperability Incompatibilities between a specific plugin and the block editor. Close with workaround notes. labels Feb 10, 2023
@ntsekouras
Copy link
Contributor

This is very weird and I couldn't pinpoint any PR that affected that and works in GB. I tested with wp/6.2 branch and it seems to work even with the commit for packages release. So unless I missed something it seems something php related wasn't back ported? 🤔

@t-hamano
Copy link
Contributor

This problem does not seem to be limited to header areas such as option popover.

List view popover not coming to the forefront

list-sidebar

Popover in the post sidebar is cut off and also not coming to the forefront

post-sidebar

I have also noticed that when this problem occurs, the popovers are rendered in different positions.

When a problem occurs, it is rendered next to the toggle button:

before

When there is no problem (after first interact with editor content), the popover is rendered outside the button, not next to it:

after

@ndiego
Copy link
Member

ndiego commented Feb 14, 2023

Just confirming that this remains an issue in Beta 2.

@franzaurus
Copy link

franzaurus commented Feb 15, 2023

I recorded it on Replay.io debugger if this could be of any help. https://app.replay.io/recording/replication-of-issue-47923--7f924d91-ae86-44a3-9667-768a14c17076

This is in WP 6.2 Beta 2.

@t-hamano
Copy link
Contributor

I have discovered that the problem probably involves the components package. The problem can be reproduced in the latest version of Gutenberg by adding the following code:

diff --git a/lib/client-assets.php b/lib/client-assets.php
index 0f6e64c27c..cd868b2711 100644
--- a/lib/client-assets.php
+++ b/lib/client-assets.php
@@ -200,6 +200,10 @@ function gutenberg_register_packages_scripts( $scripts ) {
                // For example, `…/build/a11y/index.min.js` becomes `wp-a11y`.
                $handle = 'wp-' . basename( dirname( $path ) );
 
+               if ( $handle === 'wp-components' ) {
+                       continue;
+               }
+
                // Replace extension with `.asset.php` to find the generated dependencies file.
                $asset_file   = substr( $path, 0, -( strlen( '.js' ) ) ) . '.asset.php';
                $asset        = file_exists( $asset_file )

@annezazu
Copy link
Contributor

If it is components related, perhaps @ciampo can chime in!

@ciampo
Copy link
Contributor

ciampo commented Feb 15, 2023

Thank you for the ping!

I have discovered that the problem probably involves the components package. The problem can be reproduced in the latest version of Gutenberg by adding the following code:

If I understand correctly @t-hamano 's message, his findings imply that:

  • the version of the Gutenberg plugin shipped with WordPress 6.2 beta has the bug
  • but the version of the Gutenberg plugin shipped with WordPress 6.1 doesn't have it

Considering that @t-hamano and @ntsekouras also confirmed that the bug can't be reproduced on trunk, my initial guess is that one or more PRs that should have been back-ported to the 6.2 release were not back-ported, and that's introducing the bug.

@Mamaduka
Copy link
Member

The difference between the plugin and core behavior is that theslot.ref is undefined in the Popover component, and dialogs aren't rendered inside the popover slot. This creates a problem with stacking context.

I'm starting to believe this has something to do with the core build process.

Why?

The bug can't be reproduced on the release branch (wp/6.2). But if I build in the plugin and then override the core dist file for the components package (wp-includes/js/dist/components.js) - the bug is fixed!

These two builds should be identical.

@talldan
Copy link
Contributor

talldan commented Feb 17, 2023

Could it be related to the process.env.IS_GUTENBERG_PLUGIN variable? It'd be true when building in the plugin, but false for core.

edit: from what I can tell it's not related, but I'll keep digging.

@talldan
Copy link
Contributor

talldan commented Feb 17, 2023

The difference between the plugin and core behavior is that the slot.ref is undefined in the Popover component

Interestingly, the slot is defined, but the .ref property is missing. Not sure why that would happen. @youknowriad refactored some parts of slot/fill recently and I'm sure knows the code well, so I imagine would have a better idea of how it all works.

I set a breakpoint here and can see that the Slot with the "Popover" name does render and has a ref when it's initially set.

@ntsekouras
Copy link
Contributor

ntsekouras commented Feb 17, 2023

In this line the registry.slots contain the Popover slot, but the value returned from valtio's useSnapshot doesn't.. I tried to see if it has something to do with that library and the init function, but it was quite hard to debug.

I'm starting to believe this has something to do with the core build process.

I have the same impression about this.. I even tried locally latest core trunk with the packages from GB 15.2 and the issue is there.. Maybe @gziolo or @desrosj have any insights?

@talldan
Copy link
Contributor

talldan commented Feb 18, 2023

I have a feeling this is related to the valtio library, I notice that wordpress-develop has a more recent version (the package-lock is showing 1.10.2) compared to gutenberg (package-lock is showing 1.7.0).

If I update Gutenberg to use 1.10.2, then I seem to be able to reproduce the issue, but it'd be good to get corroboration on that.

Seems like there may have been a breaking change in that package.

@pbiron
Copy link

pbiron commented Feb 19, 2023

@tatasha2004 thanx for reporting this. I encountered this problem myself yesterday and did a bunch of testing to try to narrow down when (if not where) the problem was introduced before coming to report the issue. I'll add info on what I tested in case it helps something discover where the problem actually resides and how to fix it.

I originally bumped into the problem using WP 6.2-beta2 (in Win 10 Chrome 110.0.5481.104) without the GB plugin active, and a custom block-based theme.

I then tested:

  • WP 6.1.1 w/o the plugin active
  • WP 6.2-beta1 w/ and w/o the plugin active
  • WP 6.2-beta2 w/ and w/o the plugin active

In each of those, I used a bundled block-based theme (2023) and a bundled classic theme (2021), as well custom block-based and custom classic themes.

The theme being used made no difference as to whether the bug surfaced.

The only time the bug surfaced was when running both of the 6.2 betas w/o the plugin active.

Hope this helps.

@gziolo
Copy link
Member

gziolo commented Feb 20, 2023

I have a feeling this is related to the valtio library, I notice that wordpress-develop has a more recent version (the package-lock is showing 1.10.2) compared to gutenberg (package-lock is showing 1.7.0).

If I update Gutenberg to use 1.10.2, then I seem to be able to reproduce the issue, but it'd be good to get corroboration on that.

Seems like there may have been a breaking change in that package.

In theory, valtio didn’t have any breaking changes according to release history, but they performed internal refactoring, and several bug fixes, between the versions that differ.

I think @talldan identified the issue correctly. There are two options at the moment:

  • we quickly pin valtio to 1.7.x and check if that helps
  • we upgrade to ^1.10.2 and make the code work with the changes applied

Option 2 would be preferred if it doesn’t come with too many changes risky for the major WP release. It will be necessary anyway in trunk as it impacts 3rd party package usage.

@Mamaduka
Copy link
Member

@gziolo, maybe we can go with option one for next beta, keep the issue open and work on the upgrade separately.

What do you think?

@gziolo
Copy link
Member

gziolo commented Feb 20, 2023

@Mamaduka, definitely the simplest approach for this week 💯

@youknowriad
Copy link
Contributor

Interesting issue. Thanks all for the debugging, I'm 👍 with pinning valtio version for now until we figure this out.

@t-hamano
Copy link
Contributor

t-hamano commented Feb 22, 2023

I have confirmed that this issue is not reproduced in WordPress 6.2-beta3-55401 without Gutenberg enabled.

@pbiron
Copy link

pbiron commented Feb 22, 2023

I have confirmed that this issue is not reproduced in WordPress 6.2-beta3-55401 without Gutenberg enabled.

you beat me to it :-) I was just coming to say the same thing...now works fine for me

thanx everyone!

@ndiego
Copy link
Member

ndiego commented Feb 22, 2023

Works for me as well in Beta 3 with and without Gutenberg active. Given the number of confirmations that this issue is resolved, I am going to close this out. However, if anyone continues to experience popover issues, please let us know, and I will reopen. Thanks!

@ndiego ndiego closed this as completed Feb 22, 2023
@Mamaduka
Copy link
Member

@ndiego, we might want to create a new issue for the valtio library upgrade. Currently, we're "hotfixing" the problem by pinning the specific version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
No open projects
Development

No branches or pull requests