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

Fix drag and drop in chrome when the document has iframes #9511

Merged
merged 3 commits into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@nosolosw
Member

nosolosw commented Aug 31, 2018

Fixes #6145

In Chrome, the dragend event won't be dispatched if an iframe is affected by the drag operation (moved as a result of dragging other nodes, etc). Chrome bug.

Testing

  • Create a post that contains an iframe. For example, you can add an embed block (twitter, youtube video, etc).
  • Drag it to several positions.
  • Drag other elements above and below it.

The expected result is that the drag-and-drop operation works and it's not stuck mid-drag.

@nosolosw nosolosw self-assigned this Aug 31, 2018

@nosolosw nosolosw added the [Type] Bug label Aug 31, 2018

@nosolosw nosolosw requested a review from youknowriad Aug 31, 2018

@nosolosw nosolosw added this to the 3.8 milestone Aug 31, 2018

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Aug 31, 2018

Member

Note that #9311 refactors the Draggable component. This PR will be rebased once the former is merged.

Member

nosolosw commented Aug 31, 2018

Note that #9311 refactors the Draggable component. This PR will be rebased once the former is merged.

@@ -57,6 +64,13 @@ class Draggable extends Component {
this.cursorTop = event.clientY;
}
onDrop( ) {

This comment has been minimized.

@nosolosw

nosolosw Aug 31, 2018

Member

Note that, as per the spec and tested in practice, the drop event is fired before the dragend event. So, an alternative approach to fix this would be to use the drop event in every case, as it is fired reliably AFAIK. That would require changing Draggable API to use the onDrop instead.

Because this bug only affect Chrome if the document has iframes, I feel more comfortable trying to limit the hack to that particular use case. I hope the Chrome team fix this at some point and if they do, we can remove the hack without affecting Draggable's API.

@nosolosw

nosolosw Aug 31, 2018

Member

Note that, as per the spec and tested in practice, the drop event is fired before the dragend event. So, an alternative approach to fix this would be to use the drop event in every case, as it is fired reliably AFAIK. That would require changing Draggable API to use the onDrop instead.

Because this bug only affect Chrome if the document has iframes, I feel more comfortable trying to limit the hack to that particular use case. I hope the Chrome team fix this at some point and if they do, we can remove the hack without affecting Draggable's API.

nosolosw added some commits Aug 31, 2018

Fix chrome bug related to dragevent
See https://bugs.chromium.org/p/chromium/issues/detail?id=737691#c8
In Chrome, the dragend won't be dispatched is an iframe is affected
by the drag operation (moved as a result of dragging other nodes,
etc).

@youknowriad youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 7, 2018

Contributor

Tested and confirmed the fix but I think design wise, it's not great. I wonder if we should show the block icon as a drag handle instead of removing the iframes which can cause weird states.

cc @jasmussen

Contributor

youknowriad commented Sep 7, 2018

Tested and confirmed the fix but I think design wise, it's not great. I wonder if we should show the block icon as a drag handle instead of removing the iframes which can cause weird states.

cc @jasmussen

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 7, 2018

Contributor

Oh I guess the iframe removal is already in master, so I guess I'm just going to approve here and we'll see if we can improve the drag handle for blocks with iframes later.

Contributor

youknowriad commented Sep 7, 2018

Oh I guess the iframe removal is already in master, so I guess I'm just going to approve here and we'll see if we can improve the drag handle for blocks with iframes later.

@youknowriad

LGTM 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 7, 2018

Contributor

Given designs, perhaps we should merge this along wiht the improvements that are scheduled in add/drag-handle?

Contributor

jasmussen commented Sep 7, 2018

Given designs, perhaps we should merge this along wiht the improvements that are scheduled in add/drag-handle?

@nosolosw nosolosw referenced this pull request Sep 11, 2018

Closed

Drag & Drop stalls #9766

@nosolosw nosolosw merged commit 87a5e2d into master Sep 12, 2018

2 checks passed

codecov/project 50.33% (-0.04%) compared to 4528ee3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nosolosw nosolosw deleted the fix/dnd-stuck-mid-drag branch Sep 12, 2018

@nosolosw nosolosw referenced this pull request Sep 12, 2018

Merged

Add/drag handle #9569

10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment