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

useDisabled hook causes infinite loop #40845

Closed
Inwerpsel opened this issue May 5, 2022 · 5 comments · Fixed by #40649
Closed

useDisabled hook causes infinite loop #40845

Inwerpsel opened this issue May 5, 2022 · 5 comments · Fixed by #40649
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Performance Related to performance efforts

Comments

@Inwerpsel
Copy link

Inwerpsel commented May 5, 2022

Description

Usage of the useDisabled hook can result in infinite loops. I found this on the Post Template block where it disables the inputs of the none selected list items. It has a heavy impact on editor performance.

It seems like the listener here is doing attribute updates that trigger the same listener. See screenshots of the Call Stack below.

If I check the mutation record the function receives, it always has null for the previous tabIndex value. (could be unrelated)

Screenshot from 2022-05-05 13-57-59

Screenshot from 2022-05-05 14-03-17

Screenshot from 2022-05-05 14-46-56

Step-by-step reproduction instructions

I don't know if there's any preconditions about our setup that could trigger this. From a first look it seems like it should happen for anyone.

  1. Add a Query Loop block in the editor, with a block that has focusable elements (for example Post Excerpt).
  2. Set up the Query Loop so that there's more than 1 result.
  3. Run a performance recording with your browser's dev tools. Or put a breakpoint in the listener.
  4. See how the listener is triggering itself continuously.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress: Tested on both 5.9 and 6.0-RC1
Chrome: Version 101.0.4951.54 (Official Build) (64-bit)

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

Yes

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

Not yet

@Mamaduka
Copy link
Member

Mamaduka commented May 5, 2022

@youknowriad, I think you fixed the loop issue in #40649, correct?

@youknowriad
Copy link
Contributor

I fixed one loop issue but I can't confirm 100% whether it fixes this one as well :P

@Mamaduka
Copy link
Member

Mamaduka commented May 5, 2022

I was able to reproduce the issue on both 5.9.3 without the Gutenberg plugin. I think it was the problem with the original useDisabled hook + loop issue fixed in #40649.

I'm no longer able to reproduce the issue when testing on the update/block-overlay branch.

@Mamaduka Mamaduka added [Type] Performance Related to performance efforts [Block] Query Loop Affects the Query Loop Block labels May 5, 2022
@Inwerpsel
Copy link
Author

Using a mutation observer with React and letting that observer itself mutate the nodes it's observing seems like an inherently fragile setup. I'm not saying the linked PR doesn't address this concern, but it seems there's still good reason for prudence before starting to make wide us of useDisabled, especially on reusable blocks.

There's a lot of complexity involved in how it will in practice behave for a particular block on a particular site with particular theme and plugins, and probably still some cross browser/OS differences to worry about.

At this moment the useDisabled is only used in a few quite specific cases: for post comments and the query loop post template. So in practice it's only running with a limited set of core blocks most of the time. And those blocks mostly have dynamic content that is not entered by the user, so a more limited set of interactions / editor elements. And even in this limited usage it already surfaced performance issues.

Once the hook is used for the reusable block overlay, it will, overnight, start being used in practice not only with every possible core block, but also every possible third party block, in every possible combination.

@youknowriad
Copy link
Contributor

The hook is also used in the Disabled component which is used in all block previews and in a few other blocks. I'm fine with a better implementation if you have any ideas on how to approach this without a mutation observer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants