-
Notifications
You must be signed in to change notification settings - Fork 719
[css-cascade-5] Cascade Layers need CSSOM support #6576
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
Comments
This is not entirely trivial. Since we have two types of 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. |
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. |
I'd prefer not having two interfaces for this, fwiw... Also, for the |
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. |
Having two separate interfaces might actually make sense as there is nothing that can be shared. CSSImportRule also needs an extension. |
Yeah, I guess that's fine / alright, specially given we've given up on making |
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. |
That'd be totally ok for me too. |
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
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:
Where |
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
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
Did the same thing in WebKit. |
Can we prioritize this issue? We need a spec to move forward. |
Added agenda+ to get a resolution on the approach. |
The CSS Working Group just discussed
The full IRC log of that discussion |
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
It seems pretty clear that we'd have (as @xiaochengh outlined above):
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. |
I think this is better than what I previously proposed. I wasn't aware of the array type in IDL. |
I like these names and return types. They also match the names WebKit uses internally already. |
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. |
Yeah, should be readonly. No new mutable CSSOM should be added ever, really. It is a useless footgun. |
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 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 |
The CSS Working Group just discussed
The full IRC log of that discussion |
@anttijk @xiaochengh @emilio let us know if the edits in 6e8482b satisfy the question here. |
For ease of review, that's https://drafts.csswg.org/css-cascade-5/#layer-apis. |
b7c7a15 lgtm |
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 |
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
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
CSSLayerRule interface needs to be specified.
The text was updated successfully, but these errors were encountered: