Summary: | dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aroben, dbates, oliver, tony, yael | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Brian Weinstein
2010-01-14 16:37:22 PST
Created attachment 51012 [details]
Patch
(In reply to comment #0) > dragOpFromIEOp("move") returns DragOperationGeneric instead of > DragOperationMove. This doesn't make any sense. This is causing fast/events/drag-and-drop.html to fail on Chromium. The test tries to set a dropEffect of 'move', but no operation occurs because the source operation mask doesn't include generic. Also, as far as I can tell, this code hasn't changed since the initial commit: http://trac.webkit.org/changeset/6779 How do we know this won't cause any regressions in interacting with the rest of the OS? Your patch doesn't mention what effect this will have on platforms that *do* have a concept of a "generic" operation. (In reply to comment #4) > How do we know this won't cause any regressions in interacting with the rest of > the OS? Your patch doesn't mention what effect this will have on platforms that > *do* have a concept of a "generic" operation. The layout tests pass on mac and manual testing seems to confirm that nothing has regressed. Is there some other way for me to test for regressions (manual or automated tests)? I'm happy to write additional tests if you have some suggestions on areas that might not be well covered. (In reply to comment #5) > (In reply to comment #4) > > How do we know this won't cause any regressions in interacting with the rest of > > the OS? Your patch doesn't mention what effect this will have on platforms that > > *do* have a concept of a "generic" operation. > > The layout tests pass on mac and manual testing seems to confirm that nothing > has regressed. Is there some other way for me to test for regressions (manual > or automated tests)? I'm happy to write additional tests if you have some > suggestions on areas that might not be well covered. Have you tried the case mentioned in <http://trac.webkit.org/changeset/3400>? Comment on attachment 51012 [details] Patch > + Covered by existing tests (fast/events/drag-and-drop.html). If this change is covered by those tests, why aren't the tests failing? (In reply to comment #7) > (From update of attachment 51012 [details]) > > + Covered by existing tests (fast/events/drag-and-drop.html). > > If this change is covered by those tests, why aren't the tests failing? The win port is passing because it converts generic into move: http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp#L4861 gkt and qt skip this test and this test is failing on the chromium ports. If you prefer, I can make the chromium ports convert generic to move, but I imagine qt and gtk will have to do the same. (In reply to comment #6) > Have you tried the case mentioned in <http://trac.webkit.org/changeset/3400>? Yes, I am still able to drag text from Safari to Terminal. Note that I haven't removed Generic, I just added Move. Let me know if this isn't a patch you want to take and I'll just work around the problem in WebKit/chromium/ like the code in WebKit/win/. Comment on attachment 51012 [details]
Patch
This looks sane to me. Doesn't this also require changes to test_expectations now that that's in webkit.org?
(In reply to comment #10) > (From update of attachment 51012 [details]) > This looks sane to me. Doesn't this also require changes to test_expectations > now that that's in webkit.org? It does. I will rev the patch in https://bugs.webkit.org/show_bug.cgi?id=36484 which will include the test_expectations change. Comment on attachment 51012 [details] Patch Clearing flags on attachment: 51012 Committed r56513: <http://trac.webkit.org/changeset/56513> All reviewed patches have been landed. Closing bug. |