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

UA shadow roots are observable via inheritance. #3748

Open
emilio opened this issue Jun 8, 2018 · 27 comments
Open

UA shadow roots are observable via inheritance. #3748

emilio opened this issue Jun 8, 2018 · 27 comments
Labels
topic: rendering topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@emilio
Copy link
Contributor

emilio commented Jun 8, 2018

See https://bugs.chromium.org/p/chromium/issues/detail?id=850872 for example, where implementing the right <slot> behavior regressed a website in Chrome because they use a UA shadow root for <object>.

A way to minimize the impact would be to spec that UA ShadowRoots should have slot { all: inherit; display: contents; } instead of just the default display: contents. But you could still observe it using display: inherit...

I'm not sure this is a frequent problem, I guess not, so feel free to resolve as you think it's more appropriate, but sounded worth letting the spec people know...
@annevk annevk added topic: rendering topic: shadow Relates to shadow trees (as defined in DOM) labels Jun 8, 2018
@annevk
Copy link
Member

annevk commented Jun 8, 2018

@whatwg/components I think it would be good if we kept UA shadow roots unobservable. Special casing inheritance of display seems unfortunate though. (And it's unclear if this is the only observable effect.)

@emilio
Copy link
Contributor Author

emilio commented Jun 8, 2018

If we're special-casing inheritance I'd rather special-case inheritance across UA shadow trees entirely (that is, making the content inheriting from the light tree parent in this case)...

That would be easy-ish to implement for Firefox, but not sure that's something that other engines would like to implement.

@lilles
Copy link
Contributor

lilles commented Jun 8, 2018

I haven't checked which UA roots + slots we have, but slot { all: inherit; display: contents } would still not work if the slot is not a direct descendant of the shadow root.

@emilio
Copy link
Contributor Author

emilio commented Jun 8, 2018

Oh, yeah, that's for sure. I was assuming all the UA shadow roots had <slot> as a direct descendant, but I guess that's not necessarily the case.

@lilles
Copy link
Contributor

lilles commented Jun 8, 2018 •
edited
Loading

Seems <meter> with -webkit-appearance:none has a slot wrapped in a div in Chrome.

This does not give a green border on the first one even with SlotInFlatTree disabled.

<!DOCTYPE html>
<p>Both borders green?</p>
<meter style="border-color:green; -webkit-appearance:none">
  <div style="border-style:solid; border-width:10px; border-color:inherit">Meter</div>
  <div style="border-style:solid; border-width:10px; border-color:green">Meter</div>
</meter>

The other element in Chrome I found is <marquee>

@emilio
Copy link
Contributor Author

emilio commented Jun 8, 2018

I wonder if @tabatkins / @rniwa have any opinions about this.

@domenic
Copy link
Member

domenic commented Jun 8, 2018

I don't really understand this issue; are UA shadow roots something that specifications govern? I thought it was just a way in which implementations reused some web technologies for their own internal implementation details, and thus outside the scope of specs.

@annevk
Copy link
Member

annevk commented Jun 8, 2018

@domenic the HTML Standard suggests using them for <details>. Also, if this cannot be fixed and leads sites to depend on the observable behavior we'll have to say something I suspect.

@tabatkins
Copy link
Contributor

I haven't checked which UA roots + slots we have, but slot { all: inherit; display: contents } would still not work if the slot is not a direct descendant of the shadow root.

Yeah, this is my problem with the attempted hack. It might work for current elements, but there's no guarantee that the slot will be top-level for future elements using UA shadow trees.

So I suspect we probably do want to special-case the inherit keyword here, at least for UA shadows. But maybe we should do it for all shadows? I mean, if a page author writes:

<foo-component>
  <p style="inherit: background">...
</foo-component>

They probably intend for the p to inherit from foo-component, regardless of what the shadow tree looks like.

So yeah, while standard inheritance still goes thru the flat tree, maybe the inherit keyword itself should inherit from the tree-of-trees parent?

@hayatoito
Copy link
Member

hayatoito commented Jun 11, 2018 •
edited
Loading

I tend to agree @domenic here. Basically, I think it is a browser vendor's responsibility to make "implementation details" un-observable.

@emilio Have you filed an issue for Chromium? At least, I am not aware of this issue so far.

At the same time, it would be unfortunate that browser vendor can't use UA shadow roots because of these accidental side effects. I wouldn't be surprised if there are other side effects which can be observable. Can someone notice other side effects?

@hayatoito
Copy link
Member

Ah, it is already tracked at https://bugs.chromium.org/p/chromium/issues/detail?id=850872.
@emilio Sorry for the noise.

@emilio
Copy link
Contributor Author

emilio commented Jun 11, 2018

I'd prefer not special-casing inherit FWIW, that means we'd need to track two inherited styles instead of just one through the style system.

Changing the whole inheritance parent would be easier IMO, I don't see the benefit of special-casing inherit: If UA shadow roots change inherited properties they'd be observable by default (without inherit usage from the author) already, which would be wrong, right?

@annevk
Copy link
Member

annevk commented Jun 11, 2018

@hayatoito if the specification prescribes using shadow roots to implement something, it could be read as if the side effects are intended. So I think either way we need to clarify the specification here.

@lilles
Copy link
Contributor

lilles commented Jun 11, 2018

I think that implementation-wise in Blink, it would be less intrusive to have a different inheritance parent for UA slotted nodes than special-casing the inherit keyword, unless we already rely on inheritance down from shadow tree elements.

@tabatkins
Copy link
Contributor

tabatkins commented Jun 11, 2018 •
edited
Loading

That seems fine to me. I can't imagine any UA shadow root wanting to affect the inheritance of a slotted element, and making this explicitly inherit differently prevents that from happening accidentally.

(And I'm fine with rejecting the proposal to make inherit work differently from inheritance in general, I was just chasing a thought.)

@emilio
Copy link
Contributor Author

emilio commented Jun 14, 2018

Would we want to do this for closed shadow roots as well? Or just for UA shadow roots?

@tabatkins
Copy link
Contributor

I can go either way on that, whatever implementations feel would be better.

@annevk
Copy link
Member

annevk commented Jun 15, 2018

Can we do it for all shadow roots?

@ExE-Boss
Copy link

I agree that doing this for all shadow roots probably makes the most sense, as Shadow DOM exists to hide implementation details.

@emilio
Copy link
Contributor Author

emilio commented Jun 16, 2018

Thinking more about it, it's likely such a model has somewhat subtle bugs. I've audited some places and we currently have flags that propagate on the style tree which assume they're following the flattened tree, and this could potentially break, like text-decoration-lines:

https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/servo/components/style/properties/computed_value_flags.rs#20

@hayatoito
Copy link
Member

hayatoito commented Jun 25, 2018 •
edited
Loading

I agree that doing this for all shadow roots probably makes the most sense, as Shadow DOM exists to hide implementation details.

That is not always correct. Shadow DOM exists to hide internal DOM tree structure, however, hiding every implementation details has never been a design goal of Shadow DOM. That would be a desired property, from some perspectives, however, that has been out of Shadow DOM's scope.

Shadow DOM affects almost every rendering result (e.g. how host or host's child is rendered), which are observable.

@rniwa
Copy link

rniwa commented Jun 25, 2018

I think we should do this for all shadow DOM if we're doing it at all.

@TakayoshiKochi
Copy link
Member

Hiding the implementation details for UA shadow and applying it to author shadow are two separate issues, and let's focus on the original UA shadow's case here.

The problem is, that if an element is implemented using UA shadow root, not only the existence is observable but also it has interoperability issue with browsers implementing the element natively.

@rniwa
Copy link

rniwa commented Jun 26, 2018

Hiding the implementation details for UA shadow and applying it to author shadow are two separate issues, and let's focus on the original UA shadow's case here.

I don't see why these are two separate concerns. The fundamental problem here is that the presence of a slot element is causing the inheritance to not work the way users of a builtin element expected. I see no difference in this versus the expectation users of a Web component would have.

The fact even UA implementors screw this up is a good evidence that we need to provide a better default behavior. Our expectation can't be that developers who write web components are more knowledgeable about CSS than Web engine developers.

@TakayoshiKochi
Copy link
Member

It's totally fine from your perspective "always inheriting styles from shadow host to slot assignees" is a superset of the solution to "UA shadow is leaking the implementation details". I'm saying that fixing UA shadow case does not have to be tied together to non-UA shadows.

Changing the inheritance for non-UA shadows would involve spec change and potential breaks in existing sites, fixing only UA shadows does not require spec change, and should improve interoperability whether or not an element is implemented using UA shadow.

@smaug---- smaug---- mentioned this issue Jun 27, 2018
Closed
@rniwa
Copy link

rniwa commented Oct 25, 2018

TPAC F2F WebPlat: This probably needs to be discussed in the CSS WG since this would affect inheritance.

We think this is a high priority bug because it leaks the information that shadow root exists on a given element thereby breaking the encapsulation.

@annevk annevk mentioned this issue Oct 25, 2018
Open
@xfq
Copy link
Contributor

xfq commented Oct 25, 2018 •
edited
Loading

CSSWG issue: w3c/csswg-drafts#3248

@rniwa rniwa mentioned this issue Nov 16, 2018
Closed
@annevk annevk mentioned this issue Mar 28, 2019
Open
@annevk annevk mentioned this issue Feb 2, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: rendering topic: shadow Relates to shadow trees (as defined in DOM)
Milestone
No milestone
Development

No branches or pull requests
10 participants
@hayatoito @rniwa @domenic @tabatkins @lilles @emilio @annevk @xfq @ExE-Boss @TakayoshiKochi