Block draggable chip's position is incorrect when dragging over an iframe

[andrewserong]

Description

The drag chip (the little icon that follows the mouse cursor around) is incorrectly positioned when dragging a block over the site editor canvas, or the post editor canvas when the post editor is iframed (i.e. running TT3 theme with core blocks).

This appears to be to do with the Draggable component's positioning when dragging over an iframe in particular.

Step-by-step reproduction instructions

1. Open the site editor
2. Go to edit a template or the site home
3. Open the block inserter
4. Drag a paragraph block to the editor canvas
5. As you drag over the editor canvas, notice that the drag chip position flicks suddenly to the edge of the browser window

Screenshots, screen recording, code snippet

2023-10-05.16.55.20.mp4

The same issue appears to happen with the Draggable component in Storybook if you use the Docs view that shows all stories. This is a good way to test it because each preview occurs in an iframe, too, it seems. The URL for testing the Draggable component in Storybook is: https://wordpress.github.io/gutenberg/?path=/docs/components-draggable--docs

2023-10-05.16.57.57.mp4

Environment info

• Gutenberg trunk on WP 6.3.1 (without GB running, the position is correct in 6.3.1)
• WP 6.4 Beta 1 (without GB running)

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

[andrewserong]

I'm not sure what's caused this one, but from logging out the event in the Draggable component's over function, it does appear that e.clientX / e.clientY "reset" when hoving over an iframe so that the coords are relative to the iframe instead of the page 🤔

This is where I was looking:

gutenberg/packages/components/src/draggable/index.tsx

Lines 188 to 203 in f36f4cd

	function over( e: DragEvent ) {
	// Skip doing any work if mouse has not moved.
	if ( cursorLeft === e.clientX && cursorTop === e.clientY ) {
	return;
	}
	const nextX = x + e.clientX - cursorLeft;
	const nextY = y + e.clientY - cursorTop;
	cloneWrapper.style.transform = `translate( ${ nextX }px, ${ nextY }px )`;
	cursorLeft = e.clientX;
	cursorTop = e.clientY;
	x = nextX;
	y = nextY;
	if ( onDragOver ) {
	onDragOver( e );
	}
	}

[ciampo]

Mhh, since Draggable hasn't changed much recently, I wonder what other change could have caused this. Maybe it's something to do with the editor iframe?

Normally iframes should act as a black hole and absorb all pointer events, but our iframe actually bubbles some of these events (as I recently learned). I can see that #54080 introduced some changes to that functionality, I wonder if it could be related, or at least a good starting point.

[t-hamano]

I reported the same issue in #54176, but closed #54176 due to ongoing discussion here.

As @ciampo said, this problem seems to be caused by #54080 (commit hash is f71f153). Checking out the previous commit (a247fe8) resolves the issue.

[andrewserong]

I reported the same issue in #54176, but closed #54176 due to ongoing discussion here.

Oh, thank you @t-hamano! Apologies for missing your earlier report in that issue, I did a quick skim for issues that already flagged this, but my search was not thorough enough 🙂

I can see that #54080 introduced some changes to that functionality, I wonder if it could be related, or at least a good starting point.

Thank you both for identifying this one as the culprit! That's a big help, I was definitely scratching my head trying to figure out what could've changed 😄

I wonder if we can get the Draggable component to provide an offset if the event is coming from an iframe, so that to a user there's no discernible jump? I'll have a play around.

[andrewserong]

Interestingly I found that I can get this issue working properly in the editor again (I think!) if I comment out the check over in bubbleEvent before it applies the offset for the event x/y position:

gutenberg/packages/block-editor/src/components/iframe/index.js

Line 41 in 1ae4d44

if ( event instanceof frame.ownerDocument.defaultView.MouseEvent ) {

Without that check, the position seems okay:

2023-10-06.14.38.13.mp4

I do notice there's a bit of a performance hit when dragging over the iframe, possibly due to the cost of the bubbleEvent function's getBoundingClientRect calls?

I've added a question about the above check in bubbleEvent over in https://github.com/WordPress/gutenberg/pull/54080/files#r1348207960, but I'll just ping @youknowriad here, too, for visibility. It'd be good to better understand the intention behind that check, because superficially at least, it seems that if we can fix that check, we might be able to resolve this issue in an elegant way, without having to change how the Draggable component works.

Happy to continue digging on Monday if that doesn't get us there, though!

[ciampo]

the cost of the bubbleEvent function's getBoundingClientRect calls

I also noticed some jankiness, and calling getBoundingClientRect is very likely the reason. Maybe we could "cache" those measurements and update them only when the window resizes (or something similar) ?

[t-hamano]

I found that I could solve the problem by changing the following:

diff --git a/packages/block-editor/src/components/iframe/index.js b/packages/block-editor/src/components/iframe/index.js
index 9a6371dcaf..badbbfd2b9 100644
--- a/packages/block-editor/src/components/iframe/index.js
+++ b/packages/block-editor/src/components/iframe/index.js
@@ -38,7 +38,7 @@ function bubbleEvent( event, Constructor, frame ) {
                init[ key ] = event[ key ];
        }
 
-       if ( event instanceof frame.ownerDocument.defaultView.MouseEvent ) {
+       if ( event instanceof frame.contentDocument.defaultView.MouseEvent ) {
                const rect = frame.getBoundingClientRect();
                init.clientX += rect.left;
                init.clientY += rect.top;

Before being refactored in #54080, it appears to be checking if the current event is an instance of the iframe document itself. On the other hand, the refactored code seems to check if the event is an instance of the iframe's parent document.

So should we check the current document (currentDocument) instead of the parent document (ownerDocument)?

[andrewserong]

@t-hamano we both found the same thing at the same time! I have a PR up over in #55150 😄

Note: it doesn't address the issue of Storybook in the Docs view, since Storybook doesn't receive the offset logic in our Iframe component. I reckon we could leave that issue aside for now, as fixing that would likely involve moving the offset logic to the Draggable component, and it'd probably be better for us to go with a simple fix in the short-term for WP 6.4.