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

[css-values-4] Should arguments to min/max get simplified when only partial simplification is possible? #4756

Closed
emilio opened this issue Feb 7, 2020 · 16 comments

Comments

@emilio
Copy link
Collaborator

emilio commented Feb 7, 2020

That is, should max(10em, 10px, 20em) be simplified to max(10px, 20em)?

If so, we should probably also specify the order in which the factors appear in serialization to be specified like we do for sums (you really want to have the factors sorted before simplifying so you only need to do one pass over the arguments rather than multiple).

If not, we may want to also call that out in the spec. I think we may want to do that if only for consistency with sums.

@emilio emilio added the css-values-4 Current Work label Feb 7, 2020
@Loirooriol
Copy link
Contributor

I'd say no. It may seem fine now, but who knows, in the future we might have negative font sizes, making 1em resolve to a negative length. So 10em would be greater than 20em, making this simplification invalid.

@Loirooriol
Copy link
Contributor

Also, how far would this partial simplifications go?

Like max(3em, 3px + 1em, 1px + 2em), mathematically it can be simplified to max(3em, 3px + 1em) because 1px + 2em < 3em when 1em > 1px and 1px + 2em < 3px + 1em when 1em < 2px. So no matter the value of 1em, 1px + 2em will be smaller than some of the other expressions.

However, this may not be that trivial to find. And it can get much worse if we have nested functions, especially non-linear like exp() or non-continuous like round().

@emilio
Copy link
Collaborator Author

emilio commented Feb 7, 2020

I'd say no. It may seem fine now, but who knows, in the future we might have negative font sizes, making 1em resolve to a negative length. So 10em would be greater than 20em, making this simplification invalid.

That seems unlikely, but anyhow we did already resolve in #4399 (comment) that we'd simplify these aggressively. I think simplifying only when the nodes are leafs and they have the same units (and they're not percentages, as percentages can be resolved with negative basis) seems like a nice boundary to me.

@emilio
Copy link
Collaborator Author

emilio commented Feb 7, 2020

(it'd be the same boundary we draw for sums, to be clear)

@Loirooriol
Copy link
Contributor

I don't remember all the details from #4399 but it's talking about "fully resolved" expressions.

In your example I'm assuming that we haven't resolved em yet. Otherwise, if e.g. we know 1em = 10px, then max(10em, 10px, 20em) is simplified to calc(200px).

So if we don't know what's the value of 10em and 20em, then I don't think we should try to simplify max(10em, 10px, 20em).

Sums can do partial simplifications but I would special-case them since they are an operator. But I would treat functions as black boxes until we have resolved all their arguments and can call them. They may very well be black boxes once we let authors define their own custom functions with JS.

@emilio
Copy link
Collaborator Author

emilio commented Feb 7, 2020

The resolution above says:

All math functions aggressively simplify their calculations as far as possible for a given value-computation stage

That doesn't sound like we should only simplify them at computed/used-value time, and I think the spirit was "resolve stuff as much as possible to avoid more work later on".

I agree we shouldn't do this for all functions blindly, but the simplification in the OP doesn't seem unreasonable to me.

@tabatkins
Copy link
Member

Ah, my intention in getting that resolution was to be able to simplify down the arguments themselves to the minimal form; I wasn't expecting to do things like combining min() arguments that are already known to be comparable.

Similarly, I wasn't planning on reordering the arguments.

All of these sorts of changes are things I'm not necessarily opposed to, but it's more complication to the simplification algorithm for what I assume is relatively little benefit. Do you feel like it is significantly worthwhile to do so? If so I could be convinced.

Sums can do partial simplifications but I would special-case them since they are an operator. But I would treat functions as black boxes until we have resolved all their arguments and can call them. They may very well be black boxes once we let authors define their own custom functions with JS.

Yup, this is my current thinking.

@tabatkins
Copy link
Member

Any objection to closing no change, Emilio? Or do you still want to do some argument simplification in these two functions?

@emilio
Copy link
Collaborator Author

emilio commented Feb 11, 2020

I'm ambivalent, so I guess that's ok with no change :)

@emilio
Copy link
Collaborator Author

emilio commented Feb 11, 2020

Well, thinking twice, I'd tend to prefer simplifying slightly, just because work scales better.

So if you have min(10px, 32px, 500px, 10%), you need to do a bunch of comparisons every time that value is used instead of a couple... Now repeat that a thousand times and...

I wonder what @smfr and other implementors think. If they're fine with no change so am I.

@smfr
Copy link
Contributor

smfr commented Feb 11, 2020

I don't feel strongly. I think authors will usually just have two values in their min().

@tabatkins
Copy link
Member

I can go with merging values that are simple UnitValues with identical units, other than %s, if y'all really want. It's not an invasive change. (For the two-value cases that @smfr thinks will be most common, this is precisely when the whole function simplifies away anyway, so no difference there.)

@Loirooriol
Copy link
Contributor

But how do you decide which functions should be special-cased with this arguments simplifications? For example hypot() could be a candidate since mathematically

hypot(x, y, z) = hypot(hypot(x, y), z)

The problem is that in practice there can be a loss of precision, e.g. with JS in Firefox I get

Math.hypot(0, 1, 5);             // 5.099019513592785
Math.hypot(Math.hypot(0, 1), 5); // 5.0990195135927845

So, if this simplification is added only to min() and max(), will authors also expect it for hypot()? And if it's also added to hypot(), will authors expect the precision loss?

@tabatkins
Copy link
Member

Tree simplifications (beyond just replacing a function entirely with its result) are I think a step too far. I'd only be doing cross-argument simplification, and only in the simplest of cases (fully resolved unit values), aka exactly the simplification amount that we currently do for sums.

@Loirooriol
Copy link
Contributor

Loirooriol commented Mar 4, 2020

I mean, if you have width: hypot(3px, 4px, 5%), would it be simplified into hypot(5px, 5%), since hypot(3px, 4px) = 5px? Even if in some cases this may cause a precision loss?

@tabatkins
Copy link
Member

Oh, no, definitely not.

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

4 participants