Fix grid item resizing in non-iframed editor. #61636
Conversation
tellthemachines
added
[Type] Bug
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +34 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the quick follow-up!
This is testing nicely for me in the non-iframed post editor and I've been unable to reproduce the editor locking up with this PR applied 👍
From the looks of it, it's intentional that this PR doesn't address the issues with the iframed editor, so I imagine the sizing of the resizer in the following screengrab is to be looked at separately:
2024-05-14.11.02.28.mp4
Just thought I'd mention it just in case.
Assuming that's a known issue, then this change LGTM, but might be worth getting a second opinion from @noisysocks, too 🙂
packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
Outdated
Show resolved
Hide resolved
Why does
I don't understand the height discrepancy and the logic needed to accomodate it. Why can't we set |
It does select the document body, and that's why the error occurs. I'm not sure why it happens but looks like it might be related to the abortController used to remove the event listener, because my console shows The error doesn't happen when selecting
The |
|
Sorry for taking so long to review. I really wanted to understand what's going on here... 😅
I did some testing and the infinite recursion happens because calling The simplest fix I think is to remove the diff --git a/packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js b/packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
index bc82cb9d8e..0eb79984d3 100644
--- a/packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
+++ b/packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
@@ -164,9 +164,8 @@ function GridItemResizerInner( {
* isn't directly above the handle, so we try to detect if it happens
* outside the grid and dispatch a mouseup event on the handle.
*/
- const rootElementParent =
- rootBlockElement.closest( 'body' );
- rootElementParent.addEventListener(
+ controller.abort();
+ event.target.ownerDocument.addEventListener(
'mouseup',
() => {
event.target.dispatchEvent(
I assume this fix works because the I don't really like targeting
Right, gotcha. I wonder if we can simplify this by passing |
That's a good point. If that fix works for you locally, would you like to push it to this PR?
I tried that already and it doesn't work. Both the visualizer and the resizer re-render whenever we select a grid item to resize, and that throws off the calculation. |
|
You beat me to it! Looks good. |
|
Flaky tests detected in 3e1f425. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9073755112
|
What?
Fixes one of the issues described in #61633: the non-iframed editor crashing when using the grid resizer, by moving the mouseup event listener that was added to
body(which was meant to target the iframe body element) to.editor-styles-wrapperwhich will target the root editor canvas element in both iframed and non-iframed scenarios.Also fixes a discrepancy in the grid resizing bounds for the non-iframed editor by reintroducing a dummy div to attach the resizer ref to, which was originally removed in #60986 (comment). The problem is that attaching the ref to any of the child components of
GridItemResizerInnerdoesn't guarantee it'll actually be in the DOM when we need it. For context, the ref is needed to calculate the height discrepancy between the grid item and its resizer, in the scenario where the editor is iframed and they belong to different documents.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast