| Summary: | editing/inserting/insert-3907422 should be dumpAsText test | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Annie Sullivan <sullivan> | ||||||||
| Component: | HTML Editing | Assignee: | 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
Annie Sullivan
2011-05-27 18:22:09 PDT
Created attachment 95239 [details]
Patch
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 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 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. Created attachment 95242 [details]
Patch
(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 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 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. Created attachment 95244 [details]
Patch
(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. Thanks for converting the test! 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 on attachment 95244 [details] Patch Clearing flags on attachment: 95244 Committed r87604: <http://trac.webkit.org/changeset/87604> All reviewed patches have been landed. Closing bug. |