Bug 88681

Summary:

WebKit doesn't allow replacing the document element with a DocumentFragment containing one element

Product:

WebKit

Reporter:

Ms2ger (he/him; ⌚ UTC+1/+2) <Ms2ger>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, esprehn, ojan, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (6.65 KB, patch) 2012-06-11 16:19 PDT, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch (14.53 KB, patch) 2012-06-11 17:59 PDT, Elliott Sprehn	no flags	Details Formatted Diff Diff
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.