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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Sep 12, 2018
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18d9a6a
Fix chrome bug related to dragevent
oandregal Aug 31, 2018
d042701
Scope iframe search to editor component
oandregal Aug 31, 2018
24241c7
Check both chrome and Chrome
oandregal Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 32 additions & 2 deletions packages/components/src/draggable/index.js
View file
Open in desktop
Expand Up @@ -16,14 +16,20 @@ const cloneWrapperClass = 'components-draggable__clone';
const cloneHeightTransformationBreakpoint = 700;
const clonePadding = 20;

const isChromeUA = ( ) => /Chrome/i.test( window.navigator.userAgent );
const documentHasIframes = ( ) => [ ...document.getElementById( 'editor' ).querySelectorAll( 'iframe' ) ].length > 0;

class Draggable extends Component {
constructor() {
super( ...arguments );

this.onDragStart = this.onDragStart.bind( this );
this.onDragOver = this.onDragOver.bind( this );
this.onDrop = this.onDrop.bind( this );
this.onDragEnd = this.onDragEnd.bind( this );
this.resetDragState = this.resetDragState.bind( this );

this.isChromeAndHasIframes = false;
}

componentWillUnmount() {
Expand All @@ -36,10 +42,11 @@ class Draggable extends Component {
*/
onDragEnd( event ) {
const { onDragEnd = noop } = this.props;
event.preventDefault();
if ( event ) {
event.preventDefault();
}

this.resetDragState();

this.props.setTimeout( onDragEnd );
}

Expand All @@ -58,6 +65,13 @@ class Draggable extends Component {
this.cursorTop = event.clientY;
}

onDrop( ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// As per https://html.spec.whatwg.org/multipage/dnd.html#dndevents
// the target node for the dragend is the source node that started the drag operation,
// while drop event's target is the current target element.
this.onDragEnd( null );
}

/**
* - Clones the current element and spawns clone over original element.
* - Adds a fake temporary drag image to avoid browser defaults.
Expand Down Expand Up @@ -128,6 +142,17 @@ class Draggable extends Component {
document.body.classList.add( 'is-dragging-components-draggable' );
document.addEventListener( 'dragover', this.onDragOver );

// Fixes https://bugs.chromium.org/p/chromium/issues/detail?id=737691#c8
// dragend event won't be dispatched in the chrome browser
// when iframes are affected by the drag operation. So, in that case,
// we use the drop event to wrap up the dragging operation.
// This way the hack is contained to a specific use case and the external API
// still relies mostly on the dragend event.
if ( isChromeUA() && documentHasIframes() ) {
this.isChromeAndHasIframes = true;
document.addEventListener( 'drop', this.onDrop );
}

this.props.setTimeout( onDragStart );
}

Expand All @@ -143,6 +168,11 @@ class Draggable extends Component {
this.cloneWrapper = null;
}

if ( this.isChromeAndHasIframes ) {
this.isChromeAndHasIframes = false;
document.removeEventListener( 'drop', this.onDrop );
}

// Reset cursor.
document.body.classList.remove( 'is-dragging-components-draggable' );
}
Expand Down