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.
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.
Is this not working on other non-Qt/non-Mac platforms, e.g. Chromium Linux?
(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.
(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.
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.
(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.
(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.
> 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.
> 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.
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?
Oliver was Mr. drag and drop in a past life.
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?
Created attachment 111228 [details] Patch
Comment on attachment 111228 [details] Patch Needs a test.
Created attachment 111233 [details] Patch
Created attachment 111234 [details] Patch
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
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.
Created attachment 111341 [details] Patch
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.
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
Created attachment 111379 [details] Patch
Created attachment 111380 [details] Patch
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
Created attachment 111494 [details] Patch
Created attachment 111527 [details] Patch
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
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?
(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.
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.
(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?
(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 :(
(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?
(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.
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.
Created attachment 111663 [details] Patch
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?
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.
Committed r97878: <http://trac.webkit.org/changeset/97878>
This may have broken a test on Mac: bug 70551