Bug 62998 – Don't always select images during an image drag

• Component: UI Events
• Status: RESOLVED FIXED

[Aparna Nandyal]

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.

[Aparna Nandyal]

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]

Is this not working on other non-Qt/non-Mac platforms, e.g. Chromium Linux?

[Yael]

(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]

(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]

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]

(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]

(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]

> 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]

> 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 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)]

Oliver was Mr. drag and drop in a past life.

[Daniel Cheng]

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]

Created attachment 111228 [details]
Patch

[Daniel Cheng]

Comment on attachment 111228 [details]
Patch

Needs a test.

[Daniel Cheng]

Created attachment 111233 [details]
Patch

[Daniel Cheng]

Created attachment 111234 [details]
Patch

[WebKit Review Bot]

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 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]

Created attachment 111341 [details]
Patch

[Daniel Cheng]

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 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]

Created attachment 111379 [details]
Patch

[Daniel Cheng]

Created attachment 111380 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 111494 [details]
Patch

[Daniel Cheng]

Created attachment 111527 [details]
Patch

[Daniel Cheng]

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 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]

(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 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]

(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]

(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]

(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]

(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 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]

Created attachment 111663 [details]
Patch

[Tony Chang]

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 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]

Committed r97878: <http://trac.webkit.org/changeset/97878>

[Simon Fraser (smfr)]

This may have broken a test on Mac: bug 70551