Drag and Drop source/destination code needs cleanup
Created attachment 46611 [details] Patch
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?
(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.
(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.
Attachment 46611 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/187684
Attachment 46611 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/186750
The old behavior was added in r6779: <http://trac.webkit.org/changeset/6779#file10>.
Created attachment 46615 [details] Patch that works on Qt and Chromium
(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!
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.)
Landed in r53296.