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

[css-cascade-5] Cascade Layers need CSSOM support #6576

Closed
anttijk opened this issue Sep 3, 2021 · 24 comments
Closed

[css-cascade-5] Cascade Layers need CSSOM support #6576

anttijk opened this issue Sep 3, 2021 · 24 comments

Comments

@anttijk
Copy link

anttijk commented Sep 3, 2021

CSSLayerRule interface needs to be specified.

@xiaochengh
Copy link
Contributor

This is not entirely trivial. Since we have two types of @layer rules, the empty statement and the block rule, do we want the same interface for both of them, or different interfaces?

I don't have a strong preference. Just putting the options here for discussion.


If we use different interfaces, the following seems pretty straightforward:

[Exposed=Window]
interface CSSLayerStatementRule : CSSRule {
  readonly attribute CSSOMString names;
};

[Exposed=Window]
interface CSSLayerBlockRule : CSSGroupingRule {
  readonly attribute CSSOMString name; // empty string for anonymous layer
};

If we use the same interface, it would probably be like

[Exposed=Window]
interface CSSLayerRule : CSSGroupingRule {
  readonly attribute CSSOMString name;
  readonly attribute boolean isEmptyStatement;

  unsigned long insertRule(CSSOMString rule, optional unsigned long index = 0); // Is this the correct syntax for override?
};

We need to override CSSGroupingRule.insertRule() so that it throws an exception when called on empty statements.

@tabatkins
Copy link
Member

I think we should make them separate objects, since they have quite distinct data models - one vs N names, child rules vs not. They're the same name for authoring convenience, but that doesn't need to translate to them being the same object type.

@emilio
Copy link
Collaborator

emilio commented Sep 3, 2021

I'd prefer not having two interfaces for this, fwiw... Also, for the @layer a, b, c; case we need a sequence of names, not just one name, right?

@tabatkins
Copy link
Member

Yup, that's what I meant by "one vs N names". @xiaochengh's example supports that, it just exposes the N names as a single CSSOMString rather than as a list of them.

@mirisuzanne mirisuzanne added this to To Resolve in Cascade 5 (Layers) Sep 3, 2021
@anttijk
Copy link
Author

anttijk commented Sep 4, 2021

Having two separate interfaces might actually make sense as there is nothing that can be shared.

CSSImportRule also needs an extension.

@emilio
Copy link
Collaborator

emilio commented Sep 4, 2021

Yeah, I guess that's fine / alright, specially given we've given up on making CSSRule.type return something meaningful for these.

@anttijk
Copy link
Author

anttijk commented Sep 4, 2021

On the other hand these interfaces are pretty useless anyway so I'm equally fine with a generic object that exposes nothing but cssText and the tree structure.

@emilio
Copy link
Collaborator

emilio commented Sep 6, 2021

That'd be totally ok for me too.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 8, 2021
The specifics of how this is going to work are still getting spec'd /
discussed in w3c/csswg-drafts#6576, but this
allows DevTools to work fine and the feature to be complete enough for
Nightly experimentation (with the other in-flight patches).

Otherwise devtools crashes when trying to inspect pages that use them.

Differential Revision: https://phabricator.services.mozilla.com/D124656
@emilio
Copy link
Collaborator

emilio commented Sep 8, 2021

FYI. Not ideal, but I needed some OM support to make DevTools etc work in Firefox.

What I did for now is implementing it as:

interface CSSLayerRule : CSSGroupingRule {};

Where cssRules will return null for statement layers. Again, probably not ideal, it was just the easiest to implement.

aosmond pushed a commit to aosmond/gecko that referenced this issue Sep 18, 2021
The specifics of how this is going to work are still getting spec'd /
discussed in w3c/csswg-drafts#6576, but this
allows DevTools to work fine and the feature to be complete enough for
Nightly experimentation (with the other in-flight patches).

Otherwise devtools crashes when trying to inspect pages that use them.

Differential Revision: https://phabricator.services.mozilla.com/D124656
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Sep 28, 2021
https://bugs.webkit.org/show_bug.cgi?id=230882

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

* web-platform-tests/css/css-cascade/parsing/layer-expected.txt: Added.

Source/WebCore:

Add a minimal CSSLayerRule interface. This is yet unspecified (w3c/csswg-drafts#6576)
but the final version likely won't differ much or at all. This also matches Firefox.

This makes parsing and serialization WPT tests work.

* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* css/CSSLayerRule.cpp: Added.
(WebCore::CSSLayerRule::CSSLayerRule):
(WebCore::CSSLayerRule::create):
(WebCore::CSSLayerRule::cssText const):

The only available functionality is getting the cssText.

* css/CSSLayerRule.h: Added.
* css/CSSLayerRule.idl: Added.
* css/CSSRule.h:
* css/StyleRule.cpp:
(WebCore::StyleRuleBase::createCSSOMWrapper const):

Make the wrapper.

* css/StyleRuleType.h:

Update the type constant to match Firefox (this is not specified).

* css/StyleSheetContents.cpp:
(WebCore::StyleSheetContents::wrapperInsertRule):

Remember the return after succesful insert.

LayoutTests:

* TestExpectations:


Canonical link: https://commits.webkit.org/242218@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283170 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@anttijk
Copy link
Author

anttijk commented Sep 28, 2021

interface CSSLayerRule : CSSGroupingRule {};

Did the same thing in WebKit.

@xiaochengh
Copy link
Contributor

Can we prioritize this issue? We need a spec to move forward.

@mirisuzanne
Copy link
Contributor

Added agenda+ to get a resolution on the approach.

@mirisuzanne mirisuzanne added this to Contain/Layers in EUR Sep 29 2021 TPAC Meeting Oct 1, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Cascade Layers need CSSOM support, and agreed to the following:

  • RESOLVED: Prop: We will have 2 separate objects; one for rules w/o a block and one for rules with a block
The full IRC log of that discussion <dael> Topic: Cascade Layers need CSSOM support
<dael> github: https://github.com//issues/6576
<dael> miriam: A few options shown in the first ocmment on the issue. Discussion about 2 objects or 1 single. If 2 one is for empty layer rules and the other for layer rules with something
<dael> miriam: Don't have an opinion, if anyone does love for them to speak up
<dael> astearns: Looks like people in issue are saying whatever
<dael> miriam: Seems no strong preference. A little sense of it's not useful but needed
<dael> astearns: That 2 objects are needed?
<dael> miriam: That some resolution is needed. But not particularly useful either way
<heycam> q+
<dael> fremy: Maybe idea for simlied. @layer a with no rule and @ layer b with no rule and you don't need one or the other. Then it's only one type of rule. I don't write these, but maybe an idea?
<astearns> ack heycam
<astearns> q?
<dael> heycam: With a single interface...perhaps issue with both approachs...a bit weird if insert and remove rule can change the type. Maybe argument for one interface
<fremy> was suggesting maybe considering `@layer a, b, c` syntactic sugar for `@layer a{} @layer b{} @layer c{}` then you only have one type of rule
<dael> heycam: I see suggestion from xiao was throw exception when insert called on rule without block. I guess need inverse as well
<fantasai> note that @layer b {} is not valid before @import, but @layer b; is
<dael> astearns: Have not thought much but slightly more in favor of single interface that could expect both and lets you go back and forth between
<dael> heycam: Reasonable for author to say start with a layer that has no block and then want to, in cssom, add children?
<fremy> @fantasai: ah, ok, then that doesn't work. except if we change it to allow @layer {} before import but not allow it to have rules in it?
<dael> heycam: If we want to allow that I guess the single interface makes more sense. Else no way to preserve object identity. Perhaps that's not reasonable
<astearns> ack fantasai
<TabAtkins> q+ to talk about syntax mismatch
<dael> fantasai: The @layer rule with a block that accepts style ruels is not allowed before @import but is allowed after
<dael> fantasai: There's several different syntax restrictions
<dael> fantasai: I do think heycam suggestion makes sense. Likely authors will want to add and remove rules from @layer. If converting from being a block ro not I'm not sure, but adding to an empty block I can imagine
<dael> fantasai: There's complication there about being before or after @import
<dael> heycam: Symantically important to allow @layer block before @import?
<dael> fantasai: Yes b/c layered in order of apperance. If importing rules that define appearance it matters. Layers with same name combine.
<dael> fantasai: If you want rules before the @import you can add the layer and have it appear before @import
<astearns> ack TabAtkins
<Zakim> TabAtkins, you wanted to talk about syntax mismatch
<dael> TabAtkins: fantasai covered some of this, but more syntactic mismatch. @layer that doesn't have block allows comma sep. list but with block is only 1 name.
<dael> TabAtkins: Closely related but syntax is difference. If allowing to modify they need to be 2 objects
<dael> astearns: For blockless do we need object to represent that or multiple blockless each with one name
<dael> TabAtkins: Don't have precedent for single rule expanding to multiple. Not impossible, but prefer avoid
<dael> astearns: Sounds to me like need 2 objects, one for blockless and one for block
<dael> astearns: Other comments on just that? Then figure out mutability and errors. Single or 2 objects. Anyone continuing to argue for single?
<dael> astearns: Prop: We will have 2 separate objects; one for rules w/o a block and one for rules with a block
<dael> astearns: Obj?
<dael> RESOLVED: Prop: We will have 2 separate objects; one for rules w/o a block and one for rules with a block
<dael> astearns: Object with a block we should be able to add. For blockless should we add an error or should it not have methods?
<dael> TabAtkins: Won't have methods. No child or insert rule
<dael> astearns: miriam anything else on this? Or should we go with resolution and see what we need on future calls
<dael> miriam: Nothing else

@mirisuzanne mirisuzanne moved this from To Resolve to To Draft in Cascade 5 (Layers) Oct 6, 2021
bertogg pushed a commit to Igalia/webkit that referenced this issue Oct 9, 2021
https://bugs.webkit.org/show_bug.cgi?id=230882

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

* web-platform-tests/css/css-cascade/parsing/layer-expected.txt: Added.

Source/WebCore:

Add a minimal CSSLayerRule interface. This is yet unspecified (w3c/csswg-drafts#6576)
but the final version likely won't differ much or at all. This also matches Firefox.

This makes parsing and serialization WPT tests work.

* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* css/CSSLayerRule.cpp: Added.
(WebCore::CSSLayerRule::CSSLayerRule):
(WebCore::CSSLayerRule::create):
(WebCore::CSSLayerRule::cssText const):

The only available functionality is getting the cssText.

* css/CSSLayerRule.h: Added.
* css/CSSLayerRule.idl: Added.
* css/CSSRule.h:
* css/StyleRule.cpp:
(WebCore::StyleRuleBase::createCSSOMWrapper const):

Make the wrapper.

* css/StyleRuleType.h:

Update the type constant to match Firefox (this is not specified).

* css/StyleSheetContents.cpp:
(WebCore::StyleSheetContents::wrapperInsertRule):

Remember the return after succesful insert.

LayoutTests:

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283170 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@fantasai
Copy link
Collaborator

It seems pretty clear that we'd have (as @xiaochengh outlined above):

[Exposed=Window]
interface CSSLayerBlockRule : CSSGroupingRule {
  attribute somethingsomethingforthenames;
};

[Exposed=Window]
interface CSSLayerStatementRule : CSSRule {
  attribute somethingsomethingforthenames;
};

But the open question is, what are the somethingsomethingforthenames?

Tab suggests to have .name or .nameText or something on the group rule be a simple string (e.g. "name.subname"), which is pretty straightforward, but then for the statement rule, do we want a single string ("name1, name.subname, name3") or a list of strings ("name1", "name.subname", "name3")?

It feels pretty weird if we have the attribute on these rules both be single strings while the group rule string has commas in it and the other one doesn't. Miriam also thinks it feels more natural to have an array here. We'd prefer something like string .name for the grouping rule, and an array of strings .nameList for the statement rule. (But neither of us knows much about CSSOM interfaces.)

One point that @tabatkins brought up was that various comma-separated things in the CSSOM (e.g. selectors, conditionText) are represented as a single list. But a) in these cases the comma represents an "or" operator, so the string is not so much representing a list of things as a single combined condition and b) none of these are trying to coordinate with a nearly-identical rule whose corresponding attribute represents a single item of the type represented in that comma-separated list.

@xiaochengh
Copy link
Contributor

We'd prefer something like string .name for the grouping rule, and an array of strings .nameList for the statement rule.

I think this is better than what I previously proposed. I wasn't aware of the array type in IDL.

@anttijk
Copy link
Author

anttijk commented Oct 15, 2021

We'd prefer something like string .name for the grouping rule, and an array of strings .nameList for the statement rule.

I like these names and return types. They also match the names WebKit uses internally already.

@emilio
Copy link
Collaborator

emilio commented Oct 15, 2021

Sounds good. Should the attributes be readOnly as per #6645? It's not a massive deal to support mutable attributes, but it's almost always a footgun.

@anttijk
Copy link
Author

anttijk commented Oct 15, 2021

Yeah, should be readonly.

No new mutable CSSOM should be added ever, really. It is a useless footgun.

@tabatkins
Copy link
Member

Ah, if we're doing readonly, then my objections to arrays mostly go away. Having mutable arrays requires a lot more logic that isn't really justified here. So we can just do a readonly FrozenArray<CSSOMString> for the type on the CSSLayerStatementRule and be good.

I still suspect having the layer names themselves be a single string with the possibly-dot-separated name in it is best here; most of the time you want to treat the name as a unit anyway, and when you do want to break it down, a simple name.split(".") is reliably correct unless someone put an escaped period in their layer name like foo\.bar (which, uh, stop hitting yourself).

@astearns astearns moved this from Temp to Cascade/Contain in APAC Nov 3 2021 TPAC meeting Oct 29, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-cascade-5] Cascade Layers need CSSOM support, and agreed to the following:

  • RESOLVED: add the cssom rules as .name and .namelist
The full IRC log of that discussion <dael_> Topic: [css-cascade-5] Cascade Layers need CSSOM support
<dael_> github: https://github.com//issues/6576
<fantasai> https://github.com//issues/6576#issuecomment-943858864
<dael_> miriam: My understanding is we wanted 2 interfaces, one for statement and one for block rules
<dael_> miriam: block rule has read only attribute of the name of the layer. statement would have read only frozen array of strings
<dael_> miriam: Does that cover it?
<Rossen_> q
<dael_> fantasai: Question was what to call them. .name or .nametext vs .namelist
<dael_> TabAtkins: .names
<dael_> fantasai: .name and .names seems tricky
<dael_> TabAtkins: yeah
<dael_> fantasai: .namelist regardless of the other being .name or .nametext
<dael_> Rossen_: .name is shorter
<dael_> fantasai: great. resolve on that
<dael_> Rossen_: obj or suggestions?
<dael_> RESOLVED: add the cssom rules as .name and .namelist

@mirisuzanne
Copy link
Contributor

@anttijk @xiaochengh @emilio let us know if the edits in 6e8482b satisfy the question here.

@foolip
Copy link
Member

foolip commented Nov 22, 2021

For ease of review, that's https://drafts.csswg.org/css-cascade-5/#layer-apis.

@xiaochengh
Copy link
Contributor

b7c7a15 lgtm

@xiaochengh
Copy link
Contributor

Wait, there's one last thing missing: We need to be able to distinguish whether a CSSImportRule is unlayered or in an anonymous layer.

How about making layerName nullable (readonly attribute CSSOMString? layerName;), so that it returns null when the rule is not layered?

tabatkins added a commit that referenced this issue Dec 1, 2021
Co-authored-by: Tab Atkins Jr <jackalmage@gmail.com>
@fantasai fantasai added Closed Accepted by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Commenter Response Pending labels Dec 3, 2021
@fantasai fantasai closed this as completed Dec 3, 2021
@mirisuzanne mirisuzanne moved this from To Review/Publish to Complete in Cascade 5 (Layers) Aug 4, 2022
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 30, 2023
The specifics of how this is going to work are still getting spec'd /
discussed in w3c/csswg-drafts#6576, but this
allows DevTools to work fine and the feature to be complete enough for
Nightly experimentation (with the other in-flight patches).

Otherwise devtools crashes when trying to inspect pages that use them.

Differential Revision: https://phabricator.services.mozilla.com/D124656
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 30, 2023
The specifics of how this is going to work are still getting spec'd /
discussed in w3c/csswg-drafts#6576, but this
allows DevTools to work fine and the feature to be complete enough for
Nightly experimentation (with the other in-flight patches).

Otherwise devtools crashes when trying to inspect pages that use them.

Differential Revision: https://phabricator.services.mozilla.com/D124656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

9 participants