RESOLVED FIXED 88681
WebKit doesn't allow replacing the document element with a DocumentFragment containing one element 
https://bugs.webkit.org/show_bug.cgi?id=88681
Summary WebKit doesn't allow replacing the document element with a DocumentFragment c...
Ms2ger (he/him; ⌚ UTC+1/+2)
Reported 2012-06-08 13:21:42 PDT
Created attachment 146636 [details] Patch Found by <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Node-replaceChild.html>. Can either of you turn this patch into something acceptable?
Attachments
Patch (829 bytes, patch)
2012-06-08 13:21 PDT, Ms2ger (he/him; ⌚ UTC+1/+2) 		no flags
DetailsFormatted DiffDiff
Patch (6.65 KB, patch)
2012-06-11 16:19 PDT, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Patch (14.53 KB, patch)
2012-06-11 17:59 PDT, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2012-06-11 16:19:15 PDT
Created attachment 146952 [details] Patch
Ryosuke Niwa
Comment 2 2012-06-11 16:30:48 PDT
Comment on attachment 146952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146952&action=review > Source/WebCore/ChangeLog:8 > + Covered by existing tests with some modifications. Please explain how this bug manifested and how you fixed it. r- due to the lack of description. Also, please add URLs of relevant specifications. > LayoutTests/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description but below the bug URL.
Ojan Vafai
Comment 3 2012-06-11 16:35:03 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=146952&action=review > Source/WebCore/ChangeLog:8 > + Covered by existing tests with some modifications. No need to spell this out if the test is clearly testing the new code. You only need to justify leaving out tests if it's not obvious that existing tests cover the code (e.g. if no tests output changes or if you're making a big change where the changed test output only seems to cover part of the change). > Source/WebCore/ChangeLog:11 > + (WebCore::Document::canReplaceChild): Can use a line describing the fix, e.g. "When the new child was a DocumentFragment we were incorrectly iterating over the Document's children instead of the DocumentFragment's children." Part of the point of these change descriptions is so people can quickly decide if they want to take the time to look at the patch in detail, so even though that's obvious by a quick look at the patch, the change description is useful. > LayoutTests/fast/dom/Document/replace-child-expected.txt:1 > +ALERT: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> I think this is out of date from what you uploaded. > LayoutTests/fast/dom/Document/replace-child.html:41 > + debug('replacing element with element in fragment') > + try { > + var doc = parser.parseFromString('<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"><foo/>', "text/xml") > + var fragment = doc.createDocumentFragment(); > + fragment.appendChild(doc.createElement('bar')); > + doc.replaceChild(fragment, doc.documentElement); > + } catch (ex) { > + debug('FAILED: ' + ex) > + return; > + } > + debug('SUCCESS: ' + serializer.serializeToString(doc)); > + successCount++; Can you use shouldNotThrow from js-test-pre.js here and shouldThrow below? Then you wouldn't need the successCount stuff either. I know the test is already doing this, but the test is dumb. :) > LayoutTests/fast/dom/Document/replace-child.html:47 > + var doc = parser.parseFromString('<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"><foo/>', "text/xml") Here and above, can you just use the HTML doctype of <!DOCTYPE html>. I don't see any benefit from using this long doctype just to be consistent with the rest of the uses in this test. Feel free to update the other cases in this test too if you want. > LayoutTests/fast/dom/Document/replace-child.html:50 > + fragment.appendChild(doc.createElement('bar')); > + fragment.appendChild(doc.createElement('baz')); It's really weird to me that this test uses unknown tagnames, but I suppose it's fine given that the other test-cases here do that.
Elliott Sprehn
Comment 4 2012-06-11 17:59:03 PDT
Created attachment 146981 [details] Patch Made
Elliott Sprehn
Comment 5 2012-06-11 18:02:59 PDT
(Err. Missed some quotes in the shell there...) Made the requested changes and rewrote that test to be much shorter and use the shouldThrow/shouldNotThrow functions.
Ojan Vafai
Comment 6 2012-06-11 20:55:45 PDT
Comment on attachment 146981 [details] Patch Wow, that's 100x more readable now.
WebKit Review Bot
Comment 7 2012-06-11 21:10:58 PDT
Comment on attachment 146981 [details] Patch Clearing flags on attachment: 146981 Committed r120037: <http://trac.webkit.org/changeset/120037>
WebKit Review Bot
Comment 8 2012-06-11 21:11:08 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.