Bug 229786 - Implement abs,sign calc functions
Summary: Implement abs,sign calc functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Mouchtaris
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-01 17:57 PDT by Nikos Mouchtaris
Modified: 2021-09-24 15:35 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.55 KB, patch)
2021-09-01 17:58 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (30.94 KB, patch)
2021-09-14 14:06 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (31.11 KB, patch)
2021-09-15 17:19 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (31.15 KB, patch)
2021-09-17 18:31 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (58.41 KB, patch)
2021-09-20 14:50 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2021-09-21 14:35 PDT, Nikos Mouchtaris
simon.fraser: review+
Details | Formatted Diff | Diff
abs/sign functions (45.35 KB, patch)
2021-09-23 17:57 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
abs/sign functions (42.81 KB, patch)
2021-09-24 11:10 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikos Mouchtaris 2021-09-01 17:57:47 PDT
Implement abs,sign calc functions
Comment 1 Nikos Mouchtaris 2021-09-01 17:58:06 PDT
Created attachment 437098 [details]
Patch
Comment 2 Jon Lee 2021-09-07 13:49:04 PDT
rdar://82268535
Comment 3 Nikos Mouchtaris 2021-09-14 14:06:51 PDT
Created attachment 438167 [details]
Patch
Comment 4 EWS Watchlist 2021-09-14 14:07:52 PDT
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
Comment 5 Nikos Mouchtaris 2021-09-15 17:19:37 PDT
Created attachment 438306 [details]
Patch
Comment 6 Nikos Mouchtaris 2021-09-17 18:31:57 PDT
Created attachment 438540 [details]
Patch
Comment 7 Simon Fraser (smfr) 2021-09-17 19:20:01 PDT
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.
Comment 8 Nikos Mouchtaris 2021-09-20 14:50:40 PDT
Created attachment 438733 [details]
Patch
Comment 9 Nikos Mouchtaris 2021-09-21 14:35:32 PDT
Created attachment 438860 [details]
Patch
Comment 10 Nikos Mouchtaris 2021-09-23 17:57:47 PDT
Created attachment 439112 [details]
abs/sign functions
Comment 11 Nikos Mouchtaris 2021-09-24 11:10:14 PDT
Created attachment 439169 [details]
abs/sign functions
Comment 12 EWS 2021-09-24 15:35:09 PDT
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].