Bug 61680

Summary: editing/inserting/insert-3907422 should be dumpAsText test
Product: WebKit Reporter: Annie Sullivan <sullivan>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 34564    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Annie Sullivan
Reported 2011-05-27 18:22:09 PDT
The output would be much easier to understand as dumpAsText.
Attachments
Patch (42.42 KB, patch)
2011-05-27 18:24 PDT, Annie Sullivan 		no flags
DetailsFormatted DiffDiff
Patch (42.35 KB, patch)
2011-05-27 18:32 PDT, Annie Sullivan 		no flags
DetailsFormatted DiffDiff
Patch (42.99 KB, patch)
2011-05-27 18:52 PDT, Annie Sullivan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Annie Sullivan
Comment 1 2011-05-27 18:24:10 PDT
Created attachment 95239 [details] Patch
Ryosuke Niwa
Comment 2 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.
Ryosuke Niwa
Comment 3 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
Annie Sullivan
Comment 4 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.
Annie Sullivan
Comment 5 2011-05-27 18:32:52 PDT
Created attachment 95242 [details] Patch
Annie Sullivan
Comment 6 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?
Ryosuke Niwa
Comment 7 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.
Ryosuke Niwa
Comment 8 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.
Annie Sullivan
Comment 9 2011-05-27 18:52:24 PDT
Created attachment 95244 [details] Patch
Annie Sullivan
Comment 10 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.
Ryosuke Niwa
Comment 11 2011-05-27 19:00:46 PDT
Thanks for converting the test!
WebKit Commit Bot
Comment 12 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.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2011-05-27 23:19:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.