Bug 58043 – Unify the way we generate HTML for an image in the Clipboard

• Component: Platform
• Status: RESOLVED FIXED

[Benjamin Poulain]

Currently, when adding an image to the clipboard, we have the following behaviors:
-GTK serialize the full image element with createMarkup()
-Chromium has a custom method imageToMarkup() to serialize the img element and all its attribute
-The Windows port has a custom method imageToMarkup() creating an img element

That means the behavior change when passing an image around. I think it would be simpler to use createMarkup() everywhere.

[Benjamin Poulain]

Created attachment 88633 [details]
Patch

I have not tested this, it is just an idea to reduce maintainance for the Windows and Chromium ports.
Feel free to reject if you prefer having custom methods per port.

[Darin Adler]

Comment on attachment 88633 [details]
Patch

Good idea to remove unneeded code.

Besides fixes to quoting, the behavior change here is that createMarkup will include all attributes of the image element, and the existing functions include only the image and use only a src attribute.

So the clipboard will now have things like a class and ID for the image, any explicit style attributes, width and height attributes, and the like.

What’s your argument for it being OK? We probably have to do better than “I hope it’s OK”.

[Darin Adler]

Also, does this code path get used for things that are not <img> elements?

[Benjamin Poulain]

(In reply to comment #2)
> So the clipboard will now have things like a class and ID for the image, any explicit style attributes, width and height attributes, and the like.
> 
> What’s your argument for it being OK? We probably have to do better than “I hope it’s OK”.

I choose that implementation simply because that is also what Firefox and IE do. :)

[Benjamin Poulain]

(In reply to comment #3)
> Also, does this code path get used for things that are not <img> elements?

As far as I can see, this is only used if the RenderObject::isImage() returns true.

Which, after some digging, can actually be used for HTMLPlugInImageElement and WMLImageElement and input[type=image].

I just noticed dragging input[type=image] fails with WebKit because of the conversion input->img.

I'll r- my patch. That should be tested.

[Benjamin Poulain]

Comment on attachment 88633 [details]
Patch

Needs test.

[Benjamin Poulain]

Created attachment 102352 [details]
Patch

[Benjamin Poulain]

Since last time, the code of the Windows port has actually been changed to be like Chromium: http://trac.webkit.org/changeset/85064

I think it is not correct, and I think two of the attached tests will fail on the Windows on Chromium ports (the object and input[image] would fail on those ports because the original element will be transformed in <img> by imagetoMarkup()).

[WebKit Review Bot]

Comment on attachment 102352 [details]
Patch

Attachment 102352 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9269416

New failing tests:
fast/events/drag-and-drop-image-contenteditable.html
fast/events/drag-and-drop-inputimage-contenteditable.html
fast/events/drag-and-drop-objectimage-contenteditable.html

[Benjamin Poulain]

(In reply to comment #9)
> New failing tests:
> fast/events/drag-and-drop-image-contenteditable.html
> fast/events/drag-and-drop-inputimage-contenteditable.html
> fast/events/drag-and-drop-objectimage-contenteditable.html

So that does indeed fail on Chromium. :)
The expected results were generated on the Mac port.

[Ryosuke Niwa]

+tony, +dcheng since they're clipboard experts in Chromium side.

[Tony Chang]

Switching from imageToMarkup to createMarkup sounds good to me.

It's not clear to me how/why the new tests fail on chromium, but maybe if you combined the tests and code change into a single patch, the cr-linux ews bot would pass?

[Benjamin Poulain]

Comment on attachment 102352 [details]
Patch

(In reply to comment #12)
> Switching from imageToMarkup to createMarkup sounds good to me.
> 
> It's not clear to me how/why the new tests fail on chromium, but maybe if you combined the tests and code change into a single patch, the cr-linux ews bot would pass?

The problem on chromium is that you need a src attribute and everything is converted to <img>.

I will make an update with a speculative patch :)

[Benjamin Poulain]

Created attachment 102523 [details]
Patch

[Benjamin Poulain]

Comment on attachment 102523 [details]
Patch

Oops, did not mean to put it in review queue, I just need the bots to have a look since I cannot check those platforms... :)

