Drag and Drop source/destination code needs cleanup

• Component: New Bugs
• Status: RESOLVED FIXED

[Brian Weinstein]

Drag and Drop source/destination code needs cleanup

[Brian Weinstein]

Created attachment 46611 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 46611 [details]
Patch

> -static DragOperation defaultOperationForDrag(DragOperation srcOpMask)
> -{
> -    // This is designed to match IE's operation fallback for the case where
> -    // the page calls preventDefault() in a drag event but doesn't set dropEffect.
> -    if (srcOpMask & DragOperationCopy)
> -         return DragOperationCopy;
> -    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric)
> -        return DragOperationMove;
> -    if (srcOpMask & DragOperationLink)
> -        return DragOperationLink;
> -
> -    // FIXME: Does IE really return "generic" even if no operations were allowed by the source?
> -    return DragOperationGeneric;
> -}
> -
>  bool DragController::tryDHTMLDrag(DragData* dragData, DragOperation& operation)
>  {
>      ASSERT(dragData);
> @@ -517,10 +502,8 @@ bool DragController::tryDHTMLDrag(DragDa
>          return false;
>      }
>  
> -    if (!clipboard->destinationOperation(operation)) {
> -        // The element accepted but they didn't pick an operation, so we pick one (to match IE).
> -        operation = defaultOperationForDrag(srcOpMask);
> -    } else if (!(srcOpMask & operation)) {
> +    operation = clipboard->destinationOperation();
> +    if (!(srcOpMask & operation)) {
>          // The element picked an operation which is not supported by the source
>          operation = DragOperationNone;
>      }

Is it really OK to make this change? Will we break websites that call preventDefault() but don't set dropEffect?

[Brian Weinstein]

(In reply to comment #2)
> (From update of attachment 46611 [details])
> > -static DragOperation defaultOperationForDrag(DragOperation srcOpMask)
> > -{
> > -    // This is designed to match IE's operation fallback for the case where
> > -    // the page calls preventDefault() in a drag event but doesn't set dropEffect.
> > -    if (srcOpMask & DragOperationCopy)
> > -         return DragOperationCopy;
> > -    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric)
> > -        return DragOperationMove;
> > -    if (srcOpMask & DragOperationLink)
> > -        return DragOperationLink;
> > -
> > -    // FIXME: Does IE really return "generic" even if no operations were allowed by the source?
> > -    return DragOperationGeneric;
> > -}
> > -
> >  bool DragController::tryDHTMLDrag(DragData* dragData, DragOperation& operation)
> >  {
> >      ASSERT(dragData);
> > @@ -517,10 +502,8 @@ bool DragController::tryDHTMLDrag(DragDa
> >          return false;
> >      }
> >  
> > -    if (!clipboard->destinationOperation(operation)) {
> > -        // The element accepted but they didn't pick an operation, so we pick one (to match IE).
> > -        operation = defaultOperationForDrag(srcOpMask);
> > -    } else if (!(srcOpMask & operation)) {
> > +    operation = clipboard->destinationOperation();
> > +    if (!(srcOpMask & operation)) {
> >          // The element picked an operation which is not supported by the source
> >          operation = DragOperationNone;
> >      }
> 
> Is it really OK to make this change? Will we break websites that call
> preventDefault() but don't set dropEffect?

That code will never be hit after r53287, because destinationOperation will never return false. This makes it compliant with the spec, but I haven't found any demos. I did have to make changes to some of our layout tests, that was why I had to roll out the change and roll it back in. I feel like being more compliant with the spec is important, but if there are a lot of websites that use drag and drop, then I could drop this change, and make some changes to r53287.

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 46611 [details] [details])
> > Is it really OK to make this change? Will we break websites that call
> > preventDefault() but don't set dropEffect?
> 
> That code will never be hit after r53287, because destinationOperation will
> never return false.

Ah, OK, so we've already made this change. You're just removing dead code now.

> This makes it compliant with the spec, but I haven't found
> any demos. I did have to make changes to some of our layout tests, that was why
> I had to roll out the change and roll it back in. I feel like being more
> compliant with the spec is important, but if there are a lot of websites that
> use drag and drop, then I could drop this change, and make some changes to
> r53287.

We should try to find sites that rely on the old behavior. It might be useful to find out what sites were used to reverse-engineer IE's implementation in order to write WebKit's.

[WebKit Review Bot]

Attachment 46611 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/187684

[WebKit Review Bot]

Attachment 46611 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/186750

[Adam Roben (:aroben)]

The old behavior was added in r6779: <http://trac.webkit.org/changeset/6779#file10>.

[Brian Weinstein]

Created attachment 46615 [details]
Patch that works on Qt and Chromium

[Adam Roben (:aroben)]

(In reply to comment #4)
> We should try to find sites that rely on the old behavior. It might be useful
> to find out what sites were used to reverse-engineer IE's implementation in
> order to write WebKit's.

Vicki Murley might remember!

[Adam Roben (:aroben)]

Comment on attachment 46615 [details]
Patch that works on Qt and Chromium

>  void Clipboard::setSourceOperation(DragOperation op)
>  {
> +    ASSERT(op != DragOperationPrivate);
>      m_effectAllowed = IEOpFromDragOp(op);
>  }
>  
>  void Clipboard::setDestinationOperation(DragOperation op)
>  {
> +    ASSERT(op == DragOperationCopy || op == DragOperationNone || op == DragOperationLink || op == DragOperationMove);
>      m_dropEffect = IEOpFromDragOp(op);
>  }

ASSERT_ARG would be better for these.

This looks fine to land. But I don't want to forget to find out if removing our old IE-compat code is going to bite us in the end. Maybe we should get a bug filed saying that we need to do more testing? (E.g., it would be useful to find out if IE even still has this behavior.)

[Brian Weinstein]

Landed in r53296.