Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Setting outerHTML on child of DocumentFragment should not throw error #15575

Merged
merged 1 commit into from Jul 15, 2023
Merged

Setting outerHTML on child of DocumentFragment should not throw error #15575

merged 1 commit into from Jul 15, 2023

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Jul 5, 2023 •
edited by webkit-early-warning-system

b41af45

Setting outerHTML on child of DocumentFragment should not throw error

https://bugs.webkit.org/show_bug.cgi?id=249737
rdar://problem/103746193

Reviewed by Chris Dumez.

This patch aligns WebKit with Gecko / Firefox and Web-Spec [1].

In 238774@main, WebKit aligned with behavior for better compatibility but
it was not correct and clarified later to not throw error [2]. This PR is
essentially revert part of 238774@main with update to align by not throwing error.

[1] https://w3c.github.io/DOM-Parsing/#dom-element-outerhtml
[2] whatwg/html#8657

* Source/WebCore/dom/Element.cpp:
(Element::setOuterHTML): Update to not throw error
* LayoutTests/fast/dynamic/outerHTML-no-element.html: Rebaselined
* LayoutTests/fast/dynamic/outerHTML-no-element-expected.txt: Rebaselined
* LayoutTests/fast/dom/set-outer-html-special-cases.html: Rebaselined
* LayoutTests/fast/dom/set-outer-html-special-cases-expected.txt: Rebaselined

Canonical link: https://commits.webkit.org/266086@main

cdf5c2e

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Jul 5, 2023
@Ahmad-S792 Ahmad-S792 self-assigned this Jul 5, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash ea6917e)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 5, 2023
@Ahmad-S792
Copy link
Contributor Author

@cdumez - We have genuine WPT test failure (domparsing/outerhtml-01.html), we pass, if we return error. Although Firefox passes WPT test plus also don't throw error.

@Ahmad-S792
Copy link
Contributor Author

@cdumez - We have genuine WPT test failure (domparsing/outerhtml-01.html), we pass, if we return error. Although Firefox passes WPT test plus also don't throw error.

Figured out locally on how to fix it. We don't need to revert completely but just change 'return' for one specific type of case. Will do updated push tomorrow (late here - 12 AM).

@Ahmad-S792 Ahmad-S792 force-pushed the fix249737-outerHTML-on-child-of-DocumentFragment-throw-error branch from ea6917e to ca7fb05 Compare July 15, 2023 13:32
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash ca7fb05)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 force-pushed the fix249737-outerHTML-on-child-of-DocumentFragment-throw-error branch from ca7fb05 to cdf5c2e Compare July 15, 2023 18:40
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash cdf5c2e)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Jul 15, 2023
@Ahmad-S792
Copy link
Contributor Author

@cdumez - Ready for your review now. :-) We are now matching Firefox.

cdumez
cdumez approved these changes Jul 15, 2023
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Jul 15, 2023
@Ahmad-S792 @cdumez
Setting outerHTML on child of DocumentFragment should not throw error
b41af45 
https://bugs.webkit.org/show_bug.cgi?id=249737
rdar://problem/103746193

Reviewed by Chris Dumez.

This patch aligns WebKit with Gecko / Firefox and Web-Spec [1].

In 238774@main, WebKit aligned with behavior for better compatibility but
it was not correct and clarified later to not throw error [2]. This PR is
essentially revert part of 238774@main with update to align by not throwing error.

[1] https://w3c.github.io/DOM-Parsing/#dom-element-outerhtml
[2] whatwg/html#8657

* Source/WebCore/dom/Element.cpp:
(Element::setOuterHTML): Update to not throw error
* LayoutTests/fast/dynamic/outerHTML-no-element.html: Rebaselined
* LayoutTests/fast/dynamic/outerHTML-no-element-expected.txt: Rebaselined
* LayoutTests/fast/dom/set-outer-html-special-cases.html: Rebaselined
* LayoutTests/fast/dom/set-outer-html-special-cases-expected.txt: Rebaselined

Canonical link: https://commits.webkit.org/266086@main
@webkit-commit-queue webkit-commit-queue force-pushed the fix249737-outerHTML-on-child-of-DocumentFragment-throw-error branch from cdf5c2e to b41af45 Compare July 15, 2023 22:06
@webkit-commit-queue
Copy link
Collaborator

Committed 266086@main (b41af45): https://commits.webkit.org/266086@main

Reviewed commits have been landed. Closing PR #15575 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit b41af45 into WebKit:main Jul 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 15, 2023
@Ahmad-S792 Ahmad-S792 deleted the fix249737-outerHTML-on-child-of-DocumentFragment-throw-error branch July 15, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

Assignees

@Ahmad-S792 Ahmad-S792

Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
Milestone
No milestone
5 participants
@Ahmad-S792 @webkit-early-warning-system @webkit-commit-queue @cdumez @webkit-ews-buildbot