Please clearly show in the README the limitation / bugs it can create #4

Closed
MoOx opened this Issue May 5, 2015 · 9 comments

Projects

None yet

4 participants

@MoOx
MoOx commented May 5, 2015

Currently you use a title like

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

With the current html, this is completlly wrong

<div class="component">
  Black
  <div class="header">
    Blue
    <div class="text">
      Green
      <div class="header">Green</div>
    </div>
  </div>
</div>

See native result on firefox http://jsbin.com/tireneluxo/1/edit?html,css,output

Your output give a completly different result

http://jsbin.com/zacakilamo/1/edit?html,css,output

It's unexpected according to what I can read in the readme https://github.com/MadLittleMods/postcss-css-variables#using-cascading-variables-the-var-notation

Using Cascading Variables: the var() notation
...

  • No limitation on what scope CSS variables can be declared or used (:root or wherever)
  • Proper value substition based on explicit DOM/structure traversal

So it's totally unclear.

@tunnckoCore

I dont see problem. http://jsbin.com/wewunusubu/2

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

this would be compiled to

.component .header {
  color: blue;
}

.component .text {
  color: green;
}

so <div class="header">Green</div> would be blue because the defined color in .component .header
the problem here is other... this overriding. .component --text-color override .root --text-color

@MoOx
MoOx commented May 5, 2015

the problem here is other... this overriding. .component --text-color override .root --text-color

Yes this is ONE problem.

The output you provide is completely not enough to say "No limitation".

And when I read again this quote, I see a something wrong:

Using Cascading Variables: the var() notation
...

  • No limitation on what scope CSS variables can be declared or used (:root or wherever)
  • Proper value substition based on explicit DOM/structure traversal

There is a bug + limitation with the current implementation.

@MadLittleMods
Owner

Proper value substition based on explicit DOM/structure traversal

This is trying to convey: we look through the explicit structure of the rules and selectors in your CSS and resolve the value from that.

I understand that with real CSS variables, the true cascade structure would resolve the values you are expecting.

To get what you expect with postcss-css-variables, you need explicitly state this structure:

Input:

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);

  /* This #1 */
  .header {
    color: var(--text-color);
  }
}

/* That #2 */
.component .text .header {
  color: var(--text-color);
}

Output:

.component .header {
  color: blue;
}

.component .text {
  color: green;

  /* This #1 */
  .header {
    color: green;
  }
}

/* That #2 */
.component .text .header {
  color: green;
}
@MoOx
MoOx commented May 6, 2015

So I have to repeat some definition with more css ?
Looks like you are not really adding value that much ;)

@MadLittleMods
Owner

@MoOx I think what you are wanting would be in the minority of what people actually want/expect. I realize the CSS custom properties spec would make it your way which does make sense, but definitely not in terms of this plugin because no nesting structure was provided for us to work from.

If I was nesting a component over and over, I would expect it it look the same.


Maybe a better solution to your problem would be to use currentColor for any nested components.

/* ... */

.header .header {
    color: currrentColor;
}
@MoOx
MoOx commented May 6, 2015

I think what you are wanting would be in the minority of what people actually want/expect.

You choose the syntax from w3c spec and you don't want to follow the spec ? Weird.

but definitely not in terms of this plugin because no nesting structure was provided for us to work from

So don't put link to w3c spec everywhere in the readme. You are harming people that are eager to use w3c spec compliant code.

currrentColor

color was just an example. If you put any other properties, this whould work as expected by the specs?

@MadLittleMods
Owner

@MoOx I think the opening part of the readme explains pretty well and reiterates multiple times, that this can not perform exactly as the spec.

The spec is linked for reference so people can check out the syntax or compare to the actual functionality.


Because CSS custom properties are not widely supported. The use-case you bring up is not widely used today and in my opinion, is not a very good way to construct styles. To get the output you expect now, you would have to nest/descendant-selector anyway.

@MoOx MoOx referenced this issue in postcss/postcss Jul 3, 2015
Closed

Rename some plugins to postcss-w3c-* #403

@MoOx MoOx added a commit to postcss/postcss that referenced this issue Jul 9, 2015
@MoOx MoOx Move `postcss-css-variables` out of `Future CSS Syntax` section
Since this plugin does not provide a future proof code (see MadLittleMods/postcss-css-variables#4) and documentation is still misleading and unclear about the differences, I think it does not belong into this section.
8221285
@MoOx MoOx added a commit to postcss/postcss that referenced this issue Jul 9, 2015
@MoOx MoOx Move `postcss-css-variables` out of `Future CSS Syntax` section
Since this plugin does not provide a future proof code (see MadLittleMods/postcss-css-variables#4) and documentation is still misleading and unclear about the differences, I think it does not belong into this section.
660cc12
@MoOx MoOx added a commit to postcss/postcss that referenced this issue Jul 9, 2015
@MoOx MoOx Move `postcss-css-variables` out of `Future CSS Syntax` section
Since this plugin does not provide a future proof code (see MadLittleMods/postcss-css-variables#4) and documentation is still misleading and unclear about the differences, I think it does not belong into this section.

[skip ci]
47b9661
@isiahmeadows
Contributor

@MadLittleMods Mind noting this spec deviation on the readme? Unlike some, I'm not as concerned about intentional deviations from spec (even Babel has a several with ES2015), but I'd find it much more comforting knowing them.

I think this stems from the fact this plugin makes some unsafe assumptions regarding the selectors, like above. As in with this CSS:

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

you'll get this for each one:

//- In Jade, for brevity
.component: Black text
.component: .header: Blue text
.component: .header: .text: Green text
.component: .header: .text: .header: Green text per spec, blue per this polyfill

The reason this is the case is because of selector order, and this plugin substitutes the color when it's not necessarily safe to do so. The result will end up like this:

.root {
  /* --text-color: red; */
}

.component {
  /* --text-color: blue; */
}

.component .header {
  color: /* var(--text-color) */ blue;
}

.component .text {
  /* --text-color: green; */
  color: /* var(--text-color) */ green;
}

/* The above, reduced */
.component .header { color: blue; }
.component .text { color: green; }

The browser knows that .component > .header > .text > .header will trigger the .component .text selector to change the --text-color variable to green and then use that variable's value in .component .header for the color property.

This plugin, on the other hand, does not. It has no ability to know anything about the HTML itself (especially if it's generated through JS), and thus it is prone to making unsafe assumptions unless it's ultra-conservative like postcss-custom-properties.


It would be appreciated if you note this in the README, though.

@MadLittleMods
Owner

Hey @isiahmeadows,

Per your investigation and this issue, the deviation is not knowing the actual DOM structure and instead we work off the explicit structure assumed from your CSS selectors.

Feel free to make a PR to update the readme to make this point more clear 😀

Currently we have this:

We strive for the most complete transformation but we/no plugin can achieve true complete parity according to the specification because of the DOM cascade unknowns.

and this comment which definitely needs to be rewritten as I meant to say something like "assumed DOM structure from your explicit CSS selectors."

Proper value substitution based on explicit DOM/structure traversal

@isiahmeadows isiahmeadows added a commit to isiahmeadows/postcss-css-variables that referenced this issue Jun 16, 2016
@isiahmeadows isiahmeadows Fix, clean up, and clarify README [ci skip]
Based on [this comment](MadLittleMods#4 (comment)). I've also added + fixed several other things, and I cleaned up the organization and presentation to be a little more cohesive.
6e8ca27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment