Fix operator associativity and result units in calc() - #239
Draft
meziantou wants to merge 2 commits into
Draft
Conversation
The calc() operand parser recursed into itself for the right operand of
every operator, which made the resulting expression tree right
associative. Chains of two or more identical operators were therefore
evaluated in the wrong order: calc(10px - 2px - 3px) built
Sub(10px, Sub(2px, 3px)) and computed to 11px instead of 5px, and
calc(100px / 2 / 5) built 100 / (2 / 5) instead of (100 / 2) / 5.
Mixed precedence expressions happened to come out right, so only chains
of same precedence operators were affected. No parse error was raised;
the wrong number was simply handed to the computed style.
Replace the four right recursive levels with two iterative loops that
fold the operands to the left, matching the grammar
expression := term (('+' | '-') term)*
term := factor (('*' | '/') factor)*
This also collapses the artificial split between the Add/Sub and the
Mul/Div levels, which is what introduced the asymmetry. Serialization is
unaffected, as CssText concatenates the operands in order without
adding parentheses.
Expected values are taken from Chrome via getComputedStyle.
Two independent defects made a division in calc() report a value with the wrong unit. Dividing two values that share a unit cancels the unit out and yields a plain number, but CssCalcDivExpression kept the unit of the left operand. calc(10px / 20px) computed to 0.5px instead of 0.5, so declarations such as opacity, flex-grow, z-index or line-height ended up with a length where a number was expected. CssMetricValueExtensions.WithValue creates the result through Activator.CreateInstance(type, value), and the single argument constructor of CssLengthValue defaults to pixels. Any unitless length was therefore turned into a length in pixels: calc(1 / 4) computed to 0.25px rather than 0.25. Preserve the unit of the template instead; this covers multiplication too, where calc(2 * 3) computed to 6px. Expected values are taken from Chrome via getComputedStyle, which reports 0.5 for opacity: calc(10px / 20px), 2 for flex-grow: calc(100px / 50px) and 150px for width: calc(100px / 2px * 3px).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of Changes
Prerequisites
Please make sure you can check the following two boxes:
Contribution Type
What types of changes does your code introduce? Put an
xin all the boxes that apply:Description
Fixes three related defects that made
calc()report silently wrong numbers. No parse error was raised in any of these cases; the wrong value was simply handed to the computed style. Every expected value below was taken from Chrome viagetComputedStylebefore writing the fix.1. Repeated operators were right associative
CalcParserrecursed into itself for the right operand of every operator, so the expression tree came out right associative. Chains of two or more identical operators were evaluated in the wrong order:calc(10px - 2px - 3px)5px11px5pxcalc(100px - 10px - 20px - 30px)40px100px40pxcalc(100px / 2 / 5)10px250px10pxMixed precedence expressions such as
calc(2 * 3px + 1px)happened to come out right, which is why this survived so long.The four right recursive levels are replaced by two iterative loops that fold operands to the left:
That also collapses the artificial split between the
Add/SubandMul/Divlevels, which is what introduced the asymmetry. Serialization is unaffected, sinceCssTextconcatenates operands in order without adding parentheses, so the existing round trip tests still hold.The loop guard additionally stops a failed left operand from being wrapped in an expression node with a
nullchild. Previously onlyrightwas null checked.2. Dividing equal units kept the unit
Dividing two values that share a unit cancels the unit out and yields a plain number, but
CssCalcDivExpressionkept the unit of the left operand, so a length landed where a number was expected:opacity: calc(10px / 20px)0.50.5px0.5flex-grow: calc(100px / 50px)22px2z-index: calc(100px / 25px)44px4opacity: calc(2s / 8s)0.250.25s0.25line-height: calc(40px / 20px)in Chrome computes to32px, that is the number2times the16pxfont size, which confirms the result is a number rather than2px.3.
WithValueturned unitless lengths into pixelsCssMetricValueExtensions.WithValuebuilds the result viaActivator.CreateInstance(type, value), and the single argument constructor ofCssLengthValuedefaults toUnit.Px. Any unitless length therefore came back as a length in pixels:opacity: calc(1 / 4)0.25px0.25flex-shrink: calc(20 / 6)3.33333333333333px3.33333333333333opacity: calc(2 * 3)6px6It now preserves the template's unit for lengths and falls back to the activator for the other metric types. This one also covers multiplication, not just division.