Bug 226808 – Relax "parent must be an HTMLElement" restriction in outerHTML setter

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[[e-mail]]

Steps to reproduce:

(1) Open a tab and navigate to https://www.cdc.gov/;
(2) Run the following code in the Console of Devtools:
```
document.getElementById('sas').outerHTML = "any_str";
```
(3) Then this code would throw a NoModificationAllowedError exception.

Actual results:

this code would throw a NoModificationAllowedError exception: `NoModificationAllowedError: The object can not be modified.`

Expected results:

In my test, both Chrome and Firefox would not throw any exceptions. https://w3c.github.io/DOM-Parsing/#outerhtml says `If parent is a Document, throw a "NoModificationAllowedError" DOMException.` Since the parent node is not a Document, maybe something wrong with the outerHTML setter.

[Chris Dumez]

Parent of document.getElementById('sas') is an <svg> element. We have code throwing a NoModificationAllowedError if the parent is NOT an HTML element.

[Chris Dumez]

Created attachment 431266 [details]
WIP Patch

[Chris Dumez]

Looks like WPT testing is a bit lacking in this area. No test result changes in the domparsing/ folder.

[Chris Dumez]

There is also quite a bit of disagreement between the different browsers here. Gecko mostly matches the specification but our behavior is mostly aligned with Blink. I'll make the change to allow non-HTMLElement parents though since Firefox & Blink agree on that at least.

[Chris Dumez]

Created attachment 431268 [details]
Patch

[Chris Dumez]

Created attachment 431269 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 431269 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        - WebKit & Blink have some Text node merging logic that is not present in the specification
> +          and which Gecko doesn't implement.

Could you file a spec issue for this? It seems like Gecko should implement this behavior as well.

[Chris Dumez]

Created attachment 431281 [details]
Patch

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 431269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431269&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        - WebKit & Blink have some Text node merging logic that is not present in the specification
> > +          and which Gecko doesn't implement.
> 
> Could you file a spec issue for this? It seems like Gecko should implement
> this behavior as well.

Sure thing: https://github.com/w3c/DOM-Parsing/issues/70

[Darin Adler]

Comment on attachment 431269 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:3252
> +    // The specification allows setting outerHTML on an Element whose parent is a DocumentFragment and Gecko supports this.
> +    // However, as of June 2021, Blink matches our behavior and throws a NoModificationAllowedError for non-Element parents.

Can we apply gentle pressure to resolve this so the browsers, specification, and WPT all agree?

> Source/WebCore/dom/Element.cpp:3255
> +        return Exception { NoModificationAllowedError, "Cannot set outerHTML on element because its parent is not an Element" };

Peculiar wording when called on something with no parent.

> Source/WebCore/dom/Element.cpp:3268
> +    // The following is not part of the specification but matches Blink's behavior as of June 2021.

Pretty sure this is just because of our "shared heritage" since Blink is a fork of WebKit.

[Darin Adler]

I would have titled this bug something more like this:

    Relax "parent must be HTMLElement" restriction on outerHTML

I know the goal is to align with other browsers and the specification, but that part of the story is complicated since the two other major browsers don’t match each other and this doesn’t seem to be covered in WPT.

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> Comment on attachment 431269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431269&action=review
> 
> > Source/WebCore/dom/Element.cpp:3252
> > +    // The specification allows setting outerHTML on an Element whose parent is a DocumentFragment and Gecko supports this.
> > +    // However, as of June 2021, Blink matches our behavior and throws a NoModificationAllowedError for non-Element parents.
> 
> Can we apply gentle pressure to resolve this so the browsers, specification,
> and WPT all agree?

As mentioned earlier, WPT testing seems to be lacking here and I did file an issue against the spec to start a discussion.

> 
> > Source/WebCore/dom/Element.cpp:3255
> > +        return Exception { NoModificationAllowedError, "Cannot set outerHTML on element because its parent is not an Element" };
> 
> Peculiar wording when called on something with no parent.

I personally felt that the "parent is not an Element" statement was true if the parent was null :) I didn't think it was worth it to add an extra if check just to provide better logging. But given your comment, I'll add it.

> 
> > Source/WebCore/dom/Element.cpp:3268
> > +    // The following is not part of the specification but matches Blink's behavior as of June 2021.
> 
> Pretty sure this is just because of our "shared heritage" since Blink is a
> fork of WebKit.

That may well be but then again we were not totally matching Blink either and we do count as separate major engines nowadays when it comes to spec'ing.

[Chris Dumez]

(In reply to Darin Adler from comment #11)
> I would have titled this bug something more like this:
> 
>     Relax "parent must be HTMLElement" restriction on outerHTML
> 
> I know the goal is to align with other browsers and the specification, but
> that part of the story is complicated since the two other major browsers
> don’t match each other and this doesn’t seem to be covered in WPT.

Right, I did not file that bug and I should have re-titled it when I found out what the actual issue was. It's not too late though so I'll fix.

[Chris Dumez]

Created attachment 431282 [details]
Patch

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> (In reply to Darin Adler from comment #10)
> > Pretty sure this is just because of our "shared heritage" since Blink is a
> > fork of WebKit.
> 
> That may well be but then again we were not totally matching Blink either
> and we do count as separate major engines nowadays when it comes to spec'ing.

Actually looks like this merging was added by an engineer working on Chrome:

https://bugs.webkit.org/show_bug.cgi?id=52686

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> (In reply to Chris Dumez from comment #12)
> > (In reply to Darin Adler from comment #10)
> > > Pretty sure this is just because of our "shared heritage" since Blink is a
> > > fork of WebKit.
> > 
> > That may well be but then again we were not totally matching Blink either
> > and we do count as separate major engines nowadays when it comes to spec'ing.
> 
> Actually looks like this merging was added by an engineer working on Chrome:
> 
> https://bugs.webkit.org/show_bug.cgi?id=52686

Thanks, this is useful information indeed and I have added it to my spec bug report.

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> I personally felt that the "parent is not an Element" statement was true if
> the parent was null :) I didn't think it was worth it to add an extra if
> check just to provide better logging. But given your comment, I'll add it.

I think we could also word something so it doesn’t seem strange, without adding a second message. To me saying "parent is not an Element" implied that there is a parent and it’s not an Element. Could have said "because it doesn't have a parent that is an Element".

[Chris Dumez]

(In reply to Darin Adler from comment #17)
> (In reply to Chris Dumez from comment #12)
> > I personally felt that the "parent is not an Element" statement was true if
> > the parent was null :) I didn't think it was worth it to add an extra if
> > check just to provide better logging. But given your comment, I'll add it.
> 
> I think we could also word something so it doesn’t seem strange, without
> adding a second message. To me saying "parent is not an Element" implied
> that there is a parent and it’s not an Element. Could have said "because it
> doesn't have a parent that is an Element".

The extra branch is not in the hot code path so I figure we might as well provide a more accurate error message. This is what I did in the latest iteration.

[Chris Dumez]

Created attachment 431284 [details]
Patch

[EWS]

Committed r278821 (238774@main): <https://commits.webkit.org/238774@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431284 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79264001>