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.

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

getInnerHTML shouldn't distinguish between open and closed #9

Open
annevk opened this issue Oct 29, 2020 · 8 comments
Open

getInnerHTML shouldn't distinguish between open and closed #9

annevk opened this issue Oct 29, 2020 · 8 comments

Comments

@annevk
Copy link

annevk commented Oct 29, 2020

At the very least I think includeShadowRoots should be renamed (and closedRoots isn't exactly clear either, especially without the other member) so that it is more clearly scoped, but I also thought that for new APIs we would try to design them in a way that they would work for either and not give priority to open shadow roots.

cc @rniwa

@mfreed7
Copy link
Owner

mfreed7 commented Mar 29, 2021

I'm definitely open to bikeshedding the names here - suggestions appreciated. The usage is low enough at this point that we can still rename things, if need be.

As for the open vs. closed shadow roots question - are you suggesting requiring all shadow roots be enumerated and provided, in order to serialize them? The reason for the closed shadow roots being provided is to avoid serializing them if we don't have "knowledge" of those. The idea here is essentially that you "usually" never provide the closedRoots parameter, and you just get all open shadow roots. So that kind of "is" working the same way for both:

  var html = document.body.getInnerHTML({includeShadowRoots: true});

returns html in which "open" roots are serialized because you can see them, and "closed" roots aren't because you can't. If you want to do something different than that, then you need the closedRoots parameter, but that's likely a very special case.

Perhaps I'm missing the point here - please help me understand. I would just like to avoid throwing a purely academic roadblock (requiring manual tree walk to gather all open shadow roots) in the way of using getInnerHTML().

@mfreed7
Copy link
Owner

mfreed7 commented Mar 29, 2021

Ok, I think I know what you are suggesting. Please correct me if I'm wrong here:

  var html1 = document.body.getComposedInnerHTML({includeOpenShadowRoots: true});
  var html2 = document.body.getComposedInnerHTML({shadowRoots: arrayOfShadowRoots});
  var html3 = document.body.getComposedInnerHTML({includeOpenShadowRoots: true, shadowRoots: arrayOfShadowRoots});

Here, html1 would contain all open shadow roots, and no closed shadow roots. html2 would only contain the shadow roots provided in the arrayOfShadowRoots array, whether they were open or closed. html3 would include all open shadow roots, plus any others that were included in arrayOfShadowRoots.

Said another way, the changes would be:

  • *** Rename getInnerHTML to getComposedInnerHTML
  • Rename includeShadowRoots to includeOpenShadowRoots
  • Rename closedRoots to shadowRoots
  • Change behavior so that the options are more independent, and either can be used without the other.

*** This change is motivated by this comment, and is intended to be parallel to the (future) selection API.

I'd be ok making these changes, if it would bring this API closer to agreement. Given that Chromium is shipping this overall API very soon, I'll need to move a bit quickly to get this changed in the implementation. So comments appreciated.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 30, 2021
See [1] for more context, but this CL makes the following changes
to the declarative Shadow DOM getInnerHTML API:

1. Rename getInnerHTML to getComposedInnerHTML
2. Rename includeShadowRoots to includeOpenShadowRoots
3. Rename closedRoots to shadowRoots
4. Change behavior so that the options are more independent, and
   either can be used without the other.

Mostly, the above is a rename operation, with the exception of #4.
There, the logic change is relatively minor, mostly happening in
markup_accumulator.cc around line 564.

Note: this also fixes the MeasureAs vs. RuntimeCallStats.

[1] mfreed7/declarative-shadow-dom#9 (comment)

Bug: 1042130
Change-Id: Ie4a0b18a2ef28f17b97eca33c018f7479fc20de8
@rniwa
Copy link

rniwa commented Apr 3, 2021

returns html in which "open" roots are serialized because you can see them, and "closed" roots aren't because you can't. If you want to do something different than that, then you need the closedRoots parameter, but that's likely a very special case.

What I really don't understand is the underlying use cases motivating the need for getInnerHTML in the first place. I can't imagine that all custom elements that have shadow roots need to serialize its shadow tree.

It's unclear that just because shadow tree is open, its content needs to be serialized; nor that just because a custom element uses a closed shadow tree that it doesn't need to serialize its contents. Why is this choice not made by each custom element's author? Why are users of components dictating whether a custom element's shadow tree will be serialized or not?

Perhaps I'm missing the point here - please help me understand. I would just like to avoid throwing a purely academic roadblock (requiring manual tree walk to gather all open shadow roots) in the way of using getInnerHTML().

This is not a purely academic issue. It's entirely unclear to me getInnerHTML as currently proposed solves any problem because nobody has clearly stated concrete use cases for it. We need to step back and enumerate concrete use cases for which this API was motivated, and decide what API will make sense.

In summary, I object to getInnerHTML API as currently proposed in the PR or the amendments you've proposed above.

I'm really frustrated that the current DOM PR includes this brand new feature that wasn't really discussed anywhere before. Why can't we just back this API out of the PR and add it later? Why do we need to tie them together?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 5, 2021
See [1] for more context, but this CL makes the following changes
to the declarative Shadow DOM getInnerHTML API:

1. Rename getInnerHTML to getComposedInnerHTML
2. Rename includeShadowRoots to includeOpenShadowRoots
3. Rename closedRoots to shadowRoots
4. Change behavior so that the options are more independent, and
   either can be used without the other.

Mostly, the above is a rename operation, with the exception of #4.
There, the logic change is relatively minor, mostly happening in
markup_accumulator.cc around line 564.

Note: this also fixes the MeasureAs vs. RuntimeCallStats.

[1] mfreed7/declarative-shadow-dom#9 (comment)

Bug: 1042130
Change-Id: Ie4a0b18a2ef28f17b97eca33c018f7479fc20de8
Cq-Do-Not-Cancel-Tryjobs: true
@mfreed7
Copy link
Owner

mfreed7 commented Apr 5, 2021

I'm really frustrated that the current DOM PR includes this brand new feature that wasn't really discussed anywhere before. Why can't we just back this API out of the PR and add it later? Why do we need to tie them together?

This part of the feature was included in the explainer from the beginning (February 7, 2020, 14 months ago), and the current form of this API (modulo the current bikeshedding) was actually proposed by a framework author, in this comment on Feb 12, 2020, only five days after the proposal was made public. In that comment, he makes the case (which I agree with) that the decision of what to serialize lies with the consumer of the HTML, not the component author. No other HTML element gets to decide which of its children (light or shadow) get serialized. As mentioned in the explainer, the most natural thing would be if regular .innerHTML returned shadow content, but that wouldn't be web compatible. Hence the new API.

@annevk what are your thoughts on the proposed changes? Do they properly address your concerns here?

@rniwa
Copy link

rniwa commented Apr 5, 2021

I'm really frustrated that the current DOM PR includes this brand new feature that wasn't really discussed anywhere before. Why can't we just back this API out of the PR and add it later? Why do we need to tie them together?

This part of the feature was included in the explainer from the beginning (February 7, 2020, 14 months ago), and the current form of this API (modulo the current bikeshedding) was actually proposed by a framework author, in this comment on Feb 12, 2020, only five days after the proposal was made public.

I mean this is what you wrote so I'm sure you included there. This topic hadn't really come up in prior discussions.

In that comment, he makes the case (which I agree with) that the decision of what to serialize lies with the consumer of the HTML, not the component author. No other HTML element gets to decide which of its children (light or shadow) get serialized.

textarea or input element doesn't serialize its dirty value as a part of HTML. Neither does canvas and its drawn content or style element after its stylesheet content were modified by scripts.

@annevk
Copy link
Author

annevk commented Apr 13, 2021

@rniwa makes a good point that other elements do not serialize their implementation details either. (I'm also still very much uncomfortable with the overall feature so it's somewhat hard to comment on specifics.)

@mfreed7
Copy link
Owner

mfreed7 commented Apr 23, 2021

@rniwa makes a good point that other elements do not serialize their implementation details either. (I'm also still very much uncomfortable with the overall feature so it's somewhat hard to comment on specifics.)

Isn't this exactly analogous to closed shadow roots, which (in the getComposedInnerHTML() proposal) will similarly not serialize their shadow roots automatically?

Personally, I think all of these element details should eventually be serializable. Besides the ones mentioned above (input/textarea content, canvas content, styles), other missing non-serializable things include AOM states and adoptedStyleSheets. HTML is intended to be the language used to describe a DOM tree. All such missing details that keep it from "round tripping" would seem to be holes in the language, at least to me. I do realize we can probably never achieve complete parity, but it seems reasonable to try.

@rniwa
Copy link

rniwa commented Apr 24, 2021

Personally, I think all of these element details should eventually be serializable. Besides the ones mentioned above (input/textarea content, canvas content, styles), other missing non-serializable things include AOM states and adoptedStyleSheets. HTML is intended to be the language used to describe a DOM tree. All such missing details that keep it from "round tripping" would seem to be holes in the language, at least to me. I do realize we can probably never achieve complete parity, but it seems reasonable to try.

HTML was never designed to be a roundtrip medium for DOM tree in the first place. The biggest problem I have with what's being proposed is the fact UA dictates how custom elements will get serialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants