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

Don't cascade styles to nested components? #583

Closed
ethanresnick opened this issue May 14, 2017 · 11 comments
Closed

Don't cascade styles to nested components? #583

ethanresnick opened this issue May 14, 2017 · 11 comments

Comments

@ethanresnick
Copy link

Currently, as the Svelte docs state, "scoped" styles will still cascade to nested components. Here's an example in the repl where the parent component's styles effect the paragraph styles of the nested component: https://svelte.technology/repl?version=1.20.2&gist=473c9964efa1d3b03474b14c115dc70b

Is this desirable? It arguably has some uses, but it seems to negate half the value of the scoping—i.e., styles can't leak out of a component, but styles from parent components can leak in, which seems just as problematic.

I'll note that other libraries that isolate styles (some even using similar, attribute-selector based approaches) don't allow styles to effect nested components by default. E.g., https://www.npmjs.com/package/styled-jsx, which also has an escape hatch (in the form of .rootClassName :global(...)) that can be used to recreate this cascading effect.

@leeoniya
Copy link

i'll just chime in for no reason :p

after lengthy consideration of adding css-in-js scoping to domvm, this "leakage into subcomponents" situation was the final nail in the coffin [1]. the most problematic part of CSS is the "C", which scoping-via-prefixing doesnt quite solve.

[1] domvm/domvm#108 (comment)

@constgen
Copy link

I use parent > child selector combinator to apply styles only to internal component elements but not to the same matches in the children. It may be not always suitable when you use a composition of other components inside your component. But it helped so far.

@Rich-Harris
Copy link
Member

I've found it to be beneficial more often than not. But there is one major downside — it's impossible to analyse the CSS:

<div>
  <p>some text</p>
  <Widget/>
</div>

<style>
  div {
    font-size: 2em;
  }

  p {
    color: red;
  }

  strong {
    font-family: 'Comic Sans MS';
  }
</style>

That CSS will be transformed like this:

div[svelte-3281250378], [svelte-3281250378] div {
  font-size: 2em;
}

p[svelte-3281250378], [svelte-3281250378] p {
  color: red;
}

strong[svelte-3281250378], [svelte-3281250378] strong {
  font-family: 'Comic Sans MS';
}

If it weren't for the component — or if we decided that styles should not leak into children — then that CSS could become this:

div[svelte-3281250378] {
  font-size: 2em;
}

/* note that we'd have to put the scoping attribute on the <p> */
p[svelte-3281250378] {
  color: red;
}

(If the compiler had information about <Widget/> then we could perform app-wide CSS analysis, but that's not currently possible.)

So at the very least, I think we should have the option to keep styles confined to the component, and I could be persuaded that that should be the default (a breaking change, so would happen in v2).

And yeah, we could always invent some styled-jsx-esque syntax to control it instead.

@constgen
Copy link

Some linters say that a selector by attribute without value is slow from performance perspective. So probably to make the isolation optional.

@ethanresnick
Copy link
Author

ethanresnick commented May 14, 2017

@constgen

I use parent > child selector combinator to apply styles only to internal component elements but not to the same matches in the children.

Yeah, but, as you point out, that doesn't work in all cases (e.g., in my example it wouldn't work because the nested component also happens to render a <p> as it's top level element). Also, it could get quite annoying to have to do this everywhere, I'd think.

Some linters say that a selector by attribute without value is slow from performance perspective.

Yeah, I think attribute selector performance could be a real issue. I haven't looked into it closely, but I remember seeing this.

Maybe the right solution is to add a custom class to every element/selector, rather than the custom attribute (with the exception of not adding the class to nested components), i.e.:

<div>
  <p class="test">some text</p>
  <Widget/>
</div>

<style>
  div {
    font-size: 2em;
  }

  .test {
    color: red;
  }
</style>

becomes:

<div class="svelte-3281250378">
  <p class="test svelte-3281250378">some text</p>
  <Widget/>
</div>

<style>
  div.svelte-3281250378 {
    font-size: 2em;
  }

  .test.svelte-3281250378 {
    color: red;
  }
</style>

@Rich-Harris

I've found it to be beneficial more often than not.

Can you post some examples of this? I'm just genuinely curious, because the fashionable opinion seems to emphasize isolation above all (and, with an escape, that makes sense to me)...but you know how fashion's are.

it's impossible to analyse the CSS:

This confuses me a bit. What type of analysis would you want to do that the current approach makes impossible?

@Rich-Harris
Copy link
Member

Some linters say that a selector by attribute without value is slow from performance perspective.

Yeah, I think attribute selector performance could be a real issue. I haven't looked into it closely, but I remember seeing this.

This came up recently in #570 — we didn't reach a particular conclusion, but we certainly could use classes instead of attribute selectors if necessary.

Can you post some examples of this?

Basically anywhere you'd use cascading, period. For example, I might have several different components that all have buttons — I don't want to add button styles for each component that has a button, nor do I want to create a special <Button> component, since that feels like unnecessary over-engineering.

In the Svelte REPL (the code for which probably isn't a great advert for Svelte! 😬), the button styles are defined once here. I can easily override those styles in child components — for example, the 'delete' button in the component selector needs a fixed width because its text changes to 'sure?' when you click it. As it happens the width is in an inline style attribute but it could easily be in CSS as well, because that's how CSS is designed to work. I sometimes think I'm the only person who actually likes that!

Of course, you could argue that the button styles should be in some global document-level CSS instead of in a component. But you might not have access to document level CSS, depending on the kind of app you're building (in my day job, I don't), and in any case it prevents a potentially useful optimisation — code-splitting your CSS the same way people code-split their JS (though there isn't yet a good way to do that).

This confuses me a bit. What type of analysis would you want to do that the current approach makes impossible?

In the example above, it's not possible to know whether there are non-top-level <p> tags that the color: red style should apply to, which means we can't know whether it's safe to rewrite the selector to p[svelte-3281250378], [svelte-3281250378] p instead of just p[svelte-3281250378].

@paulocoghi
Copy link
Contributor

paulocoghi commented May 29, 2017

I sometimes think I'm the only person who actually likes that!

You are not the only one. :) For me, cascading style is much more beneficial than the opposite.

@ethanresnick
Copy link
Author

I sometimes think I'm the only person who actually likes that!

Personally, I would find it too risky to have a selector like button apply to child components, as it seems quite possible a child might later add a <button> element for something that shouldn't look like a button (but should be focusable/tappable), and I wouldn't want it to accidentally pick up those styles. I can understand this "apply to children" behavior being nice when the selector is less likely to be matched inadvertently, though (e.g. .svelte-button). Still, I think I'm ultimately in the camp that would create a custom button component.

Without debating whether that's overengineering, though, I guess I'm just saying that I'd be 👍 on not applying styles to children by default, with some syntax to override that on a per-selector basis. That would enable the static analysis for the css optimization you talked about, and would make (what I'd consider to be) very unsafe selectors, like p or h1, not apply to children, while still enabling inheritance where its useful.

@Rich-Harris
Copy link
Member

Been doing some 'competitor analysis'. css-modules and styled-jsx have quite a nice approach to this — wrapping a selector in :global(...) prevents the selector from being transformed.

So to take the example above, you could do this...

:global(button) {
  /* button styles here */
}

/* or this */
:global(.svelte-button) {
  /* button styles here */
}

...and those styles can be picked up by child components. If you wanted to ensure they are only applied to the current component and its children (not parents), then you could use a selector like this:

div :global(button) {
  /* button styles here */
}

/* or this */
div :global(.svelte-button) {
  /* button styles here */
}

(That assumes there's a <div> somewhere in the current component. Could also use * I suppose, or maybe we could introduce :host, though I'm wary of making this look too much like Shadow DOM since it doesn't behave exactly the same viz parent styles crossing the boundary.)

I think that probably has the best of all possible worlds — it allows for better analysis and more conservative selector transformation, defaults to 'fewer surprises' while still being easy for people like me and @paulocoghi to use cascading, and makes things nice and explicit using syntax that is familiar to users of similar tools. Even better, :global(...) is already valid CSS, so there's no difficulty getting it to parse.

Does anyone object to us going down that road?

Annoyingly this would be a breaking change, of course — so I suppose we would have to make this behaviour opt-in until v2:

svelte.compile(source, {
  cascade: false // defaults to `true` in v1, has no effect in v2
});

@Rich-Harris
Copy link
Member

PR for this --> #607

Rich-Harris added a commit that referenced this issue Jun 1, 2017
add cascade option, to prevent components inheriting styles
@Rich-Harris
Copy link
Member

Released this as 1.22 — cascade: false to opt-in to the v2 behaviour now

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

No branches or pull requests

5 participants