WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
33691
Drag and Drop source/destination code needs cleanup
https://bugs.webkit.org/show_bug.cgi?id=33691
Summary
Drag and Drop source/destination code needs cleanup
Brian Weinstein
Reported
2010-01-14 15:01:05 PST
Drag and Drop source/destination code needs cleanup
Attachments
Patch
(5.34 KB, patch)
2010-01-14 15:01 PST
,
Brian Weinstein
no flags
Details
Formatted Diff
Diff
Patch that works on Qt and Chromium
(8.26 KB, patch)
2010-01-14 15:39 PST
,
Brian Weinstein
aroben
: review+
bweinstein
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Brian Weinstein
Comment 1
2010-01-14 15:01:39 PST
Created
attachment 46611
[details]
Patch
Adam Roben (:aroben)
Comment 2
2010-01-14 15:10:18 PST
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
Comment 3
2010-01-14 15:13:29 PST
(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)
Comment 4
2010-01-14 15:16:13 PST
(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
Comment 5
2010-01-14 15:16:26 PST
Attachment 46611
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/187684
WebKit Review Bot
Comment 6
2010-01-14 15:28:06 PST
Attachment 46611
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/186750
Adam Roben (:aroben)
Comment 7
2010-01-14 15:32:40 PST
The old behavior was added in r6779: <
http://trac.webkit.org/changeset/6779#file10
>.
Brian Weinstein
Comment 8
2010-01-14 15:39:16 PST
Created
attachment 46615
[details]
Patch that works on Qt and Chromium
Adam Roben (:aroben)
Comment 9
2010-01-14 15:54:01 PST
(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 10
2010-01-14 15:56:19 PST
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
Comment 11
2010-01-14 16:08:06 PST
Landed in
r53296
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug