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.
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.
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”.
Also, does this code path get used for things that are not <img> elements?
(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. :)
(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.
Comment on attachment 88633 [details] Patch Needs test.
Created attachment 102352 [details] Patch
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()).
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
(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.
+tony, +dcheng since they're clipboard experts in Chromium side.
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?
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 :)
Created attachment 102523 [details] Patch
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... :)
(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
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
(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?
The bot runs on Linux so it might be that there is a Linux-specific quirk.
(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).
(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.
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 :)
Created attachment 103300 [details] Patch
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.
Comment on attachment 103300 [details] Patch Clearing the review flags. I'll update the patch at work tomorrow :)
Created attachment 103398 [details] Patch
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.
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.
Created attachment 103408 [details] Patch
(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 :)
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?
Created attachment 103485 [details] Patch for landing
Comment on attachment 103485 [details] Patch for landing Clearing flags on attachment: 103485 Committed r92769: <http://trac.webkit.org/changeset/92769>
All reviewed patches have been landed. Closing bug.