Improve color token operations

[mgreter]

This addresses a general issue how color tokens are handled.
I guess this is related to #558 and #629
This fixes the spec test spec/libsass-todo-tests/libsass/colors!

This pretty sure addresses a bug, since unquote("red") and unquote(red) would not be distinguishable, without propagating if the original variable was quoted or not. The first is a simple string, while the latter can be interpreted as a color (which matters for operations, like addition).

input:

color: red + green;
color: red + hux;
color: unquote("red") + green;
color: unquote(red) + green;

current:

color: #ff8000;
color: #ff0000hux;
color: red#008000;
color: red#008000;

after (as expected)

color: #ff8000;
color: redhux;
color: redgreen;
color: #ff8000;

Unfortunately this breaks spec/basic/16_hex_arithmetic, but it somehow gets it half right and half wrong. It's still closer to the latest ruby sass version, but falls behind at one certain color shortening. Overall this looks pretty promising to get this fixed once and for all.

I don't know what the implications are by "abusing" needs_unquoting and by removing the !lhs->is_delayed() check! Therefore I leave this open as WIP for further review and comments!

[xzyfer]

I don't know what the implications are by "abusing" needs_unquoting and by removing the !lhs->is_delayed() check!

I ran recently ran into this aswell - #695

@akhleung can you please give us some background on the intent and affect of is_delayed on AST nodes? Best I can tell it's used to short-circuit some evaluations. Is it purely a performance measure or are the edge cases to be aware of?

[mgreter]

I also don't understand what has_non_hoistable really means! Some hints in this direction would be appreciated (beside the questions by @xzyfer )! //CC @akhleung

[akhleung]

@mgreter Let me see ... the is_delayed flag was, at the time, the only way I could think of to handle those cases where Sass sometimes evaluates the RHS of a style declaration, and sometimes not (specifically declarations with slashes, which are sometimes quotients, and sometimes just separators). However, I believe that this may be fixed by the recent tokenizing update that scans for static rules -- this might be the way that Ruby Sass decides whether or not to evaluate the RHS of a style (i.e., always evaluate, unless the declaration was matched by the "static declaration" regexp).

As for the hoistable flags, I'll need to review the code a bit -- it has to do with whether blocks can be flattened or not, which depends on whether the elements within are "hoisted out" during final rendering, or remain nested. I can't recall all the details at the moment, so I'll have to get back to you on this one.

[akhleung]

Also tagging @xzyfer

[xzyfer]

Thanks for clarifying is_delayed @akhleung. Looking at the code that was pretty much what I had expected. As you mentioned my recent tokenizer updates should make it safe to remove delayed evaluations as we come across them.

[mgreter]

I cleaned up the commit and removed all references to is_delayed. Had to add normalize_color to exactly match the output of sass 3.4.9. @xzyfer do you think this is clean enough to be merged?

[xzyfer]

Does this still break spec/basic/16_hex_arithmetic ? Generally speak I'd rather avoid introducing regressions especially since we're gearing up for new release.

[xzyfer]

The PR itself looks good to me though! Nice work

[mgreter]

Indeed spec/basic/16_hex_arithmetic needs to be updated. I regenerated it with sass 3.4.9 and had to add the normalize_color call to match the expected output perfectly. The corressponding changes to sass-spec are here: mgreter/sass-spec@5f07051

[xzyfer]

Ah so it's the spec that's incorrect, not the implementation? If that's the case 👍 🚢

[mgreter]

Yep, there seem to be two edge cases that have changed in sass. One is the shortining of the color and the other is the "weird issue with unquote(0xf00)". Both issues are also related to 3.4 support blockers! and should be fixed by this.

[xzyfer]

Sounds good! Ship it!

FTR: the first edge case is due to changes in 3.4.0

When using colors in SassScript, the original representation of the color will be preserved wherever possible. If you write #f00, it will be rendered as #f00, not as red or #ff0000. In compressed mode, Sass will continue to choose the most compact possible representation for colors.

[mgreter]

Unfortunately your quote showed me another regression, and not sure how easy this is to fix. To do it properly I probably have to add and pass a few more tests that currently are not even in place. Souldn't be extremly hard, but this will take at least a few days till I can come up with a solution for this.
Or do you think we should still ship it in the current state? It is still better than before but not on par with the current sass implementation!?

[xzyfer]

Can you be more specific about the regression? Does it break existing
behaviour? If so I'd say we wait for shipping this.

It's important we show stability so people aren't afraid to depend on us.
I'll almost always err on the side of stability over new features.
On 15 Dec 2014 20:22, "Marcel Greter" [email protected] wrote:

Unfortunately your quote showed me another regression, and not sure how
easy this is to fix. To do it properly I probably have to add and pass a
few more tests that currently are not even in place. Souldn't be extremly
hard, but this will take at least a few days till I can come up with a
solution for this.
Or do you think we should still ship it in the current state? It is still
better than before but not on par with the current sass implementation!?

—
Reply to this email directly or view it on GitHub
#718 (comment).

[mgreter]

What is your definition of "break" :) The output format of colors is not preserved as per your previous comment. I guess I should revert the normalize_color, since it does not really do the right thing. Looks like we have to store the original format to check how to output it. But it looks like we currently do not really have any unit tests for that and the existing "test" uses the short form (and there is no test if it preserves the long form). So, no, IMO it does not break anything, since both formats are still valid css.

Only problem I see is that we have two separate things going in spec/basic/16_hex_arithmetic. One is fixed by this PR, but the other one does not yet match current sass behavior and I would need to adjust the test manually. But I probably have to take another look to be 100% sure why I had to adjust that test in the first place!

[xzyfer]

If it's purely a matter of output formatting I'm fine with shipping with. AFAIK we never preserved output formatting and Sass only just introduced it as a 3.4 feature.

[xzyfer]

We should make another issue to track the "preserve color formatting" feature 👍

[mgreter]

This was pretty much a piece of cake once it was obvious I had to preserve the input format.
I also tested a few more things with sass and found it a bit strange that it does this:

t01: #abc; // => #abc
t02: #aabbcc; // => #aabbcc
t03: rgb(170, 187, 204); // => #aabbcc
t04: rgba(170, 187, 204, 1); // => #aabbcc

I thought I would also need to look into these, but they seem to convert as before. So this made it a lot easier and it looks like this is now actually on par with current sass implementation. Yeah! ⛵

[xzyfer]

Nice I'm surprised Ruby sass doesn't preserve rgba formats.

Looking through the Ruby sass issues tracker I found this explanation.

In general, we try to preserve the user's input format for colors, but rgb(), rgba(), hsl(), and hsla() are kind of special cases. The Sass functions support a superset of the input their CSS counterparts do, and in some cases allow users to use new CSS syntax and have it compiled to a form that older browsers will understand. Although there are probably some cases where preserving the input format of these functions is safe, in general it would be risky and inconsistent, so we just don't do it at all.

[xzyfer]

👍 🚢 🚀

[mgreter]

Ok, so travis-ci showed a regression with the second test this PR adresses in spec/basic/16_hex_arithmetic (the weird one involving 0xf00). I restarted the compile when I merged the sass-spec PR. I was able to track it down, but could not come up with a really clean solution yet. I think the last commit should at least get travis-ci going again (and actually fix that issue). But it's really not clean.

https://travis-ci.org/sass/libsass/jobs/44259916#L336

The issue was that 0xf00 is actually interpreted as a number with a dimension. This is equal to ruby sass (I checked). Unfortunately this whole string (and not only the number) is passed to atof, which will convert it to a double with the value of 3840 (since it is a valid hex representation of f00). Therefore we should probably extract the number according to our rules before passing it to atof.

[coveralls]

Coverage increased (+0.03%) when pulling 35c55e1 on mgreter:fix/color-token-ops into 7aaa45a on sass:master.

[mgreter]

So travis is happy and spec/basic/16_hex_arithmetic yields the same result as ruby sass 3.4.9 (with a few patches here and there). I'm still positive to ship it that way as I'm pretty confident that the fix should do the right thing. But it's not pretty or very clean, I agree on that. But we cannot rely on atof to split the number and unit for us anyway.

@xzyfer feel free to hit the merge button if you agree!

[xzyfer]

Done!