Summary: | Relax "parent must be an HTMLElement" restriction in outerHTML setter | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zyscoder <zyscoder> | ||||||||||||||
Component: | WebKit Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cdumez, cmarcelo, darin, dbarton, esprehn+autocc, ews-watchlist, fred.wang, ggaren, kangil.han, rniwa, sam, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Linux | ||||||||||||||||
URL: | https://github.com/w3c/DOM-Parsing/issues/70 | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=52686 http://www.w3.org/Bugs/Public/show_bug.cgi?id=12584 https://bugs.webkit.org/show_bug.cgi?id=249737 |
||||||||||||||||
Attachments: |
|
Description
zyscoder@gmail.com
2021-06-09 02:15:21 PDT
Parent of document.getElementById('sas') is an <svg> element. We have code throwing a NoModificationAllowedError if the parent is NOT an HTML element. Created attachment 431266 [details]
WIP Patch
Looks like WPT testing is a bit lacking in this area. No test result changes in the domparsing/ folder. 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. Created attachment 431268 [details]
Patch
Created attachment 431269 [details]
Patch
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. Created attachment 431281 [details]
Patch
(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 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. 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. (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. (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. Created attachment 431282 [details]
Patch
(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 (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. (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". (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. Created attachment 431284 [details]
Patch
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]. |