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

How to handle Maths #1880

Closed
lukeapage opened this issue Feb 17, 2014 · 102 comments
Closed

How to handle Maths #1880

lukeapage opened this issue Feb 17, 2014 · 102 comments

Comments

@lukeapage
Copy link
Member

lukeapage commented Feb 17, 2014

  1. We decided on strict maths going forward but there is general unease about forcing () around every calculation
  2. I don't think we want to change things massively or go back to the drawing board

See #1872

Possibility to add another case for calc which like font, with strict mode off, essentially turns strict mode on for a rule ? Too many exceptions in the future?

@seven-phases-max :
Well, there're a lot of other possibilities, e.g. ./ or require parens for division (e.g. 1/2->1/2 but (1/2)->0.5) etc...
Also, the "special cases" (e.g. properties where x/y can appear as shorthand) are not so rare (starting at padding/margin and ending with background/border-radius and eventually there can be more) so we just can't hardcode them all like it's done for font (and because of that I think that the current font "workaround" is just a temporary and quite dirty kludge that ideally should be removed too).

@seven-phases-max
Copy link
Member

I don't think we want to change things massively or go back to the drawing board

Yes, I suggested to start this only because if we want some "default" strict math in 3.0 we'll have to invent something less heavy than the current one (and a possibility of fixing all the problems without introducing any new --alt-strict-math option seems to be quite unreal because of hidden issues behind the font-like hardcoding solution...).

@lukeapage
Copy link
Member Author

I hadn't realised that its growing

https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius

:(

I think at the moment I am in favour of initially expanding strictMaths to be

  • Off
  • Division
  • On

and then for 2.0.0 setting the default to be Division

So..

Media Queries - if off, switch to division for sub nodes
Font - if off, switch to division for sub node
calc( - if off or division, switch to on for sub nodes

@seven-phases-max
Copy link
Member

https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius

Yep, I think they are towards to allowing "shorthand values" (i.e. x/y) in any "shorhand property"...

@lukeapage
Copy link
Member Author

related: #1480

@lukeapage
Copy link
Member Author

Another option.. process calc calls but only where the units make it possible e.g
calc( 1% + 2%) => 3%
calc(100% - 10 px) => unchanged

@seven-phases-max
Copy link
Member

This will fix calc but won't fix #1627 and related stuff.
I mean yes, calc(100% - 10 px) => unchanged could be a solution for the calc but this does not cancel the need for a less-heavy-than-parens-everywhere solution.

@calvinjuarez
Copy link
Member

If strict maths is being revisited, I'd like to suggest that Less functions like percentage() not require extra parentheses inside them. With strict maths on, you have to double-wrap the argument. This would greatly reduce the possibility of confusion and tough-to-find bugs, since both percentage(16 / 17) and percentage((16 / 17)) would be valid.

@seven-phases-max
Copy link
Member

@calvinjuarez V2 provides a plugin subsystem where a plugin can add an arbitrary number of functions to the environment and in this sense the core won't be able to decide if such built-in function expects a single value or an expression, i.e. it is allowed to accept 16/17, and then evaluating 16/17 before it is passed to a function would be incorrect (well, roughly speaking - it's a bit more complicated than that internally).

In that context ./ change seems to be my favourite though I understand it would be very dramatic change (if compared to (/), not counting it is also requires a whitespace before ., e.g. 16 ./17 and not 16./17, hmm).

@ponychicken
Copy link

Another thing where less currently breaks valid css is background shorthand:

background: url(image.png) 50%/300px no-repeat;

@matthew-dean
Copy link
Member

I think at the moment I am in favour of initially expanding strictMaths to be

Off
Division
On
and then for 2.0.0 setting the default to be Division

Sorry I didn't respond to this before 2.0 was released. I think that's a good instinct. Naked division operators simply conflict with too much of CSS, and will conflict more and more as people use more CSS3 features. And I understand people's pushback to not require parens for all math, since it's not needed in many cases.

@seven-phases-max

it is allowed to accept 16/17, and then evaluating 16/17 before it is passed to a function would be incorrect

Strictly speaking, yes that's absolutely correct. This should not be evaluated BEFORE it reaches the function, especially for a custom one. However, I think it would be smart to allow functions to opt to process arguments like this, if it makes sense. So, percentage would receive 16/17 as a text argument, and then the percentage function could make a call to some helper function to see if the text is math. In the case of percentage, the argument is not ambiguous. A CSS3 declaration is not valid here. So, other user-defined functions could operate the same way: opt to evaluate arguments for valid math equations. Therefore, it's not necessarily true that strict math "forces" a double parentheses in this case. I think to suggest that causes some pushback on the idea of strict math, that it must, as a necessity, be verbose in all cases.

@matthew-dean
Copy link
Member

Our --math options could be more like:

  • Always
  • Division (default)
  • Strict

So, we could deprecate --strict-math=true and make it an alias for --math=strict

@seven-phases-max
Copy link
Member

However, I think it would be smart to allow functions to opt to process arguments like this, if it makes sense.

They are allowed already (just test how percentage(16 / 17) and percentage((16 / 17)) work with --strict-math=on).
Though none of functions uses:

and then the percentage function could make a call to some helper function to see if the text is math.

simply because this way each function would have to have those ~20 extra lines of that extra-helpers-conversion-arg-checking-smart-arg-handling-stuff while the own function code is just one(!) line in most cases.

@matthew-dean
Copy link
Member

Each function has to have 20 extra lines? How do you figure?

If percentage already works with single parentheses strict math on, then I don't understand @calvinjuarez's issue. You seemed to imply in your response that single parentheses was not achievable.

@seven-phases-max
Copy link
Member

Each function has to have 20 extra lines? How do you figure?

That's just a typical exaggeration of: maybe 5, maybe 10, maybe 20... Who would care of exact numbers if real-code/aux-code ratio -> 0. (Taking a pragmatic approach I'd stop at percentage(16 / 17) just throwing a error instead of producing NaN% (like now), and not trying to perform any conversion... Though even this way it's still 4 extra lines of code - well, less or more acceptable I guess :)

My initial reply was implying he assumes this 16/17->(16/17) conversion to be performed implicitly by the compiler itself (and not by the function).


Speaking of options. In a perfect world I would dream there're be no options for this at all (i.e. it should be the one and the only and the rest to be marked as deprecated and eventually removed)... All these extra options make the codebase non-maintainable.. even for a tiny one-liner fix/change the number of edge-cases you have to predict and tests you need to perform grows exponentially... (almost for no reason).

@matthew-dean
Copy link
Member

I was just thinking it would be something like:

percentage: function(...) {
  Helper.convertMath(arguments);  // a function that doesn't need it doesn't call it
  // ... the rest
} 

Speaking of options. In a perfect world I would dream there're be no options for this at all

I agree. But we'll need the options in the short term. Especially if we're deviating from strict math and legacy behavior. They can both be marked as deprecated if we perfect a "smarter math" option.

@seven-phases-max
Copy link
Member

Helper.convertMath(arguments);

arguments is too optimistic.
At best (counting not just percentage - which is sort of useless anyway, but any other function expecting a numeric arg) a minimal requirement would be:

some: function(a, b, c) { 
    a = convert2number(a, "Error message when fails"); 
    // b is not a number for example
    c = convert2number(c, "Error message when fails"); 
} 

But that was not my point really, what I meant is probably something like: while this always is/was possible, nobody bothers to write such code... (with a few limited exceptions like)...

@matthew-dean
Copy link
Member

Yeah, I get you.

@calvinjuarez
Copy link
Member

percentage(16 / 17) just throwing a error

would be an improvement, certainly. (I can't think of any time that NaN% would be a useful output.)

As far as which functions would try to be smart about their arguments, there's no reason to try to anticipate everything. A helper function as @matthew-dean suggests could be implemented fairly simply as feature requests are made and discussed for specific functions to be smarter.

In fact, from the start, I'd say that just the math functions should be smart about their arguments. In fact, aside from min and max, math functions that accept more than one argument would really only need to test the first.

Edit: Actually, just whenever a math function is passed only one argument.

@corysimmons
Copy link

What's the status on this? We're getting complaints that LESS tries to parse invalid CSS properties as math. e.g. lost-column: 1/3; breaks.

It also seems http://www.w3.org/TR/css-grid-1/#grid-template-rowcol won't work with LESS.

Can someone patch this so if someone explicitly wants to use LESS to do division on a property that they need to wrap it in parens or something?

@matthew-dean
Copy link
Member

@corysimmons Didn't you just ask this on #2769 and get an answer? o_O

@matthew-dean
Copy link
Member

One thing that we didn't discuss when this went around the first time. It seems that the only real math conflict is division. And division is essentially an issue because we essentially are "repurposing" a CSS "separation" character.

Rather than try to "wrap" division in any number of ways, or wrap all math (as our first solution to this problem) I wonder why we never talked about the obvious: not repurposing an existing operator in the first place, especially when it so clearly a) makes Less code ambiguous, b) causes conflict.

After we've had time to digest this, wrapping math in parentheses, even if it's just division, still causes ambiguity for the cases where the math is already in parentheses. (Which could have also meant that parentheses was a wrong choice of "math wrapper".)

So why not deprecate / as a division operator? I know that it's a common division symbol, but there are other syntax trade-offs Less has done to extend CSS. And it's clear to me now that we're basically trying to work around a problem that is created by Less in the first place by using the single slash. We're creating ambiguity, and then attempting to reverse the ambiguity.

To be fair, the other math symbols are all repurposed in other parts of CSS, it's just that their usage in math doesn't cause any ambiguity.

I was going to propose some ideas, but, lo and behold, there are already standard alternative characters to division. Other than ÷, which isn't on the keyboard, I found this:

The division sign is also mathematically equivalent to the ratio symbol, customarily denoted by a colon (:) and read "is to." Thus, for any real number x and any nonzero real number y , this equation holds:

x ÷ y = x : y

In other words:

lost-column: 1/3;   //  value
lost-column: 1:3;   // division 

I know it would take a bit of parser tweaking to use a colon in math, but it is mathematically sound, apparently. I can't think of other symbols that would make better substitutes. Maybe | as a second choice? It would be more arbitrary though.

Thoughts?

@matthew-dean
Copy link
Member

Alternatively, we could still support the division symbol within parentheses and/or brackets, as in:

lost-column: 1/3;   //  value
lost-column: 1:3;   // division 
lost-column: (1/3);
lost-column: [1/3];

@matthew-dean
Copy link
Member

matthew-dean commented Feb 10, 2018

Re: nested functions

For this:

div {
    bar: calc(floor(1 + .1) + 20%);
}

As a developer, the expected result should be:

div {
    bar: calc(1 + 20%);
}

That's exactly what happens now (with these changes). It should not throw an error.

@seven-phases-max
Copy link
Member

seven-phases-max commented Feb 10, 2018

@matthew-dean
No, the way you wrote it, this:

foo: unit(1/2, px);

with -sm:on will compile to:

foo: .5px;

^- wrong.
Same for nested functions. Moreover, the fact that most of functions usually can't take arithmetic expressions as argument is not a reason to violate -sm: on;
Thus under -sm: on both lines:

foo: floor(1 + .1);
bar: calc(floor(1 + .1) + 20%);`

must throw a error (and this is what gets broken in the commit).

@matthew-dean
Copy link
Member

matthew-dean commented Feb 10, 2018

What are you talking about?

unit(1/2, px) with -sm:on:

ERROR: error evaluating function `unit`: the first argument to unit must be a number. Have you forgotten parenthesis?

calc(floor(1 + .1) + 20%) with -sm:on

ERROR: error evaluating function `floor`: argument must be a number

Check out the branch. Try it out.

@matthew-dean
Copy link
Member

One thing that might be more helpful with reviewing code is that if you aren't sure if something will produce incorrect output, please verify it. Or ask me to try a specific input to verify it works as expected. Or ask questions if you're not sure how / why it works. Calling it wrong without knowing if it is is not helpful.

@seven-phases-max
Copy link
Member

My apologizes. I guess I missed the indirect control of a this part.

Check out the branch. Try it out

I can't, sorry.

@matthew-dean
Copy link
Member

matthew-dean commented Feb 11, 2018

My apologizes. I guess I missed the indirect control of a this part.

No worries. With that addressed, does it look like it's working as you would expect?

@seven-phases-max
Copy link
Member

With that addressed, does it look like it's working as you would expect?

Yes, so far I can't imagine any other suspicious things there.

matthew-dean added a commit that referenced this issue Feb 11, 2018
@thany
Copy link

thany commented Feb 11, 2018

@matthew-dean
Essentially change the division operator in Less. Leading contender in the thread has been ./, which works but is a little strange to look at. \ is an unknown at this point without research [...] @thany, if you or someone else is a fan of this, then this is the work required. The last thing we want is just to move the breaks somewhere else.

Not at all actually. I suggested the \ operator as a last resort after not getting through, trying to get LESS to do only its own maths. If a new division operator is what it takes... Well, calc() can also do addition, subtraction, and multiplication. New operators for those? I think it's impractical. A new division operator might be a solution for things like font, background, and border-radius, but it's no solution for CSS maths.

I still think the real solution is for LESS to know about context. It should (yes, here I go again with my "should", what other word must I use?...) know that calc() is a CSS function and it should only evaluate maths that CSS can't do. But floor() doesn't exist in CSS, so that one it would have to evaluate (entirely) as to not spew out invalid CSS. I think this is mentioned before though, in different wording.

@matthew-dean
Copy link
Member

matthew-dean commented Feb 11, 2018

I still think the real solution is for LESS to know about context. It should (yes, here I go again with my "should", what other word must I use?...) know that calc() is a CSS function and it should only evaluate maths that CSS can't do. But floor() doesn't exist in CSS, so that one it would have to evaluate (entirely) as to not spew out invalid CSS. I think this is mentioned before though, in different wording.

Fair enough re: \, but as far as context, based on your other posts, I can guarantee it's a far more complex problem than you're thinking that it is. You really have to boil it down to:

12px/10px  // Is this division, or not?
10px/1.5 // is this division, or not? 

If you want to definitely leave / as a division operator, to mimic calc(), and not have it be ambiguous, then you absolutely need to make sure that the / is in parentheses (other than the calc() exception). That would provide predictable context.

That suggestion at the beginning of this thread is also still a possibility, and had some strong support. Essentially, that the default setting be that all math is supported outside parentheses EXCEPT division (and again, except calc(), is is now the case from 3.0+). Maybe that is the way to go. That would allow:

font: 10px/1.5;   // not division
font: (10px/10);  // division, result is 1px
font: 10px+15px/1.5;   // addition but not division, result is 25px/1.5
font: (10px+15px/1.5);  // result is 20px

Thing is, the result for 10px+15px/1.5 might still be hard for devs to grok, with an expression that seems to get "half-evaluated" unless it's in parentheses. If we had gone ahead and turned strict math on by default, it probably would have been fine. It's even less ambiguous. [shrug] Either way, wrapping math in some kind of context IS the way to eliminate ambiguity, and the only way for division other than changing the division operator.

The community has to essentially decide the direction. There are viable options in this thread. Each has downsides. Each has pain points. None will have full consensus. But IMO any of them are better than the current scenario. Just gotta swallow that pill.

@matthew-dean
Copy link
Member

matthew-dean commented Jun 24, 2018

Last call for a decision

So in an effort to work the impossible, I'd like to propose we do this (referring back to comments from 3 years ago.)

Give --strict-math 3 settings

  1. off
  2. division
  3. strict (alias on for back-compat)

To settle the debate, allow the --strict-math=division switch to do the following:

  1. Do not perform division with just / character outside of parentheses. E.g. border-radius: 55px / 25px; --> no-math (yes, that's valid CSS)
  2. Allow division with two different methods:
    a. . prefix - border-radius: 55px ./ 25px;
    b. parentheses - border-radius: (55px / 25px);
  3. Both forms are valid in parentheses - e.g. border-radius: (55px ./ 25px); would be valid

So, as a developer, if you feel one version is not your preference, you can use the other. Some may prefer not to use parentheses; some may prefer not to use a modified division operator. Something for everyone. And no more unpleasant surprises for those new to Less using what is now a widespread syntax in CSS, with the / being used in font, background, border-radius, @media queries, CSS Grid properties, and probably more in the future.

Next steps

I recommend this goes into a PR as an option, and then, as discussed, gets switched as the default for a future major version

matthew-dean added a commit to matthew-dean/less.js that referenced this issue Jun 24, 2018
@rjgotten
Copy link
Contributor

rjgotten commented Jun 24, 2018

@matthew-dean

I.e. the period acts sort of like the inverse of an escape sequence?
Not a bad idea, really...

And all things considered, probably one of the cleanest possible solutions.

@matthew-dean
Copy link
Member

@rjgotten Got a start here: https://github.com/matthew-dean/less.js/tree/strict-math-division

I'm still getting a weird error one of the tests, where it it seems to turn one of the dimension nodes into an operation node (and then throws an error because it can't perform an operation on an operation node. It doesn't seem to be a parsing problem because another test not running under strictMath: division works fine. Want to check it out and see if you can help?

matthew-dean added a commit that referenced this issue Jun 27, 2018
* Fixes Mixin call args not being visited
* Add ability to use ES6 in tests
* Fixes #3205 and partial 3.0 math regression #1880
@matthew-dean
Copy link
Member

What I'd like to do is close this issue and create new issues that handle the remaining math questions. Specifically:

  1. Handling division, and the strictMath: 'division' feature.
  2. Handling math of mixed units. See: How to handle mixed unit math #3047

matthew-dean added a commit to matthew-dean/less.js that referenced this issue Jul 7, 2018
matthew-dean added a commit that referenced this issue Jul 11, 2018
Fixes #1880 - Adds two new math modes and deprecates strictMath
matthew-dean pushed a commit that referenced this issue Jul 11, 2018
* WIP - Added strictMath: 'division' option

* Removes `less-rhino` (broken for a long time) - Fixes #3241

* Expressions require a delimiter of some kind in mixin args

* Removes "double paren" issue for boolean / if function

* WIP each() re-implementation

* Allows plain conditions without parentheses

* Added each() and tests

* Added tests calling mixins from each()

* Fixes #1880 - Adds two new math modes and deprecates strictMath

* Allows named args for detached ruleset (anonymous mixin)

* Makes sure this doesn't regress needing parens around mixin guard conditions

* Remove javascriptEnabled from browser options check, add to defaults

* Workaround weird Jasmine conversion of < to HTML entity

* Removes remaining Rhino cruft

* Remove rhino files from bower

* Bump Jasmine version

* Added .() example to each

* v3.6.0

* Update CHANGELOG.md

* add explicit nested anonymous mixin test

* add explicit nested anonymous mixin test result

* derp: align test with expected output

* v3.7.0 (#3279)

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md
matthew-dean added a commit that referenced this issue Jul 18, 2018
* Rename relativeUrls option to rewriteUrls

* Refactor contexts.js

- Rename isPathRelative to pathRequiresRewrite
- Add special handling for rewriteUrls=relative
- Add rewritePath for path rewriting
- Ensure that explicit relative paths stay explicit relative
- Remove code style inconsistencies

* Add missing tests for url rewriting

* Rename rewrite-urls option value to explicit-relative

* Add missing tests for url rewriting

* Refactor rewrite urls options

- Rename explicit-relative to local
- Add rewrite-urls argument validation
- Replace unknown CSS property background-url with background-image

Based on discussion #3041

* Re-add tests for deprecated relative-urls option

* Improve tests

- Remove redundant tests
- Add rootpath-rewrite-urls-all test
- Add rootpath-rewrite-urls-local test

* Fix typo in unknown argument warning

* Revert old tests to deprecated relativeUrls option again

* Added more CSS Grid tests

* All tests passing

*  Merge branch 'master' into feature/rewrite-urls (#3282)

* WIP - Added strictMath: 'division' option

* Removes `less-rhino` (broken for a long time) - Fixes #3241

* Expressions require a delimiter of some kind in mixin args

* Removes "double paren" issue for boolean / if function

* WIP each() re-implementation

* Allows plain conditions without parentheses

* Added each() and tests

* Added tests calling mixins from each()

* Fixes #1880 - Adds two new math modes and deprecates strictMath

* Allows named args for detached ruleset (anonymous mixin)

* Makes sure this doesn't regress needing parens around mixin guard conditions

* Remove javascriptEnabled from browser options check, add to defaults

* Workaround weird Jasmine conversion of < to HTML entity

* Removes remaining Rhino cruft

* Remove rhino files from bower

* Bump Jasmine version

* Added .() example to each

* v3.6.0

* Update CHANGELOG.md

* add explicit nested anonymous mixin test

* add explicit nested anonymous mixin test result

* derp: align test with expected output

* v3.7.0 (#3279)

* Update CHANGELOG.md

* Rewrite the rewriteUrls feature to use same pattern as strictMath-to-math

* Update console warning for --relative-urls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants