Skip to content

[css-typed-om] Simplifying expressions in CSSMathValue subclasses. #503

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

Closed
darrnshn opened this issue Oct 31, 2017 · 2 comments
Closed

[css-typed-om] Simplifying expressions in CSSMathValue subclasses. #503

darrnshn opened this issue Oct 31, 2017 · 2 comments

Comments

@darrnshn
Copy link
Collaborator

In the algorithm for adding CSSNumericValues, we have an explicit step (Step 3) for simplifying the expression if all values are CSSUnitValues with the same unit. However, none of the other subclasses seem to have this step, even though theoretically it should be possible (e.g. min(2px, 3px) == 2px). The inconsistency is a bit confusing and makes addition a special case in terms of implementation. Is there any particular reason for making addition simplify but not the others?

@tabatkins
Copy link
Member

.add(), .sub(), and .negate() all have special cases. (.sub() has it by virtue of calling into the .add() algo.)

.mul() could have a special-case here - check if this + every value is a CSSUnitValue with type=number, or at most one has a different type, then return a plain CSSUnitValue accordingly.

.div() could have a special case - if this+values are all CSSUnitValue, and all the values (not this) are number type, we can return a CSSUnitValue.

.invert() could special-case a number type.

I agree that .min() and .max() could have the exact same special-case as .add().

Happy to add these in if others think they're good ideas.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Simplifying expressions in CSSMathValue subclasses, and agreed to the following resolutions:

  • RESOLVED: mul/div/invert/max/min should have shortcut just like add/negate/sub
The full IRC log of that discussion <nainar> Topic: Simplifying expressions in CSSMathValue subclasses
<TabAtkins> GitHub: https://github.com//issues/503
<nainar> TabAtkins: Math operations in class - you can say px.add(some other px value)
<nainar> TabAtkins: some operations have shortcuts to return the operation. Have it for add, negate, sub. Dont have it for mul, div, invert, min, max
<nainar> TabAtkins: Have a sketch in the issue of how to do it. Apply these shortcuts to all operations or none?
<nainar> TabAtkins: Add always returns a CSSMathSum or a unit vaoue if easily computable?
<nainar> Rossen: Similiar with what we do to calc? You simplify it down
<TabAtkins> https://drafts.css-houdini.org/css-typed-om/#dom-cssnumericvalue-add
<nainar> TabAtkins: Yes
<nainar> Rossen: would align with what we already expose
<nainar> TabAtkins: People who want to increment length by 1 unit will still get a length unit and not complicated expression
<nainar> dholbert: addition inside of a loop - would be a good idea
<nainar> Rossen: it keeps expanding.
<nainar> RESOLVED: mul/div/invert/max/min should have shortcut just like add/negate/sub

MXEBot pushed a commit to mirror/chromium that referenced this issue Jan 16, 2018
To match behaviour specified in [1], we fix the "shortcut" logic
for mul, div and invert. Normally, when we mul/div/invert, we
would return a new CSSMathProduct/Product/Invert, respectively.
However, if all the arguments are CSSUnitValues then we might
be able to return a CSSUnitValue (e.g. 1.mul(2) is just 2 and
not CSSMathProduct(1, 2)). Whether we shortcut or not depends
on the units of the arguments:

- mul: Only shortcut if there's at most one non-number value.
- div: Only shortcut if the arguments are all number values
  (note we get this for free since div is just multiplying
   the inverses).
- invert: Only shortcut if its a number value.

These rules ensure that the result can be stored as a
CSSUnitValue (e.g. 2px.invert cannot be represented as a
CSSUnitValue as its unit is px^-1).

[1] w3c/css-houdini-drafts#503

Bug: 545318
Change-Id: I0c037c0a9026119097e85409efb0919a68e676f8
Reviewed-on: https://chromium-review.googlesource.com/866525
Reviewed-by: nainar <[email protected]>
Commit-Queue: Darren Shen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#529354}
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

3 participants