less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17k stars 3.41k forks source link

fix: Output correct css when using css function in max(). #3708

Open lumburr opened 2 years ago

lumburr commented 2 years ago

What: fix #3604

Why:

How: For the following ast node

[Dimension, Expression]

less will skip non-Dimension nodes https://github.com/less/less.js/blob/master/packages/less/src/less/functions/number.js#L26-L31 So, max(1, calc(1 + 1)) is treated as max(1).

We should not skip these nodes. Checklist:

iChenLei commented 2 years ago

Thanks for contribution.

matthew-dean commented 2 years ago

So, max(1, calc(1 + 1)) is treated as max(1).

That doesn't seem correct. max(1, calc(1 + 1)) should output max(1, calc(1 + 1)) in the CSS. Similar for min(). See other function escaping code for functions like rgb() which output as-is if something like a var() statement is used.

lumburr commented 2 years ago

In rgb((0 128 var(--num)), the program tries to convert var(--number) into a number,Throws an exception if the conversion cannot be performed. https://github.com/less/less.js/blob/1eafc06db4ed062761ce6ca65044204cf32308a1/packages/less/src/less/functions/color.js#L40-L51 So, we also throw an exception on non-Dimension value. Since the node was not successfully converted, Less will be output as is

matthew-dean commented 2 years ago

@lumburr That's true but this will output as-is if compiled in Less.

.rule {
  color: rgb(0 128 var(--num));
}

So I'm saying, this in Less:

.rule {
  value: max(1, calc(1 + 1));
  }

...should output:

.rule {
   value: max(1, calc(1 + 1));
 }
lumburr commented 2 years ago

Ohh,@matthew-dean I may not have expressed it clearly. You are completely correct that in Less:

.rule {
  value: max(1, calc(1 + 1));
}

should output

.rule {
  value: max(1, calc(1 + 1));
}

And now the issue is caused because: max(1, calc(1 + 1)) is treated as max(1)

So what this PR does is to make Less output

.rule {
  value: max(1, calc(1 + 1));
}

correctly again.

matthew-dean commented 1 year ago

Oops, can you resolve conflicts?

bugron commented 1 year ago

Hi @lumburr, thanks for your contribution. Is there a chance you'll be able to fix merge conflicts here so that this PR can be merged?

matthew-dean commented 4 months ago

@iChenLei Would you have time to resolve conflicts and add test cases for this issue to see if they are resolved?

matthew-dean commented 4 months ago

Or @lumburr do you have time to do this?

SoonIter commented 4 months ago

@matthew-dean I've finished this in another pr #4266, could you CR and merge it? 😁