Bug 61683 – Convert editing/pasteboard/4861010 to dump-as-markup

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

The output of this test would be a lot easier to read as markup.

[Annie Sullivan]

Created attachment 95251 [details]
Patch

[Annie Sullivan]

(In reply to comment #1)
> Created an attachment (id=95251) [details]
> Patch

Note: When I was working on this patch, I noticed that the behavior is no longer what the test describes ("DragMe" gets inserted after "DropAboveMe", not before). The behavior changed in changeset 24255. I updated the description, but is this test still valid? If not, I can delete it instead of updating it.

[Ryosuke Niwa]

Comment on attachment 95251 [details]
Patch

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

(In reply to comment #2)
> Note: When I was working on this patch, I noticed that the behavior is no longer what the test describes ("DragMe" gets inserted after "DropAboveMe", not before). The behavior changed in changeset 24255. I updated the description, but is this test still valid? If not, I can delete it instead of updating it.

I don't think we should remove this test since it's one of very few tests that test drag & drop.

Also, would you mind renaming the file to something more descriptive?  You can still include <rdar://4861080> in the test description.

> LayoutTests/editing/pasteboard/4861080-expected.txt:1
> +This tests dropping content into the unwanted space above a list items content when the content is wrapped in a span.  You should see 'DropAboveMe DragMe'.

There is no longer unwanted space so we should rephrase this.

> LayoutTests/editing/pasteboard/4861080.html:47
>      eventSender.mouseUp();

The statements that compute dropx and dropy seem wrong.  We should get rid of li.offsetParent.offset*.

[Annie Sullivan]

Created attachment 95490 [details]
Patch

[Annie Sullivan]

Comment on attachment 95251 [details]
Patch

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

>> LayoutTests/editing/pasteboard/4861080-expected.txt:1
>> +This tests dropping content into the unwanted space above a list items content when the content is wrapped in a span.  You should see 'DropAboveMe DragMe'.
> 
> There is no longer unwanted space so we should rephrase this.

Done.

>> LayoutTests/editing/pasteboard/4861080.html:47
>>      eventSender.mouseUp();
> 
> The statements that compute dropx and dropy seem wrong.  We should get rid of li.offsetParent.offset*.

Done.

[Ryosuke Niwa]

Comment on attachment 95490 [details]
Patch

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

> LayoutTests/editing/pasteboard/drag-drop-list.html:50
> +    Markup.description("This tests dropping content onto a list items content when the content is wrapped in a span.  You should see 'DropAboveMe DragMe'. See <rdar://4861080>.");

You should probably mention that the expected behavior is different from what's described in 4861080 because the rendering bug 4907469 has been fixed.

[Annie Sullivan]

Created attachment 95497 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95497 [details]
Patch

Clearing flags on attachment: 95497

Committed r87778: <http://trac.webkit.org/changeset/87778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mario Sanchez Prada]

--- LayoutTests/editing/pasteboard/drag-drop-list-expected.txt	(revision 0)
+++ LayoutTests/editing/pasteboard/drag-drop-list-expected.txt	(revision 0)
@@ -0,0 +1,18 @@
+This tests dropping content onto a list items content when the content is 
[...]
+|     <div>
+|       id="contents"
+|       style="border: 1px solid red;"
+|       "DropAboveMe"
+|       "<#selection-anchor>Â DragMe<#selection-focus>"
[...]

Just one question about this part in the expected file:

Why is this "Â " present before "DragMe"? Is it just a typo or is there a reason for it?

Asking because at the moment that's the cause the test is failing in GTK and SnowLeopardBots.

Some links showing the issue:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/r87787%20%2812181%29/editing/pasteboard/drag-drop-list-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20%28WebKit2%20Tests%29/r87782%20%28342%29/editing/pasteboard/drag-drop-list-pretty-diff.html
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r87787%20%2822961%29/editing/pasteboard/drag-drop-list-pretty-diff.html
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r87787%20%2814261%29/editing/pasteboard/drag-drop-list-pretty-diff.html
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r87781%20%2815653%29/editing/pasteboard/drag-drop-list-pretty-diff.html

If it's a typo, please update the expected file when possible.

Thanks!

[Annie Sullivan]

(In reply to comment #10)
> --- LayoutTests/editing/pasteboard/drag-drop-list-expected.txt    (revision 0)
> +++ LayoutTests/editing/pasteboard/drag-drop-list-expected.txt    (revision 0)
> @@ -0,0 +1,18 @@
> +This tests dropping content onto a list items content when the content is 
> [...]
> +|     <div>
> +|       id="contents"
> +|       style="border: 1px solid red;"
> +|       "DropAboveMe"
> +|       "<#selection-anchor>Â DragMe<#selection-focus>"
> [...]
> 
> Just one question about this part in the expected file:
> 
> Why is this "Â " present before "DragMe"? Is it just a typo or is there a reason for it?

I think it's a newline. I'm looking into it now.

> Asking because at the moment that's the cause the test is failing in GTK and SnowLeopardBots.

Yeah, there may be platform-dependent behavior with the newlines. I am working on figuring out the issue now. Thanks for the links!

[Annie Sullivan]

Re-opening this bug because of the failing webkit2 and gtk tests.

[Ryosuke Niwa]

(In reply to comment #10)
> --- LayoutTests/editing/pasteboard/drag-drop-list-expected.txt    (revision 0)
> +++ LayoutTests/editing/pasteboard/drag-drop-list-expected.txt    (revision 0)
> @@ -0,0 +1,18 @@
> +This tests dropping content onto a list items content when the content is 
> [...]
> +|     <div>
> +|       id="contents"
> +|       style="border: 1px solid red;"
> +|       "DropAboveMe"
> +|       "<#selection-anchor>Â DragMe<#selection-focus>"
> [...]
> 
> Just one question about this part in the expected file:
> 
> Why is this "Â " present before "DragMe"? Is it just a typo or is there a reason for it?

This is smart paste. "Â " is "&nbsp; " automatically inserted by WebKit.

[Ryosuke Niwa]

Committed in http://trac.webkit.org/changeset/87778.