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

useToolsPanel: calculate menuItems in layout effect to avoid painting intermediate state #65494

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Sep 19, 2024

Fixes a visual glitch when displaying a tools panel in style editor. Go to global styles editor for individual blocks and in the list of blocks, click on Paragraph. At first, toolbar items with no content will appear:

tools-panel-empty

only after a moment the full UI is rendered:

tools-panel-final

The cause of this is how the state of the panel is updated, calculating derived state in a cascade of effects:

const [ a, setA ] = useState();
const [ b, setB ] = useState();
const [ c, setC ] = useState();

useEffect( () => {
  setB( ... );
}, [ a ] );

useEffect( () => {
  setC( ... );
}, [ b ] );

This needs several rerenders and paints until the final state is achieved.

In our case, the state involved is the menuItems object and the isShown value derived from it.

This PR fixes that by using layout effect, so that all the updates happen without an intermediate paint that reveals the non-final UI state. See also #56536 which applied exactly the same fix at another place.

Only the first commit is needed to actually fix the glitch. The rest is using useLayoutEffect also for other state-updating effects in the same file. And removing some unneeded dependencies from useCallback and useEffect calls, namely state setters.

The next step could be a bigger refactoring to make the state updates more streamlined. We don't need effects: they only trigger rerenders. We could use useMemo or, for state updates that need to see the previous state, a useReducer.

@jsnajdr jsnajdr added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Sep 19, 2024
@jsnajdr jsnajdr self-assigned this Sep 19, 2024
@jsnajdr jsnajdr requested a review from ajitbohra as a code owner September 19, 2024 14:19
@ciampo
Copy link
Contributor

ciampo commented Sep 19, 2024

The changes make sense to me, although they seem to cause errors in unit tests. I guess we'd need to understand if we can improve the performance and timing of the updates, while keeping the expected behavior.

Only the first commit is needed to actually fix the glitch. The rest is using useLayoutEffect also for other state-updating effects in the same file. And removing some unneeded dependencies from useCallback and useEffect calls, namely state setters.

Maybe removing the second commit would fix the unit tests?

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 19, 2024

they seem to cause errors in unit tests.

I fixed that by using layout effects also in ToolsPanelItem, so that the order of execution stays the same. The tool panels are a maze of fragile mutual updates, the code deserves a thorough review and refactor.

@ciampo
Copy link
Contributor

ciampo commented Sep 20, 2024

The tool panels are a maze of fragile mutual updates, the code deserves a thorough review and refactor.

Yeah, I got to the same conclusion while reviewing the PR. I think that moving the logic to a reducer could be a way to centralise state and logic a bit more.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Kapture.2024-09-20.at.11.37.20.mp4

Notes / follow-ups:

  • as mentioned earlier, ToolsPanel could benefit from an internal refactor. The logic is currently fragmented in a lot of places, making it fragile and hard to parse;
  • I noticed a considerable delay when navigating to the block screen. We should look into how to take away that delay, either by improving loading performance of the screen, or by making it non-blocking (maybe show a skeleton UI while the real data is loading and a suspense boundary?)

@jsnajdr jsnajdr merged commit 5e37a13 into trunk Sep 20, 2024
62 checks passed
@jsnajdr jsnajdr deleted the fix/tools-panel-computed-state branch September 20, 2024 09:43
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 20, 2024
@ciampo ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2024
@github-actions github-actions bot removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2024
gutenbergplugin pushed a commit that referenced this pull request Sep 20, 2024
… intermediate state (#65494)

* useToolsPanel: calculate menuItems in layout effect to avoid painting intermediate state

* useToolsPanel: remove setState deps, calculate derived values in layout effects

* ToolsPanelItem: also use layout effect to prevent loops

* Changelog entry

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Sep 20, 2024
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 2cce429

@youknowriad
Copy link
Contributor

Hello folks, this is causing a big error for me, when trying to edit an empty page in the site editor (just typing within an empty post content block). I think it's causing some kind of infinite loop. Reverting this PR solves the issue.

@youknowriad
Copy link
Contributor

youknowriad commented Sep 20, 2024

Also worth noting this PR caused an increase in "block select" metric numbers.

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 20, 2024

There are infinite loops when the tools panel is used in combination with slots and fills. I tested only in Site Editor, in one specific panel that is not extensible. I missed the fatal errors this causes elsewhere.

Also worth noting this PR caused an increase in "block select" metric numbers.

Interesting. That metric is measuring how long a focus or focusin event handlers take to execute. And this PR makes the tools panels render more "synchronously". Instead of rendering an empty placeholder initially and being populated with real content only after a cascade of useEffects, the render is now immediate.

When preparing this PR for a second try, we should try to render the tools panel as a "low priority" update.

@stokesman
Copy link
Contributor

The next step could be a bigger refactoring to make the state updates more streamlined. We don't need effects:

Maybe we could take it one effect at a time. I happened to have #61694 which removes one and tests well for me.

@ciampo
Copy link
Contributor

ciampo commented Sep 23, 2024

I personally like the approach in #65564 as it's guaranteeing that all updates happen atomically, but happy to hear everyone's thoughts.

@stokesman
Copy link
Contributor

I personally like the approach in #65564

Me too. I hadn’t seen it when I made my prior comment and wish to add that the PR I mentioned is complementary and not an alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants