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

Closed shadow roots can be serializable too #10260

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Closed shadow roots can be serializable too #10260

merged 2 commits into from
Apr 10, 2024

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 8, 2024 •
edited by pr-preview bot
Loading

I missed this in review but my understanding of the discussion we had around #8867 was that the contract would be the same for open and closed shadow roots.

(See WHATWG Working Mode: Changes for more details.)

/dynamic-markup-insertion.html ( diff )
/parsing.html ( diff )

@annevk
Closed shadow roots can be serializable too
432ad78 
I missed this in review but my understanding of the discussion we had around #8867 was that the contract would be the same for open and closed shadow roots.
@annevk
Copy link
Member Author

annevk commented Apr 8, 2024

@smaug---- does this match your understanding?

cc @mfreed7

@annevk

This comment was marked as resolved.

Sign in to view
@mfreed7
Copy link
Contributor

mfreed7 commented Apr 8, 2024

@smaug---- does this match your understanding?

cc @mfreed7

Hmm that’s not what we discussed. I thought you needed to explicitly provide closed roots to get them serialized, hence the existing spec language. I don’t have strong feelings about this change though. The tests will need to be changed.

Another thing I noticed that might need updating: GetHTMLOptions options = {} should probably be denoted as optional argument of getHTML().

I agree.

@annevk
Copy link
Member Author

annevk commented Apr 8, 2024

I think it is what we discussed. The whole idea with serializable was that it was a clear opt-in and thus not violating encapsulation. We don't have detailed minutes of WHATNOT though so I guess at best we can disagree. I looked at the issue and it does not document this aspect there.

@annevk annevk mentioned this pull request Apr 8, 2024
Merged
@annevk annevk added normative change topic: shadow Relates to shadow trees (as defined in DOM) labels Apr 9, 2024
annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 9, 2024
@annevk
Remove getHTML() shouldn't branch on shadow root's mode
cffaf77 
As with clonable and delegatesFocus, serializable is an explicit API and therefore fine from an encapsulation point of view.

For whatwg/html#10260.
@annevk annevk mentioned this pull request Apr 9, 2024
Merged
@annevk annevk requested review from smaug---- and mfreed7 April 9, 2024 10:10
@annevk
Copy link
Member Author

annevk commented Apr 9, 2024

Thinking about this more than I probably should I'm not sure how I would ever have agreed to this and therefore I'm pretty sure I haven't. This creates a difference between open and closed, which is the one thing we explicitly wanted to avoid.

It makes a lot more sense for this to be consistent with clonable and delegatesFocus.

I updated the tests.

(Explicitly listing shadow roots still has value in that you might want to serialize shadow roots not declared as serializable under certain conditions, although from my perspective that could have been left out as a v2 API.)

@mfreed7
Copy link
Contributor

mfreed7 commented Apr 9, 2024

Thinking about this more than I probably should I'm not sure how I would ever have agreed to this and therefore I'm pretty sure I haven't. This creates a difference between open and closed, which is the one thing we explicitly wanted to avoid.

It makes a lot more sense for this to be consistent with clonable and delegatesFocus.

I updated the tests.

(Explicitly listing shadow roots still has value in that you might want to serialize shadow roots not declared as serializable under certain conditions, although from my perspective that could have been left out as a v2 API.)

Ok. As I said, I don't have strong feelings about this. I'll review the PR and the tests.

domenic
domenic reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Can you update the domintro too?

@annevk
Copy link
Member Author

annevk commented Apr 10, 2024

Thanks, done.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Apr 10, 2024
@annevk
Remove getHTML() shouldn't branch on shadow root's mode
dd2e51d 
As with clonable and delegatesFocus, serializable is an explicit API and therefore fine from an encapsulation point of view.

For whatwg/html#10260.
@annevk annevk merged commit f2089ab into main Apr 10, 2024
2 checks passed
@annevk annevk deleted the annevk/html branch April 10, 2024 06:51
webkit-commit-queue pushed a commit to annevk/WebKit that referenced this pull request Apr 11, 2024
@annevk
Add serializable shadow roots and getHTML()
de64e62 
https://bugs.webkit.org/show_bug.cgi?id=271343
rdar://125513986

Reviewed by Ryosuke Niwa.

This implements whatwg/html#10139 as amended by
whatwg/html#10260.

This also changes the way a shadow root is serialized to avoid creating
a template element.

The API is guarded by DeclarativeShadowRootsSerializerAPIsEnabled just
in case there is a need to disable it without removing the code.

Thanks to Chris Dumez for his help with Vector<RefPtr<ShadowRoot>>.

WPT tests are synchronized up to and including this commit:
web-platform-tests/wpt@dd2e51d

This also adjusts the copyright lines to match the commit log for the
relevant files.

* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats.html:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml-ordering-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml-ordering.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml.tentative-expected.txt: Removed.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/gethtml.tentative.html: Removed.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/w3c-import.log:
* LayoutTests/tests-options.json:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/CMakeLists.txt:
* Source/WebCore/DerivedSources-input.xcfilelist:
* Source/WebCore/DerivedSources-output.xcfilelist:
* Source/WebCore/DerivedSources.make:
* Source/WebCore/Headers.cmake:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::getHTML const):
* Source/WebCore/dom/Element.h:
* Source/WebCore/dom/GetHTMLOptions.h: Added.
* Source/WebCore/dom/GetHTMLOptions.idl: Added.
* Source/WebCore/dom/InnerHTML.idl:
* Source/WebCore/dom/ShadowRoot.cpp:
(WebCore::ShadowRoot::ShadowRoot):
(WebCore::ShadowRoot::getHTML const):
(WebCore::ShadowRoot::cloneNodeInternal):
* Source/WebCore/dom/ShadowRoot.h:
* Source/WebCore/dom/ShadowRoot.idl:
* Source/WebCore/dom/ShadowRootInit.h:
* Source/WebCore/dom/ShadowRootInit.idl:
* Source/WebCore/editing/MarkupAccumulator.cpp:
(WebCore::MarkupAccumulator::MarkupAccumulator):
(WebCore::MarkupAccumulator::shouldIncludeShadowRoots const):
(WebCore::MarkupAccumulator::includeShadowRoot const):
(WebCore::MarkupAccumulator::serializeNodesWithNamespaces):
(WebCore::MarkupAccumulator::replacementElement):
* Source/WebCore/editing/MarkupAccumulator.h:
(WebCore::MarkupAccumulator::MarkupAccumulator):
* Source/WebCore/editing/markup.cpp:
(WebCore::serializeFragment):
* Source/WebCore/editing/markup.h:
(WebCore::serializeFragment):
* Source/WebCore/html/HTMLAttributeNames.in:
* Source/WebCore/html/HTMLTemplateElement.cpp:
(WebCore::HTMLTemplateElement::attachAsDeclarativeShadowRootIfNeeded):
* Source/WebCore/html/HTMLTemplateElement.idl:
* Source/WebCore/html/parser/HTMLConstructionSite.cpp:
(WebCore::HTMLConstructionSite::insertHTMLTemplateElement):
* Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:
(WebCore::LegacyWebArchive::create):

Canonical link: https://commits.webkit.org/277374@main
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 12, 2024
@mfreed7
Remove distinction for open roots only for serializableShadowRoots
00d1bed 
Per the spec change:

  whatwg/html#10260

This is a small behavior change. If `{serializableShadowRoots:true}`
and a given shadow root is `serializable`, then serialize it, even
if it is closed.

Tests should flow in from this upstream WPT change:

  web-platform-tests/wpt#45624

Bug: 41490936
Change-Id: I85f239599dda60637ff6301d1a501f9cce24a861
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5448166
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1286531}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2024
@annevk @moz-wptsync-bot
Bug 1890506 [wpt PR 45624] - Remove getHTML() shouldn't branch on sha…
6e4ee7a 
…dow root's mode, a=testonly

Automatic update from web-platform-tests
Remove getHTML() shouldn't branch on shadow root's mode

As with clonable and delegatesFocus, serializable is an explicit API and therefore fine from an encapsulation point of view.

For whatwg/html#10260.
--

wpt-commits: dd2e51de9985a1c9d381607c2b314b6562545d37
wpt-pr: 45624
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Apr 16, 2024
@annevk @moz-wptsync-bot
Bug 1890506 [wpt PR 45624] - Remove getHTML() shouldn't branch on sha…
8ebcc0b 
…dow root's mode, a=testonly

Automatic update from web-platform-tests
Remove getHTML() shouldn't branch on shadow root's mode

As with clonable and delegatesFocus, serializable is an explicit API and therefore fine from an encapsulation point of view.

For whatwg/html#10260.
--

wpt-commits: dd2e51de9985a1c9d381607c2b314b6562545d37
wpt-pr: 45624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfreed7 mfreed7 mfreed7 approved these changes

@domenic domenic domenic approved these changes

@smaug---- smaug---- Awaiting requested review from smaug----

Assignees
No one assigned
Labels
normative change topic: shadow Relates to shadow trees (as defined in DOM)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@annevk @mfreed7 @domenic