RESOLVED FIXED 62998
Don't always select images during an image drag
https://bugs.webkit.org/show_bug.cgi?id=62998
Summary Don't always select images during an image drag
Aparna Nandyal
Reported 2011-06-20 10:13:37 PDT
Precondition: You can either use the test case in https://bugs.webkit.org/show_bug.cgi?id=61504 or go to any website that has an image (example - http://en.wikipedia.org/wiki/Sloth). Steps to reproduce the problem: 1. Open the browser, QtTestBrowser 2. Go to the above mentioned link 3. Select just the image (nothing else) 4. Open any folder in the desktop. 5. Drag it into that folder. Expected result: The dragged image should be in the folder Actual result: Unable to drag the image Additional info: If there is only an image on the web page as is the case with test case in https://bugs.webkit.org/show_bug.cgi?id=61504, after the first drag, the image gets selected automatically. So unless the image selection is removed, we cannot drag the image into a folder. Regression info: This bug is a regression caused by changeset http://trac.webkit.org/changeset/86472 and the related change.
Attachments
Patch v01 for review (1.76 KB, patch)
2011-06-20 10:24 PDT, Aparna Nandyal
no flags
Quirky test case (316 bytes, text/html)
2011-10-17 02:12 PDT, Daniel Cheng
no flags
Patch (1.94 KB, patch)
2011-10-17 02:17 PDT, Daniel Cheng
no flags
Patch (6.51 KB, patch)
2011-10-17 03:07 PDT, Daniel Cheng
no flags
Patch (6.52 KB, patch)
2011-10-17 03:09 PDT, Daniel Cheng
no flags
Patch (7.37 KB, patch)
2011-10-17 16:15 PDT, Daniel Cheng
no flags
Patch (6.54 KB, patch)
2011-10-17 22:03 PDT, Daniel Cheng
no flags
Patch (5.67 KB, patch)
2011-10-17 22:05 PDT, Daniel Cheng
no flags
Patch (11.30 KB, patch)
2011-10-18 13:28 PDT, Daniel Cheng
no flags
Patch (11.34 KB, patch)
2011-10-18 16:44 PDT, Daniel Cheng
no flags
Patch (12.96 KB, patch)
2011-10-19 13:18 PDT, Daniel Cheng
tony: review+
Aparna Nandyal
Comment 1 2011-06-20 10:24:52 PDT
Created attachment 97821 [details] Patch v01 for review Existing test cases cover regular drag and drop of image. Cannot specifically auto test this case as it requires dropping into a folder on the system.
Daniel Cheng
Comment 2 2011-06-20 10:36:17 PDT
Is this not working on other non-Qt/non-Mac platforms, e.g. Chromium Linux?
Yael
Comment 3 2011-06-21 11:03:36 PDT
(In reply to comment #2) > Is this not working on other non-Qt/non-Mac platforms, e.g. Chromium Linux? I tested on Chromium build on Windows and Linux and this works in both. Chromium's clipboard actually converts the image to markup and Qt's clipboard should probably do the same.
Daniel Cheng
Comment 4 2011-06-21 13:21:34 PDT
(In reply to comment #3) > (In reply to comment #2) > > Is this not working on other non-Qt/non-Mac platforms, e.g. Chromium Linux? > > I tested on Chromium build on Windows and Linux and this works in both. > Chromium's clipboard actually converts the image to markup and Qt's clipboard should probably do the same. My own testing seems to indicate it doesn't work on the lastest Linux Chromium build. The problem is when an image is the only thing in a selection, we use the selection logic instead of the image drag logic. I think I'm actually OK with this, though if we want to special case a selection with only an image, that's probably OK. In opinion, the real bug here is that starting a drag on an image selects it. So if you want to drag it somewhere, change your mind, and then want to drag it again, you won't be able to drag an image file to the desktop. Note that dragging an image in IE/Firefox don't cause the image to get selected, but it causes the image to get selected in any WebKit browser.
Tony Chang
Comment 5 2011-06-21 13:53:23 PDT
Perhaps selecting when dragging an image should be Mac specific behavior? That's the behavior I get when I try to drag an image in a native Mac app like Stickies or TextEdit and it would be nice to preserve that behavior on OSX.
Alexey Proskuryakov
Comment 6 2011-06-21 13:57:26 PDT
(In reply to comment #5) > Perhaps selecting when dragging an image should be Mac specific behavior? That's the behavior I get when I try to drag an image in a native Mac app like Stickies or TextEdit and it would be nice to preserve that behavior on OSX. I know that I really dislike this behavior in Safari. Why should an image become grayed out on the page after I drag it somewhere? But if it's the platform standard, it probably cannot be changed.
Darin Adler
Comment 7 2011-06-21 17:21:18 PDT
(In reply to comment #6) > I know that I really dislike this behavior in Safari. I also dislike it in Safari. > But if it's the platform standard, it probably cannot be changed. Maybe the platform standard is for an image in editable text, and it may be acceptable to have a different behavior for images outside of that context.
Aparna Nandyal
Comment 8 2011-06-21 22:03:14 PDT
> I tested on Chromium build on Windows and Linux and this works in both. > Chromium's clipboard actually converts the image to markup and Qt's clipboard should probably do the same. My testing indicates that it doesnt work on Chromium Linux build with latest WebKit code. As indicated, dragging an image does not cause image to be selected in other browsers. Even if we make WebKit behave the same way, we will still have a problem when user selects the image manually and tries to drag it. Code before changeset 86472 follows a different logic for selection drag and different logic for image drag. The fix checks if the selection has only an image and follows code path of image drag logic there after instead of selection logic.
Daniel Cheng
Comment 9 2011-06-22 12:44:14 PDT
> As indicated, dragging an image does not cause image to be selected in other browsers. Even if we make WebKit behave the same way, we will still have a problem when user selects the image manually and tries to drag it. It's worth noting that IE and Firefox both have this problem. > Code before changeset 86472 follows a different logic for selection drag and different logic for image drag. That logic was arguably wrong. > The fix checks if the selection has only an image and follows code path of image drag logic there after instead of selection logic. I'm not certain this is the appropriate fix though. If the user accidentally selects any whitespace around the image, then they get a different behavior (drag to desktop won't work). And if we strip whitespace, we might be breaking behavior that is actually desired such as dragging for rich editing.
Eric Seidel (no email)
Comment 10 2011-09-12 15:40:41 PDT
Comment on attachment 97821 [details] Patch v01 for review View in context: https://bugs.webkit.org/attachment.cgi?id=97821&action=review > Source/WebCore/page/DragController.cpp:590 > + state.m_dragType = static_cast<DragSourceAction>(state.m_dragType | DragSourceActionImage); > + else > + state.m_dragType = DragSourceActionImage; Please use only 1 space after =. How do we test this?
Eric Seidel (no email)
Comment 11 2011-09-12 15:41:05 PDT
Oliver was Mr. drag and drop in a past life.
Daniel Cheng
Comment 12 2011-10-17 02:12:28 PDT
Created attachment 111227 [details] Quirky test case I was locally testing a change to not create a selection around the image if the image is not in a contenteditable area. This resulted in a rather interesting quirk though. Let's say we have two images, foo (in a non-editable area) and bar (in an editable area). Drag bar enough to cause it to get selected. Drag foo into the same content editable area as bar. bar disappears and foo is copied to wherever the drop occurred, so we end up with only two copies of foo and zero copies of bar. I would have expected the final result to be two foos and one bar. Interestingly enough, Firefox also exhibits this quirk. I guess whenever you drag something into an editable area it's treated as replacing the selection?
Daniel Cheng
Comment 13 2011-10-17 02:17:45 PDT
Daniel Cheng
Comment 14 2011-10-17 02:22:36 PDT
Comment on attachment 111228 [details] Patch Needs a test.
Daniel Cheng
Comment 15 2011-10-17 03:07:45 PDT
Daniel Cheng
Comment 16 2011-10-17 03:09:27 PDT
WebKit Review Bot
Comment 17 2011-10-17 04:43:24 PDT
Comment on attachment 111234 [details] Patch Attachment 111234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10075960 New failing tests: editing/pasteboard/drag-image-in-about-blank-frame.html editing/pasteboard/4947130.html editing/pasteboard/drag-image-to-contenteditable-in-iframe.html fast/events/drag-selects-image.html editing/selection/drag-to-contenteditable-iframe.html http/tests/security/drag-drop-same-unique-origin.html
Ryosuke Niwa
Comment 18 2011-10-17 11:17:43 PDT
Comment on attachment 111234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111234&action=review > Source/WebCore/page/DragController.cpp:619 > +#if OS(DARWIN) It seems like this should be tied to editing behavior.
Daniel Cheng
Comment 19 2011-10-17 16:15:00 PDT
Daniel Cheng
Comment 20 2011-10-17 16:16:34 PDT
Comment on attachment 111341 [details] Patch What's the proper way to rebaseline results for the other layout test failures? I'm pretty sure that the remaining failures are all pixel diffs because the image doesn't get selected anymore which is OK.
WebKit Review Bot
Comment 21 2011-10-17 21:49:38 PDT
Comment on attachment 111341 [details] Patch Attachment 111341 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10126112 New failing tests: editing/selection/drag-to-contenteditable-iframe.html editing/pasteboard/drag-image-in-about-blank-frame.html http/tests/security/drag-drop-same-unique-origin.html editing/pasteboard/drag-image-to-contenteditable-in-iframe.html editing/pasteboard/4947130.html
Daniel Cheng
Comment 22 2011-10-17 22:03:03 PDT
Daniel Cheng
Comment 23 2011-10-17 22:05:54 PDT
WebKit Review Bot
Comment 24 2011-10-17 22:42:39 PDT
Comment on attachment 111380 [details] Patch Attachment 111380 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10126129 New failing tests: editing/selection/drag-to-contenteditable-iframe.html editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
Daniel Cheng
Comment 25 2011-10-18 13:28:18 PDT
Daniel Cheng
Comment 26 2011-10-18 16:44:14 PDT
Daniel Cheng
Comment 27 2011-10-18 18:19:37 PDT
Tony, mind taking a look? The updated patch matches Firefox behavior: - dragging an image that is not editable does not cause the image to be selected - dragging an image in is an editable area causes the image to be selected
Ryosuke Niwa
Comment 28 2011-10-18 18:41:16 PDT
Comment on attachment 111527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111527&action=review > Source/WebCore/page/DragController.cpp:617 > static void prepareClipboardForImageDrag(Frame* src, Clipboard* clipboard, Element* node, const KURL& linkURL, const KURL& imageURL, const String& label) I'd rename src to frame if not sourceFrame. > Source/WebCore/page/DragController.cpp:619 > + if (node->isContentEditable()) { You should check for being richly editable instead. > LayoutTests/fast/events/drag-selects-image-expected.txt:7 > +This tests that images are properly left selected or unselected when an image drag is started. On non-Mac platforms, neither image should be selected when dragging. On Mac, only the image in the editable area should be selected when an image drag is started. So this beahvior difference can't be changed by editingBehavior?
Daniel Cheng
Comment 29 2011-10-18 18:56:39 PDT
(In reply to comment #28) > (From update of attachment 111527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111527&action=review > > > Source/WebCore/page/DragController.cpp:617 > > static void prepareClipboardForImageDrag(Frame* src, Clipboard* clipboard, Element* node, const KURL& linkURL, const KURL& imageURL, const String& label) > > I'd rename src to frame if not sourceFrame. > I'd argue that "Frame" doesn't really add any information though. It's a really short function, and it's clear that it's a Frame*. > > Source/WebCore/page/DragController.cpp:619 > > + if (node->isContentEditable()) { > > You should check for being richly editable instead. > What's the difference? I looked around but the obvious places don't really seem to explain what the difference between the two is. > > LayoutTests/fast/events/drag-selects-image-expected.txt:7 > > +This tests that images are properly left selected or unselected when an image drag is started. On non-Mac platforms, neither image should be selected when dragging. On Mac, only the image in the editable area should be selected when an image drag is started. > > So this beahvior difference can't be changed by editingBehavior? Oops. I'll make sure to update that comment in the next patch.
Ryosuke Niwa
Comment 30 2011-10-18 19:04:30 PDT
Comment on attachment 111527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111527&action=review >>> Source/WebCore/page/DragController.cpp:617 >>> static void prepareClipboardForImageDrag(Frame* src, Clipboard* clipboard, Element* node, const KURL& linkURL, const KURL& imageURL, const String& label) >> >> I'd rename src to frame if not sourceFrame. > > I'd argue that "Frame" doesn't really add any information though. It's a really short function, and it's clear that it's a Frame*. Then source will be better. src is an abbreviation and we should avoid using it unless we're talking about src attribute and a couple other special cases. >>> Source/WebCore/page/DragController.cpp:619 >>> + if (node->isContentEditable()) { >> >> You should check for being richly editable instead. > > What's the difference? I looked around but the obvious places don't really seem to explain what the difference between the two is. isContentEditable is plaintext only.
Daniel Cheng
Comment 31 2011-10-18 22:00:06 PDT
(In reply to comment #30) > (From update of attachment 111527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111527&action=review > > >>> Source/WebCore/page/DragController.cpp:617 > >>> static void prepareClipboardForImageDrag(Frame* src, Clipboard* clipboard, Element* node, const KURL& linkURL, const KURL& imageURL, const String& label) > >> > >> I'd rename src to frame if not sourceFrame. > > > > I'd argue that "Frame" doesn't really add any information though. It's a really short function, and it's clear that it's a Frame*. > > Then source will be better. src is an abbreviation and we should avoid using it unless we're talking about src attribute and a couple other special cases. > > >>> Source/WebCore/page/DragController.cpp:619 > >>> + if (node->isContentEditable()) { > >> > >> You should check for being richly editable instead. > > > > What's the difference? I looked around but the obvious places don't really seem to explain what the difference between the two is. > > isContentEditable is plaintext only. Hypothetically speaking, how would I end up with a contenteditable image in a plaintext-only editing area?
Ryosuke Niwa
Comment 32 2011-10-18 22:08:23 PDT
(In reply to comment #31) > Hypothetically speaking, how would I end up with a contenteditable image in a plaintext-only editing area? <div style="-webkit-user-modify: read-write-plaintext-only"><img src="~~"></div> There's nothing that prevents authors from putting random stuff inside an element that's read-write-plaintext-only. I know it's pretty bad :(
Daniel Cheng
Comment 33 2011-10-18 22:31:06 PDT
(In reply to comment #32) > (In reply to comment #31) > > Hypothetically speaking, how would I end up with a contenteditable image in a plaintext-only editing area? > > <div style="-webkit-user-modify: read-write-plaintext-only"><img src="~~"></div> > > There's nothing that prevents authors from putting random stuff inside an element that's read-write-plaintext-only. I know it's pretty bad :( How odd. I notice isContentEditable() calls updateLayoutIgnorePendingStylesheets(). Do you know how important it is for me to call it here?
Ryosuke Niwa
Comment 34 2011-10-18 22:33:52 PDT
(In reply to comment #33) > How odd. > I notice isContentEditable() calls updateLayoutIgnorePendingStylesheets(). Do you know how important it is for me to call it here? You should be using isContentEditable unless you're certain that the layout is up-to-date. We need style-recalc to figure out if an element is editable or not.
Tony Chang
Comment 35 2011-10-19 09:47:28 PDT
Comment on attachment 111527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111527&action=review In general, this approach seems fine. r- for Ryosuke's comments. > LayoutTests/fast/events/drag-selects-image.html:16 > + eventSender.leapForward(500); Nit: Making this a shorter time (like 300ms) will be nicer for QT and GTK+. I think they implement leapForward with sleep(). > LayoutTests/fast/events/drag-selects-image.html:49 > +<p>This tests that images are properly left selected or unselected when an image drag is started. On > +non-Mac platforms, neither image should be selected when dragging. On Mac, only the image in the Is the Mac/non-Mac part still accurate? It would also be nice to provide instructions on how to run the test manually.
Daniel Cheng
Comment 36 2011-10-19 13:18:53 PDT
Tony Chang
Comment 37 2011-10-19 13:39:21 PDT
Comment on attachment 111663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111663&action=review > LayoutTests/fast/events/drag-selects-image.html:16 > + eventSender.leapForward(100); It looks like the drag delay on Mac is .15s, but maybe that's only for text?
Daniel Cheng
Comment 38 2011-10-19 13:41:25 PDT
Comment on attachment 111663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111663&action=review >> LayoutTests/fast/events/drag-selects-image.html:16 >> + eventSender.leapForward(100); > > It looks like the drag delay on Mac is .15s, but maybe that's only for text? Right, I believe that's only for text.
Daniel Cheng
Comment 39 2011-10-19 14:37:06 PDT
Simon Fraser (smfr)
Comment 40 2011-10-20 15:24:50 PDT
This may have broken a test on Mac: bug 70551
Note You need to log in before you can comment on or make changes to this bug.