[Tony Chang]

(In reply to comment #15)
> (From update of attachment 102523 [details])
> Oops, did not mean to put it in review queue, I just need the bots to have a look since I cannot check those platforms... :)

You can use this page to run patches through the ews bots:
http://queues.webkit.org/submit-to-ews

[WebKit Review Bot]

Comment on attachment 102523 [details]
Patch

Attachment 102523 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9287402

New failing tests:
fast/events/drag-and-drop-image-contenteditable.html
fast/events/drag-and-drop-inputimage-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
fast/events/drag-and-drop-objectimage-contenteditable.html

[Benjamin Poulain]

(In reply to comment #17)
> (From update of attachment 102523 [details])
> Attachment 102523 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9287402
> 
> New failing tests:
> fast/events/drag-and-drop-image-contenteditable.html
> fast/events/drag-and-drop-inputimage-contenteditable.html
> editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
> fast/events/drag-and-drop-objectimage-contenteditable.html

I was a bit surprised by these results so I ended up compiling the chrome port (on Mac) :)

On my machine, I actually get what I was expecting from the code. Without the patch the following two tests fails:
  fast/events/drag-and-drop-inputimage-contenteditable.html -> unexpected text diff mismatch
  fast/events/drag-and-drop-objectimage-contenteditable.html -> unexpected text diff mismatch

With the patch, the 3 tests run correctly on my machine.
Anyone from Chrome could help me figure out what is wrong here? Is it possible to have the full output from your bot to see what results it got?

[Ryosuke Niwa]

The bot runs on Linux so it might be that there is a Linux-specific quirk.

[Tony Chang]

(In reply to comment #18)
> Anyone from Chrome could help me figure out what is wrong here? Is it possible to have the full output from your bot to see what results it got?

I tried running this on my Chromium Linux build and the three new tests timed out.  This was because the source y coordinate was 605, which is just outside the bounds of the window (the window is 800x600).  If I set the height of #target to 200px, it works fine for me.  I'm not sure why the bots are getting a text diff.

For mac, don't you need an eventSender.leapForward after the mouseDown to trigger the drag?

editing/pasteboard/drag-image-to-contenteditable-in-iframe.html is failing because the src of the dropped img is "broken_image" (not sure why).

[Benjamin Poulain]

(In reply to comment #20)
> I tried running this on my Chromium Linux build and the three new tests timed out.  This was because the source y coordinate was 605, which is just outside the bounds of the window (the window is 800x600).  If I set the height of #target to 200px, it works fine for me.  I'm not sure why the bots are getting a text diff.

Thanks a lot for testing that! I will update the test accordingly.

> For mac, don't you need an eventSender.leapForward after the mouseDown to trigger the drag?

That seems to work as it is now. I can add leapForward if that makes the test more robust.


> editing/pasteboard/drag-image-to-contenteditable-in-iframe.html is failing because the src of the dropped img is "broken_image" (not sure why).

Hum :(
I'll try to get a Linux build to test that.

Thanks for the help.

[Benjamin Poulain]

Created attachment 102799 [details]
Patch

I haven't had the time to work on this today so I just update the tests to see if the bot like that better :)

[Benjamin Poulain]

Created attachment 103300 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 103300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103300&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:377
> -        if (shouldResolveURLs() && !element->document()->url().isLocalFile())
> +        if (shouldResolveURLs() || (shouldResolveNonLocalURLs() && !element->document()->url().isLocalFile()))

Instead of adding checks like this, can shouldResolveURL take url() and internally check this condition?

> Source/WebCore/editing/markup.h:44
> +    enum EAbsoluteURLs { DoNotResolveURLs, AbsoluteNonLocalURLs, AbsoluteURLs };

I don't think these enum values are easy to understand when it appears on call sites.  Can we rename them to ResolveAllURLs and ResolveNonLocalURLs?  I don't think "absolute" adds any useful information here.

[Benjamin Poulain]

Comment on attachment 103300 [details]
Patch

Clearing the review flags. I'll update the patch at work tomorrow :)

[Benjamin Poulain]

Created attachment 103398 [details]
Patch

[WebKit Review Bot]

Attachment 103398 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/editing/MarkupAccumulator.h:69:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 103398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103398&action=review

The patch looks good overall but I'd like to see tests moved to LayoutTests/editing/pasteboard because drag & drop really is an editing feature.

> LayoutTests/ChangeLog:10
> +        Add a new folder drag-and-drop for fast tests only focused on drag and drop.
> +
> +        Add 3 tests for dragging an image-like element to an editable area.

Please move these tests to LayoutTests/editing/pasteboard.

> LayoutTests/platform/qt/Skipped:275
> +fast/drag-and-drop

Really?  These tests fail on Qt?

> Source/WebCore/editing/MarkupAccumulator.cpp:129
> +    default:
> +        break;
> +    }
> +    return urlString;

Please get rid of default and explicitly list all cases.  And please put ASSERT_NOT_REACHED; after the switch statement.  This way, if we ever add new resolve* in the future and forget to change this function, the compiler will catch it.

>> Source/WebCore/editing/MarkupAccumulator.h:69
>> +    MarkupAccumulator(Vector<Node*>* nodes, EAbsoluteURLs resolveUrlsMethod, const Range* range = 0);
> 
> The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]

Style bot is right.  Please get rid of nodes and range.

[Benjamin Poulain]

Created attachment 103408 [details]
Patch

[Benjamin Poulain]

(In reply to comment #28)
> (From update of attachment 103398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103398&action=review
> 
> The patch looks good overall but I'd like to see tests moved to LayoutTests/editing/pasteboard because drag & drop really is an editing feature.

These 3 tests are far from ideal for testing this particular bug. The editing could be done differently when the drag originate from WebKit and then this change is not tested anymore.

I made the directory because I have other tests for the same problem, but they are blocked because of 2 others bugs.

I moved the tests to editing/pasteboard for now. I can move them later if I can improve them (or make new ones :-))

> > LayoutTests/platform/qt/Skipped:275
> > +fast/drag-and-drop
> 
> Really?  These tests fail on Qt?

Qt does not support testing drag and drop. Originally this was working for Qt and I was fixing the other ports :)

[Ryosuke Niwa]

Comment on attachment 103408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103408&action=review

Yay! More code sharing between the ports.

> LayoutTests/editing/pasteboard/drag-and-drop-image-contenteditable.html:13
> +

Nit: why do we have these blank lines after </head>?

> LayoutTests/editing/pasteboard/drag-and-drop-image-contenteditable.html:22
> +jsTestIsAsync = true;

Nit: missing var.

> LayoutTests/editing/pasteboard/drag-and-drop-image-contenteditable.html:36
> +function runTest() {

Nit: maybe a blank line before function runTest()?

> LayoutTests/editing/pasteboard/drag-and-drop-inputimage-contenteditable.html:13
> +

Nit: space.

> LayoutTests/editing/pasteboard/drag-and-drop-inputimage-contenteditable.html:22
> +jsTestIsAsync = true;

Nit: missing var.

> LayoutTests/editing/pasteboard/drag-and-drop-inputimage-contenteditable.html:37
> +function runTest() {

Nit: blank line here.

> LayoutTests/editing/pasteboard/drag-and-drop-objectimage-contenteditable.html:13
> +

Nit: ditto.

> LayoutTests/editing/pasteboard/drag-and-drop-objectimage-contenteditable.html:22
> +jsTestIsAsync = true;

Nit: ditto.

> LayoutTests/editing/pasteboard/drag-and-drop-objectimage-contenteditable.html:37
> +function runTest() {

Nit: ditto.

> Source/WebCore/editing/MarkupAccumulator.cpp:126
> +

Missing break.

> Source/WebCore/editing/MarkupAccumulator.cpp:391
> +        appendQuotedURLAttributeValue(out, resolveURLIfNeeded(element, attribute.value()));

Maybe we can merge appendQuotedURLAttributeValue and resolveURLIfNeeded in a follow up patch?

[Benjamin Poulain]

Created attachment 103485 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 103485 [details]
Patch for landing

Clearing flags on attachment: 103485

Committed r92769: <http://trac.webkit.org/changeset/92769>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.