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

Fix update a style block to match reality and CSSWG resolution. #6294

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 15, 2021

No browser passed the test-case that was linked from the comment.

The algorithm was also inconsistent, as it looked at whether
the element was in a shadow tree or in the document tree, but it was
only specified to be re-run if the element becomes connected or
disconnected.

The CSSWG discussed this in
w3c/csswg-drafts#3096 (comment)
and there are WPTs for this.

This also matches closer the definition of which does
use connectedness (though it uses "browsing-context connected", which is a
bit different): https://html.spec.whatwg.org/#link-type-stylesheet

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


/semantics.html ( diff )

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.

LGTM but I would appreciate an @annevk double-check.

@LJ1102
Copy link

LJ1102 commented Jan 15, 2021

The way I understand the discussion linked here the conclusion was for the stylesheet to be parsed but not have any imports execute until the node is being connected.
Changing stylesheets programmatically is a necessity when trying to dynamically style pseudo-elements. Imo initializing those styles is clearly a concern of the constructor of a custom element and shouldn't need to be done in the connection callback.

@emilio
Copy link
Contributor Author

emilio commented Jan 15, 2021

That sounds about right, but:

  • It doesn't really make sense that <style> and <link> are inconsistent here, IMO.
  • This PR is effectively reflecting actual interoperable browser behavior.
  • There's constructable stylesheets if you need to manage the style sheet lifetime independent of the <style> or <link> element, and they were mentioned in the discussion in the CSSWG mentioned in the first comment.

So for what you want, I think constructable stylesheets is what solves your use case. Having <style> and <link> work inconsistently here (same for document.styleSheets not reflecting stuff that element.sheet reflects).

@LJ1102
Copy link

LJ1102 commented Jan 16, 2021

I don't feel like style and link are inconsistent, they're consistent in that anything asynchronous is deferred until the node is connected to the browsing context, if that's what you're referring to? Expecting that immediate CSS within a style element is parsed into a sheet when the element becomes part of any DOM seems reasonable to me.

Apart from not knowing what the future holds for constructable stylesheets in regards to adoption and subsequent API stability I don't really feel like it's a good fit for my use case. What I need is instance level programmatic control over pseudo classes; and while I could construct a stylesheet on a per instance basis using bespoken API and "adopt" it into the shadowroot, it seems to be quite redundant considering I'm having a style element in my shadow dom, which I believe to be a quite common and well supported pattern to build self-contained components.

The current workaround is to cache the values in member variables or custom attributes and then sync them in the connectedCallback which is a solution I'm not happy with as it pollutes my class definition and requires me to implement the callback and subsequent logic regarding the caching.

I feel like the status quo shouldn't really affect the decision on this but let me ask you this: if I were to do a PR on a major browser implementing bespoken behavior would that change your stance on it?

@emilio
Copy link
Contributor Author

emilio commented Jan 16, 2021

I don't feel like style and link are inconsistent, they're consistent in that anything asynchronous is deferred until the node is connected to the browsing context, if that's what you're referring to? Expecting that immediate CSS within a style element is parsed into a sheet when the element becomes part of any DOM seems reasonable to me.

That's not how styles work outside of shadow DOM, so it doesn't seem all that sensible to me. How is @import supposed to work? Lazily, I guess? (as in, start the load after the node becomes connected?). If so, it's not clear what the model for load / error events on <style> elements would be. Browsers don't expose complete sheets in ShadowRoot/document.styleSheets until @imports are loaded (though there might be some variance here). It'd be really weird for .styleSheets to shrink after inserting a shadow root into a document.

The reason why I filed w3c/csswg-drafts#3096 to begin with is because browsers were doing super-weird stuff when I was implementing Shadow DOM v1 in Firefox.

What I need is instance level programmatic control over pseudo classes; and while I could construct a stylesheet on a per instance basis using bespoken API and "adopt" it into the shadowroot, it seems to be quite redundant considering I'm having a style element in my shadow dom, which I believe to be a quite common and well supported pattern to build self-contained components.

