Bug 61683 - Convert editing/pasteboard/4861010 to dump-as-markup
Summary: Convert editing/pasteboard/4861010 to dump-as-markup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 61870 61871
Blocks: 34564
  Show dependency treegraph
 
Reported: 2011-05-27 20:24 PDT by Annie Sullivan
Modified: 2011-06-06 14:38 PDT (History)
3 users (show)

See Also:


Attachments
Patch (78.69 KB, patch)
2011-05-27 20:26 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (14.35 KB, patch)
2011-05-31 15:08 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (15.33 KB, patch)
2011-05-31 15:40 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Annie Sullivan 2011-05-27 20:24:52 PDT
The output of this test would be a lot easier to read as markup.
Comment 1 Annie Sullivan 2011-05-27 20:26:28 PDT
Created attachment 95251 [details]
Patch
Comment 2 Annie Sullivan 2011-05-27 20:29:24 PDT
(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.
Comment 3 Ryosuke Niwa 2011-05-28 18:53:47 PDT
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*.
Comment 4 Annie Sullivan 2011-05-31 15:08:04 PDT
Created attachment 95490 [details]
Patch
Comment 5 Annie Sullivan 2011-05-31 15:09:17 PDT
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.
Comment 6 Ryosuke Niwa 2011-05-31 15:31:13 PDT
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.
Comment 7 Annie Sullivan 2011-05-31 15:40:38 PDT
Created attachment 95497 [details]
Patch
Comment 8 WebKit Commit Bot 2011-06-01 00:16:17 PDT
Comment on attachment 95497 [details]
Patch

Clearing flags on attachment: 95497

Committed r87778: <http://trac.webkit.org/changeset/87778>
Comment 9 WebKit Commit Bot 2011-06-01 00:16:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Mario Sanchez Prada 2011-06-01 03:24:17 PDT
--- 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!
Comment 11 Annie Sullivan 2011-06-01 09:52:49 PDT
(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!
Comment 12 Annie Sullivan 2011-06-01 10:39:09 PDT
Re-opening this bug because of the failing webkit2 and gtk tests.
Comment 13 Ryosuke Niwa 2011-06-01 12:10:06 PDT
(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.
Comment 14 Ryosuke Niwa 2011-06-06 14:38:08 PDT
Committed in http://trac.webkit.org/changeset/87778.