Bug 61680 - editing/inserting/insert-3907422 should be dumpAsText test
Summary: editing/inserting/insert-3907422 should be dumpAsText test
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:
Blocks: 34564
  Show dependency treegraph
 
Reported: 2011-05-27 18:22 PDT by Annie Sullivan
Modified: 2011-05-27 23:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (42.42 KB, patch)
2011-05-27 18:24 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (42.35 KB, patch)
2011-05-27 18:32 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (42.99 KB, patch)
2011-05-27 18:52 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 18:22:09 PDT
The output would be much easier to understand as dumpAsText.
Comment 1 Annie Sullivan 2011-05-27 18:24:10 PDT
Created attachment 95239 [details]
Patch
Comment 2 Ryosuke Niwa 2011-05-27 18:27:31 PDT
Comment on attachment 95239 [details]
Patch

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

> LayoutTests/editing/inserting/insert-3907422-fix.html:-9
> -.editing { 
> -    word-wrap: break-word; 
> -    -khtml-nbsp-mode: space; 
> -    -khtml-line-break: after-white-space;
> -}

I don't think you should be removing these styles since they affect editing behavior.
Comment 3 Ryosuke Niwa 2011-05-27 18:28:52 PDT
Comment on attachment 95239 [details]
Patch

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

> LayoutTests/editing/inserting/insert-3907422-fix.html:20
> -<body contenteditable id="test" class="editing">
> +<body contenteditable id="root" class="editing" onload="runDumpAsTextEditingTest();">

I don't think we should change #test to #root either.  They are treated differently in editing.js
Comment 4 Annie Sullivan 2011-05-27 18:31:33 PDT
Comment on attachment 95239 [details]
Patch

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

>> LayoutTests/editing/inserting/insert-3907422-fix.html:-9
>> -}
> 
> I don't think you should be removing these styles since they affect editing behavior.

Oops, you're right. Since we had been cleaning out red/blue lines around the style, I didn't check carefully enough what I was removing. I added them back and the tests results are the same.
Comment 5 Annie Sullivan 2011-05-27 18:32:52 PDT
Created attachment 95242 [details]
Patch
Comment 6 Annie Sullivan 2011-05-27 18:38:25 PDT
(In reply to comment #3)
> (From update of attachment 95239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95239&action=review
> 
> > LayoutTests/editing/inserting/insert-3907422-fix.html:20
> > -<body contenteditable id="test" class="editing">
> > +<body contenteditable id="root" class="editing" onload="runDumpAsTextEditingTest();">
> 
> I don't think we should change #test to #root either.  They are treated differently in editing.js

Whoops, didn't see this comment before. I didn't know "test" was a special id for editing.js. But of course "root" is too, and if I don't have a node with ID root, then I don't get output. I'm not sure how to refactor the test if we don't change body to id root--the point of the test is to do a select all, then copy/paste. If I added a containing root div, then the test would behave differently. If I put ids on divs that end up getting copy/pasted, the behavior of the test gets even harder to understand.

Maybe it would be better just not to use editing.js for this test?
Comment 7 Ryosuke Niwa 2011-05-27 18:40:09 PDT
Comment on attachment 95242 [details]
Patch

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

> LayoutTests/editing/inserting/insert-3907422-fix.html:21
> +    var result = root.children[0].nodeName == "BLOCKQUOTE" ? "FAIL" : "PASS";
> +    document.write(result + ": Buggy code before fix would insert blockquote after body element in second paste");

I don't think this is the correct check.  The bug was that the node was inserted AFTER body, not inside body.  In general, it's not a good idea to have a speculative PASS/FAIL like this unless you reverted the fix and confirmed that the test prints FAIL.
Comment 8 Ryosuke Niwa 2011-05-27 18:41:51 PDT
Comment on attachment 95239 [details]
Patch

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

>>> LayoutTests/editing/inserting/insert-3907422-fix.html:20
>>> +<body contenteditable id="root" class="editing" onload="runDumpAsTextEditingTest();">
>> 
>> I don't think we should change #test to #root either.  They are treated differently in editing.js
> 
> Whoops, didn't see this comment before. I didn't know "test" was a special id for editing.js. But of course "root" is too, and if I don't have a node with ID root, then I don't get output. I'm not sure how to refactor the test if we don't change body to id root--the point of the test is to do a select all, then copy/paste. If I added a containing root div, then the test would behave differently. If I put ids on divs that end up getting copy/pasted, the behavior of the test gets even harder to understand.
> 
> Maybe it would be better just not to use editing.js for this test?

Yeah, I don't think we should be using runDumpAsTextEditingTest for this test.  We should use dump-as-markup.js instead.
Comment 9 Annie Sullivan 2011-05-27 18:52:24 PDT
Created attachment 95244 [details]
Patch
Comment 10 Annie Sullivan 2011-05-27 18:53:45 PDT
(In reply to comment #9)
> Created an attachment (id=95244) [details]
> Patch

Okay, I converted to dump as markup, got rid of the explicit pass/fail. The entire HTML prints out in the test, which is a little verbose, but if we're worried about siblings being added to <body> I think it's the best we can do. I still think the test output is easier to read than before, and it'll definitely be easier to update now.
Comment 11 Ryosuke Niwa 2011-05-27 19:00:46 PDT
Thanks for converting the test!
Comment 12 WebKit Commit Bot 2011-05-27 23:18:19 PDT
The commit-queue encountered the following flaky tests while processing attachment 95244 [details]:

http/tests/websocket/tests/close-event.html bug 61240 (author: yutak@chromium.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2011-05-27 23:19:49 PDT
Comment on attachment 95244 [details]
Patch

Clearing flags on attachment: 95244

Committed r87604: <http://trac.webkit.org/changeset/87604>
Comment 14 WebKit Commit Bot 2011-05-27 23:19:56 PDT
All reviewed patches have been landed.  Closing bug.