Can you show some code so I can take a look at what you're doing? Browsers try very hard to share <style> elements across Shadow DOM, and accessing the stylesheets programmatically will destroy that sharing, fwiw. So if your <style> is large enough your approach will cause a lot of memory overhead.

I feel like the status quo shouldn't really affect the decision on this but let me ask you this: if I were to do a PR on a major browser implementing bespoken behavior would that change your stance on it?

Well, I don't think so. This is not just a matter of my stance, there's a working group resolution on the behavior on w3c/csswg-drafts#3096. You can file an issue to reconsider that resolution of course, but meanwhile there's no reason to have the HTML spec say something that is self-inconsistent, that nobody implements, and that goes against a CSSWG resolution.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

There is still a difference, right? <link> uses browsing-context connected, this does not. See also #4547. Aligning them fully would be ideal I think.

(Also, @rniwa commented on this previously in #1137, which led to #1194 and #1199, but I guess that never got implemented. I suspect at the time the rationale for trying to make this work was that if it worked for standalone documents it should work for standalone shadow roots, but in retrospect that's not that compelling.)

source Outdated Show resolved Hide resolved
@LJ1102
Copy link

LJ1102 commented Jan 18, 2021

Ideally I'd like style elements to parse their contents immediately into a stylesheet when the tag is parsed or its contents are set (svg elements do that). The load event would be emitted when @imports have been loaded (immediately when there's no imports) followed by adding it to the styleSheets list of it's root (either shadow or document). I don't understand how or why that list would ever shrink in that process (?), it'd only shrink when the style element is disconnected. I assume you already went through all of this and I realize that there's probably some catches to this that I don't see, so thanks for indulging me.

Can you show some code so I can take a look at what you're doing? Browsers try very hard to share <style> elements across Shadow DOM, and accessing the stylesheets programmatically will destroy that sharing, fwiw. So if your <style> is large enough your approach will cause a lot of memory overhead.

I was using additional style elements just for dynamic styles so I could safely index into the rules without having to worry about the indices changing when modifying the static styles. You can view some source in this gist of mine. I figured that browsers would internally deduplicate shadow styles, constructable stylesheets seem to attempt to make that an explicit concern to the developer but I feel like internal deduplication will not go anywhere since it's just too comfortable (and widely promoted) to have markup and styling be authored the way it is now (one big block setting the innerHTML of the shadow root). Under that assumption I feel like the only "primary" use-case left for constructable stylesheets is mine which is just an edgecase for it.

@domenic
Copy link
Member

domenic commented Jan 22, 2021

@emilio @annevk should we merge this, or is the remaining mismatch of connected vs. browsing-context connected worth changing something for?

Also, I see http://wpt.live/shadow-dom/ShadowRoot-interface.html failing in Firefox; is that expected? Maybe we should add tests for connected-but-not-BC-connected, as well?

@emilio
Copy link
Contributor Author

emilio commented Jan 22, 2021

@domenic it passes on Firefox Nightly, it was an edge case that I added a test for recently, see https://bugzilla.mozilla.org/show_bug.cgi?id=1687126. Firefox stable parses the stylesheet once connected, but wouldn't remove it if you removed the shadow host.

@annevk
Copy link
Member

annevk commented Jan 25, 2021

I'm happy for this to be merged.

No browser passed the test-case that was linked from the comment.

The algorithm was also inconsistent, as it looked at whether
the element was in a shadow tree or in the document tree, but it was
only specified to be re-run if the element becomes connected or
disconnected.

The CSSWG discussed this in
w3c/csswg-drafts#3096 (comment)
and there are WPTs for this.

This also matches closer the definition of <link rel="stylesheet"> which
does use connectedness (though it uses "browsing-context connected",
which is a bit different):
https://html.spec.whatwg.org/#link-type-stylesheet
@domenic
Copy link
Member

domenic commented Jan 25, 2021

Great. I'd love to see some tests for connected but not BC-connected, but we don't need to block on that; I'll file a WPT issue.

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

Successfully merging this pull request may close these issues.

4 participants