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] Proposal to disallow interleaving of @layer and @import rules #6522

Closed
xiaochengh opened this issue Aug 17, 2021 · 13 comments
Closed

Comments

@xiaochengh
Copy link
Contributor

Regarding empty @layer statements, the current spec says:

Such empty @layer rules are allowed anywhere either @import or other @layer rules are allowed.

And also:

Any @import rules must precede all other valid at-rules and style rules in a style sheet (ignoring @charset and empty @layer definitions), or else the @import rule is invalid.

This allows interleaving @layer and @import rules. I'm proposing to disallow it because:

  • There doesn't seem to be a valid use case to interleave these two types of rules
  • No interleaving makes it much easier to implement in Chromium. Otherwise, we need to maintain the relative ordering between @layer and @import rules in the StyleSheetContents class, which makes it more complicated without getting any benefit

The exact proposal is that rules in a style sheet must follow this ordering:

  1. @charset
  2. Empty @layer statements
  3. @import
  4. @namespace
  5. All other rules + empty @layer statements
@mirisuzanne
Copy link
Contributor

I don't think this would cause any functional problems - authors can still order layers as they want, and then add imported styles to those layers. But it does make the syntax for that a bit less flexible.

@tabatkins
Copy link
Member

Yeah, no functionality is lost here, just a bit of flexibility. But I'd be okay with the restriction; it's probably better practice to put your @layer ...; rule either before or after your imports anyway.

@mirisuzanne mirisuzanne moved this from To Consider to To Resolve in Cascade 5 (Layers) Aug 23, 2021
@mirisuzanne
Copy link
Contributor

Agenda+ to get a resolution

@mirisuzanne mirisuzanne moved this from To Resolve to To Draft in Cascade 5 (Layers) Aug 25, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed disallow interweaving of @import and @layer, and agreed to the following:

  • RESOLVED: @layer statements must occur before or after all @imports, not interleaved
The full IRC log of that discussion <TabAtkins> Topic: disallow interweaving of @import and @layer
<TabAtkins> github: https://github.com//issues/6522
<TabAtkins> miriam: xiaochang is working on a cascade layers impl, and was concerned about allowing `@layers ...;`rules allowed anywhere in the doc, including between @imports
<TabAtkins> miriam: He's suggesting instead we just allow them before or after all the imports, but not between
<TabAtkins> miriam: I think that's fine; there's no functionality loss, just a little bit of flexibility. I'm fine with this.
<fantasai> wfm
<chrishtr> +1
<TabAtkins> Rossen_: Seeing support, any other opinions? Objections?
<emilio> q+
<chris> ping
<Rossen_> ack emilio
<TabAtkins> emilio: I'd argue we shoudl only allow after
<TabAtkins> emilio: The less syntax we have to mess with preload scanners the better
<TabAtkins> miriam: That actually does reduce functionality, we need to be able to order layers before we import them
<TabAtkins> emilio: That means the scanners need to deal with this
<TabAtkins> emilio: Do we also allow the block syntax?
<TabAtkins> fantasai: No, we disallow that intentionally
<TabAtkins> emilio: Okay, as long as there's no arbitrary blocks before the imports it's okay
<Rossen_> q?
<TabAtkins> RESOLVED: @layer statements must occur before or after all @imports, not interleaved

@fantasai
Copy link
Collaborator

@mirisuzanne Your edits also disallow interleaving among @layer and @namespace, is there a reason for that?

@fantasai fantasai reopened this Aug 28, 2021
@mirisuzanne
Copy link
Contributor

@fantasai That was a request from @xiaochengh, as I understand it had to do with the same parsing/performance concerns mentioned above. Maybe he can speak to it more here.

@xiaochengh
Copy link
Contributor Author

Yes, I requested that the rules should follow the exact ordering I initially proposed.

The reason is that there's no loss of functionality, and the implementation in Chromium gets much simpler.

@bramus
Copy link
Contributor

bramus commented Aug 30, 2021

I see that the spec got adjusted in ed81c17 for this, but I'm a bit confused by the use of the word "They" in the note.

Note: They are not allowed between (or intermixed with) @import and @namespace rules …

That "They", does it mean:

  1. @layer statement at-rules (from the title of that section)?
  2. @layer block at-rules (mentioned as the last few words just above the note)?
  3. Both

A few thoughts that go with all these options:

  • If option 1: then the first syntax in EXAMPLE 31 (listed just underneath it) would be invalid, no?

  • If option 2: it's pretty unclear to me to know that "They" targets only this type of @layer at-rules. It also feels a bit redundant, as this guideline is also noted in another note above it (“Note: @layer block at-rules cannot be interleaved with @import rules.”).

  • If option 3: How would one append extra local CSS to the theme layer of EXAMPLE 31? I guess that would only be possible by creating extra sub-layers inside theme?

    @layer default, theme, theme.overrides, components;
    @layer theme.overrides {
      /* Override theme "base" layer here */
    }
    
    @import url(theme.css) layer(theme);

    Feels weird to first have to write the overrides, and only then load its base layer.


I think option 2 is meant, but I'm not really sure reading all comments and logs here, as it's not explicitly mentioned 🤔

@mirisuzanne
Copy link
Contributor

@bramus option 1 is the intended behavior, though it's only intended to mean that any @import and @namespace rules can't have intervening @layer statements. The example shows the opposite -- one @import between @layer statements -- which is allowed. I can try to add more clarity there…

@bramus
Copy link
Contributor

bramus commented Aug 31, 2021

Thanks for the clarifications on that, @mirisuzanne. Think I get it now:

/* EXAMPLE 31 */
@layer default;
@import url(theme.css) layer(theme);
@layer components; /* 👈 This @layer statement here will make all subsequent @import rules be ignored. In this example however there are none */

@layer default {
  audio[controls] {
    display: block;
  }
}

I see that in d9fdc1e you changed the text to “A ''@layer'' rule that comes after …”. More precise would be “A ''@layer'' statement at-rule that comes after …”?


Winging back to “the @layer block at-rule” described just above it. There it reads in a note:

Note: @layer block at-rules cannot be interleaved with @import rules.

Is that 1 @import rule in between two @layers block at-rules? If so, I somewhat read between the lines that best practice that's being hinted at is to:

  1. Establish a layer order upfront using @layer statement at-rules
  2. Group your @imports after that
  3. Append styles to already established layers using @layer block at-rules

Would that be something to recommend people to do?


(Apologies if I'm asking too many stupid questions regarding this. Trying to understand things correctly here. Perhaps some code-examples of what is meant by all those limitations could help? In css-nesting-1 for example I see a lot of these “wrong examples“ with an explanation of what exactly is wrong added to them.)

@mirisuzanne
Copy link
Contributor

@bramus Yes, your summary of best practice is correct. Though another common practice is to avoid using @import entirely for performance reasons. :)

Only the empty layer-ordering statements are allowed before @import/@namespace, but once we encounter an @import/@namespace rule, then either type of @layer rule will make additional @import/@namespace rules invalid. So I think the current text is correct. Basically, you can't put anything else between @import/@namespace rules, those have to be applied in a single block without intervening rules.

@bramus
Copy link
Contributor

bramus commented Sep 1, 2021

Thanks again for the extra input there. Current spec text indeed is correct; wanted to verify if I laid the pieces together correctly :)

@mirisuzanne
Copy link
Contributor

@bramus clarified in the example. See 7726486

@mirisuzanne mirisuzanne moved this from To Review/Publish to Complete in Cascade 5 (Layers) Sep 1, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 3, 2021
Disabled, and of course doing nothing for now still, but this is another
piece that is useful to get reviewed separately.

Don't allow layers to be interleaved with @import / @namespace rules as
per w3c/csswg-drafts#6522.

Differential Revision: https://phabricator.services.mozilla.com/D124229
spinda pushed a commit to PLSysSec/cachet-firefox that referenced this issue Sep 8, 2021
Disabled, and of course doing nothing for now still, but this is another
piece that is useful to get reviewed separately.

Don't allow layers to be interleaved with @import / @namespace rules as
per w3c/csswg-drafts#6522.

Differential Revision: https://phabricator.services.mozilla.com/D124229
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 30, 2023
Disabled, and of course doing nothing for now still, but this is another
piece that is useful to get reviewed separately.

Don't allow layers to be interleaved with @import / @namespace rules as
per w3c/csswg-drafts#6522.

Differential Revision: https://phabricator.services.mozilla.com/D124229
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

6 participants