Fix: Draggable component depends on an element with id editor being available. #15472
Conversation
jorgefilipecosta
added
[Type] Bug
jorgefilipecosta
requested review from
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
noisysocks,
talldan and
youknowriad
as code owners
| const documentHasIframes = ( ) => [ ...document.getElementById( 'editor' ).querySelectorAll( 'iframe' ) ].length > 0; | ||
| const documentHasIframes = () => { | ||
| return !! invoke( document, [ 'body', 'querySelector' ], 'iframe' ); | ||
| }; |
There was a problem hiding this comment.
@nosolosw Any reason this check was editor specific? Is it because WP can use iframe outside of the "content" canvas (maybe in menus, topbars)? What impact could this have?
There was a problem hiding this comment.
Exactly. My old self did document this as problematic: #9511 (comment)
If we do this, what will happen in chrome browsers that don't have the fix yet (<72 if I'm reading correctly the upstream bugfix?) is that it'll fire the onDrop and then onDragEnd, effectively executing the onDragEnd property twice.
An alternative fix would be to keep track of whether the drag operation was already reseted keeping a isDragging variable similar to isChromeAndHasIframes and reset the drag state & call the onDragEnd prop only if it still wasn't done.
There was a problem hiding this comment.
@nosolosw, @youknowriad Can we just remove this logic and assume the problem is fixed in chrome? I guess the last two versions that we need to support already have this problem fixed given that the problem was fixed in Wed, Nov 21, 2018, and chrome updates are fairly regular.
There was a problem hiding this comment.
@jorgefilipecosta That makes sense to me
There was a problem hiding this comment.
Agreed to that: #15665 (wanted to test this effectively had been fixed so I had to prepare the revert anyway).
oandregal
added
the
[Feature] Drag and Drop
|
Closing this PR, with the changes being made in #15665 this PR is irrelevant. |
jorgefilipecosta
deleted the
fix/draggable-component-depends-on-a-element-with-id-editor-being-available
branch
If the Draggable component was used in a document without an element with "editor" id, the component crashed when the user tried to use it.
The components should be as generic as possible and not relying on specific elements being available.
How has this been tested?
I cherry-picked the commit from #15470 that makes sure the movers can be used (only required if testing before that PR is merged).
I went to /wp-admin/admin.php?page=gutenberg-widgets.
I added some blocks I tried to drag them and verified the draggable element did not crash because of an element with id editor not being found. (dropping blocks still does not work because of another unrelated problem).