Bug 229786 – Implement abs,sign calc functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nikos Mouchtaris]

Implement abs,sign calc functions

[Nikos Mouchtaris]

Created attachment 437098 [details]
Patch

[Jon Lee]

rdar://82268535

[Nikos Mouchtaris]

Created attachment 438167 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Nikos Mouchtaris]

Created attachment 438306 [details]
Patch

[Nikos Mouchtaris]

Created attachment 438540 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 438540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438540&action=review

> Source/WebCore/ChangeLog:14
> +        Added support for calc functions sign and abs. Involved adding new css keywords and handling
> +        for parsing calc expression and computing the resulting value. Spec for these functions:
> +        https://drafts.csswg.org/css-values-4/#sign-funcs.

This paragraph should go above the Tests line.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:198
>      // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2, pow, sqrt, hypot

This list is no longer correct.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:466
> +            if (calcOperator() == CalcOperator::Sign)
> +                combinedUnitType = CSSUnitType::CSS_NUMBER;

The spec says "The abs(A) function contains one calculation A, and returns the absolute value of A, as the same type as the input" so I'm not sure this is right.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1108
> +        if (children.size() != 1)
> +            return std::numeric_limits<double>::quiet_NaN();
> +        if (children[0] > 0)
> +            return 1;
> +        if (children[0] < 0)
> +            return -1;
> +        return children[0];

Does this work for -0?

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:117
> +        if (m_children.size() != 1)
> +            return std::numeric_limits<double>::quiet_NaN();
> +        if (m_children[0]->evaluate(maxValue) > 0)
> +            return 1;
> +        if (m_children[0]->evaluate(maxValue) < 0)
> +            return -1;
> +        return m_children[0]->evaluate(maxValue);

Same question about -0

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/signs-abs-computed.html:15
> +test_math_used('sign(-1)', '-1', {type:'number', approx:0.1});

Test with -0

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/signs-abs-computed.html:31
> +test_math_used('calc(sign(0))', '0', {type:'number', approx:0.1});

Need to also test with inputs that are lengths, angles etc to test the "as the same type as the input" part.

[Nikos Mouchtaris]

Created attachment 438733 [details]
Patch

[Nikos Mouchtaris]

Created attachment 438860 [details]
Patch

[Nikos Mouchtaris]

Created attachment 439112 [details]
abs/sign functions

[Nikos Mouchtaris]

Created attachment 439169 [details]
abs/sign functions

[EWS]

Committed r283063 (242121@main): <https://commits.webkit.org/242121@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439169 [details].