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

WordPress 6.6 RC3 and Query Loop block issue (When WooCommerce is active) #63448

Closed
sergiu-radu opened this issue Jul 11, 2024 · 10 comments · Fixed by #63565
Closed

WordPress 6.6 RC3 and Query Loop block issue (When WooCommerce is active) #63448

sergiu-radu opened this issue Jul 11, 2024 · 10 comments · Fixed by #63565
Assignees
Labels
[Feature] Component System WordPress component system [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@sergiu-radu
Copy link

sergiu-radu commented Jul 11, 2024

Description

When switching to tablet mode and back to desktop mode while the "Post Template" block is selected - the block breaks.

This seems to be somehow related to WooCommerce plugin.

Here is a quick view demonstration:
https://github.com/WordPress/gutenberg/assets/8998558/5d0bdd93-f7af-4293-b3c0-638e0a930941

Everything works fine if WooCommerce is deactivated.

Step-by-step reproduction instructions

  1. Install WooCommerce plugin (I have the latest version)
  2. Create a new page and insert the Query Loop block
  3. Open the "Document Overview" sidebar
  4. Select/click on the "Post template" block (this is important, see the attached video too)
  5. Ensure the Post Template block is set to 'Grid' layout on the toolbar
  6. Change the "View" to tablet and back to desktop

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress version 6.6 RC3
  • Reproduced on any theme
  • The only active plugin is WooCommerce

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

@sergiu-radu sergiu-radu added the [Type] Bug An existing feature does not function as intended label Jul 11, 2024
@talldan
Copy link
Contributor

talldan commented Jul 12, 2024

@sergiu-radu Thanks for reporting this. Out of interest, have you also flagged this bug with WooCommerce? The plugin seems to change the way the mobile/tablet/desktop views work quite significantly. The views seem to completely 'reload', whereas you compare it to when WooCommerce is deactivated and there's a smooth animation between mobile and desktop. That's part of the reason the bug happens.

However, with the Gutenberg plugin active the bug doesn't happen, so it makes it a little hard to repro/fix.

Anyway, the main cause is the grid visualizer for some reason that the post template block has (the grey outlines of the columns). It uses a Popover component, which also uses the floating-ui library. When switching back to Desktop it tries to recompute its position, and because of the way WooCommerce completely replaces the viewport content, it does so using DOM elements that are detached from the window, which causes the error.

I think there's some extra safety that can be added to our Popover code to prevent the bug.

@talldan talldan added the [Feature] Component System WordPress component system label Jul 12, 2024
@sergiu-radu
Copy link
Author

Hello @talldan, thank you very much for replying to this and for the detailed explanation.

Out of interest, have you also flagged this bug with WooCommerce?
No, I didn't, I initially thought this is a Gutenberg/WordPress issue related to the Query Loop block only.

I know that WooCommerce is changing the viewport by removing the iframe on desktop and it's really sad that they still didn't fixed/improved this somehow until now but maybe you can add a "check" in the Query Loop block or Popover so it won't get executed if it doesn't see/find the right viewport (just my 2 cents).

Anyway, I hope you will be able to find a solution and include it in the 6.6 release.

Thank you and have a wonderful day!

@talldan
Copy link
Contributor

talldan commented Jul 12, 2024

Unfortunately it's too late now for WordPress 6.6, but potentially a 6.6.1.

@sergiu-radu
Copy link
Author

Hey @talldan, I understand, thanks for letting me know.

@talldan
Copy link
Contributor

talldan commented Jul 12, 2024

This is the error message btw:

TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at getComputedStyle (floating-ui.utils.dom.mjs:72:1)
    at isOverflowElement (floating-ui.utils.dom.mjs:40:5)
    at getOverflowAncestors (floating-ui.utils.dom.mjs:123:1)
    at autoUpdate (floating-ui.dom.mjs:540:95)
    at Object.whileElementsMounted [as current] (index.tsx:291:14)
    at floating-ui.react-dom.mjs:223:1
    at commitHookEffectListMount (react-dom.development.js:23184:26)
    at commitLayoutEffectOnFiber (react-dom.development.js:23302:17)
    at commitLayoutMountEffects_complete (react-dom.development.js:24722:9)
    at commitLayoutEffects_begin (react-dom.development.js:24708:7)

@talldan
Copy link
Contributor

talldan commented Jul 15, 2024

Here's some more detail about where the bug happens.

The Gutenberg Popover component calls autoUpdate:

autoUpdate( referenceParam, floatingParam, updateParam, {
layoutShift: false,
animationFrame: true,
} ),

When the bug happens, the referenceParam.contextElement is disconnected from the DOM.

Floating UI calls getOverflowAncestors on the element:
https://github.com/floating-ui/floating-ui/blob/81a7da464c713e3432d1015cafb7566675a22282/packages/dom/src/autoUpdate.ts#L148

This is where things start to go wrong, that function calls isLastTraversableNode on the parent of the reference element (which is another block) and that incorrectly returns true, and so getOverflowAncestors returns the body of that element which is null:
https://github.com/floating-ui/floating-ui/blob/81a7da464c713e3432d1015cafb7566675a22282/packages/utils/src/dom.ts#L158-L162

Then finally because isBody is true (null === null), this line calls isOverflowElement using the null value which triggers the fatal getComputedStyle:
https://github.com/floating-ui/floating-ui/blob/81a7da464c713e3432d1015cafb7566675a22282/packages/utils/src/dom.ts#L50

Jumping back to isLastTraversableNode where I said things start to go wrong, getNodeName returns '#document' because the isNode check fails:
https://github.com/floating-ui/floating-ui/blob/81a7da464c713e3432d1015cafb7566675a22282/packages/utils/src/dom.ts#L3-L11

I think floating ui could have some checks for whether a node is connected somewhere in there, but then they may also consider the idea of calling autoUpdate on a disconnected node to be using the library incorrectly.

@jsnajdr
Copy link
Member

jsnajdr commented Jul 15, 2024

My steps to reproduce:

  1. make sure that when switching between the Desktop and Tablet device type, the editor canvas switches between non-iframe and iframe. You need to make useShouldIframe return false. Simply hardcode that. Then you don't even need to install WooCommerce. Installing WooCommerce only disables the iframe because the plugin has blocks with apiVersion < 3. You can also have the Gutenberg plugin installed, you don't need to use the Core version of the editor.
  2. create a Post Template block or any other block that has the grid layout type.
  3. select that block and switch it to grid mode (in block toolbar) so that GridVisualizer is rendered.
  4. switch to Tablet view and back to Desktop
  5. the editor crashes when switching from Tablet (iframe) to Desktop (non-iframe)

This happens with GridVisualizer popover, and not with the BlockToolbar popover. Because with GridVisualizer both the reference (anchor) and floating elements are inside the iframe. While with BlockToolbar the reference is inside the iframe and the floating element is outside.

The root cause is the useBlockElement( clientId ) hook. This hook returns the block's element, but update in the return value is delayed. If you unmount an old UI for a given block (inside iframe) and then render a new UI (outside iframe), then useBlockElement( clientId ) is still returning the old element (inside iframe). Because all the ref callbacks and state updates run only after the render.

If you have a variable that points on an element that's inside an iframe that has been already deleted, it has some funny properties. el.ownerDocument.defaultView is null and el.ownerDocument.body is also null. That confuses the floating-ui library and it crashes, namely its getOverflowAncestors function.

FYI @ellatrix because useBlockElement is involved. I verified that this was not caused by #60945, the bug and crash was there even before that PR.

@hellofromtonya
Copy link
Contributor

Hello @sergiu-radu 👋 I'm gathering the list of contributors who contributed to 6.6.1 (i.e. for giving each person credit). I'm not able to locate your WordPress profile. Maybe your GitHub profile is not linked to it or you don't yet have a WordPress profile.

Can you please share your profile username or the https://profiles.wordpress.org/ link? If you don't yet have a WordPress profile, please consider registering and linking your GitHub profile to it.

Thank you for contributing and following up with me.

@sergiu-radu
Copy link
Author

Hello @hellofromtonya,
I have a WordPress profile but it has my company name on it, so I am not sure if this will be ok for you.

Here it is - https://profiles.wordpress.org/creativethemeshq/

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Jul 23, 2024

@sergiu-radu, Thank you. Contribution prop / credit is given for a person, not a company. Can you please create a WP profile for yourself and then link it to your GitHub username / profile?

Some examples: You can create it using the same GitHub username sergui-radu or append with your company name such as sergui-radu-creativethemes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants