[CSS Math Functions] Correctly serialize sin/cos/tan functions #15664

nt1m · 2023-07-08T08:21:19Z

`15a8c32`

[CSS Math Functions] Correctly serialize sin/cos/tan functions
https://bugs.webkit.org/show_bug.cgi?id=259011
rdar://111947212

Reviewed by Darin Adler.

There are 2 main changes:

- We now always try to resolve top level sin/cos/tan functions, cos(0) will serialize as calc(1) instead of cos(0) for instance. This is consistent with how calc(cos(0)) is serialized.

From the spec https://drafts.csswg.org/css-values-4/#calc-simplification:

> If root is an operator node that’s not one of the calc-operator nodes, and all of its calculation children are numeric values with enough information to compute the operation root represents, return the result of running root’s operation using its children, expressed in the result’s canonical unit.

- tan(90deg) & tan(-90deg) now return respectively calc(infinity) & calc(-infinity)

* LayoutTests/imported/w3c/web-platform-tests/css/css-values/sin-cos-tan-serialize-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-values/sin-cos-tan-serialize.html:

Update these tests from upstream WPT.

* Source/WebCore/css/calc/CSSCalcOperationNode.cpp:
(WebCore::CSSCalcOperationNode::evaluateOperator):
* Source/WebCore/css/calc/CSSCalcOperationNode.h:
* Source/WebCore/platform/calc/CalcExpressionOperation.cpp:
(WebCore::CalcExpressionOperation::evaluate const):

Canonical link: https://commits.webkit.org/265881@main

7ca55ae

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
~~🛠 🧪 merge~~	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-07-08T08:21:29Z

EWS run on previous version of this PR (hash ec758de)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-07-08T09:32:03Z

EWS run on previous version of this PR (hash 935b154)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	⏳ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-07-08T09:34:28Z

EWS run on previous version of this PR (hash e477925)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/css/calc/CSSCalcOperationNode.cpp

darinadler · 2023-07-08T13:41:08Z

Source/WebCore/platform/calc/CalcExpressionOperation.cpp

@@ -132,6 +133,14 @@ float CalcExpressionOperation::evaluate(float maxValue) const
    case CalcOperator::Tan: {
        if (m_children.size() != 1)
            return std::numeric_limits<double>::quiet_NaN();
+        double x = std::fmod(m_children[0]->evaluate(maxValue), piDouble * 2);


We should share this code, not write it out twice.

This whole file is duplicated unfortunately, we've had a bug where the hypot() function was wrong in only one of the two files, so this is something I'd like to address. I have some ideas how to refactor to prevent duplication altogether, but I'd like to do it separately to catch potential regressions.

Source/WebCore/css/calc/CSSCalcOperationNode.cpp

webkit-early-warning-system · 2023-07-08T19:50:17Z

EWS run on previous version of this PR (hash ccd42ec)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-07-08T19:57:32Z

EWS run on current version of this PR (hash 7ca55ae)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
~~🛠 🧪 merge~~	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=259011 rdar://111947212 Reviewed by Darin Adler. There are 2 main changes: - We now always try to resolve top level sin/cos/tan functions, cos(0) will serialize as calc(1) instead of cos(0) for instance. This is consistent with how calc(cos(0)) is serialized. From the spec https://drafts.csswg.org/css-values-4/#calc-simplification: > If root is an operator node that’s not one of the calc-operator nodes, and all of its calculation children are numeric values with enough information to compute the operation root represents, return the result of running root’s operation using its children, expressed in the result’s canonical unit. - tan(90deg) & tan(-90deg) now return respectively calc(infinity) & calc(-infinity) * LayoutTests/imported/w3c/web-platform-tests/css/css-values/sin-cos-tan-serialize-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-values/sin-cos-tan-serialize.html: Update these tests from upstream WPT. * Source/WebCore/css/calc/CSSCalcOperationNode.cpp: (WebCore::CSSCalcOperationNode::evaluateOperator): * Source/WebCore/css/calc/CSSCalcOperationNode.h: * Source/WebCore/platform/calc/CalcExpressionOperation.cpp: (WebCore::CalcExpressionOperation::evaluate const): Canonical link: https://commits.webkit.org/265881@main

webkit-commit-queue · 2023-07-08T20:08:43Z

Committed 265881@main (15a8c32): https://commits.webkit.org/265881@main

Reviewed commits have been landed. Closing PR #15664 and removing active labels.

nt1m self-assigned this Jul 8, 2023

nt1m added the CSS Cascading Style Sheets implementation label Jul 8, 2023

nt1m requested review from nmoucht, smfr, anttijk and darinadler July 8, 2023 08:21

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 8, 2023

nt1m removed the merging-blocked Applied to prevent a change from being merged label Jul 8, 2023

nt1m force-pushed the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch from ec758de to 935b154 Compare July 8, 2023 09:31

nt1m force-pushed the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch from 935b154 to e477925 Compare July 8, 2023 09:34

darinadler approved these changes Jul 8, 2023

View reviewed changes

nt1m force-pushed the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch from e477925 to ccd42ec Compare July 8, 2023 19:50

nt1m force-pushed the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch from ccd42ec to 7ca55ae Compare July 8, 2023 19:57

nt1m added merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merge-queue Applied to send a pull request to merge-queue labels Jul 8, 2023

webkit-commit-queue force-pushed the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch from 7ca55ae to 15a8c32 Compare July 8, 2023 20:08

webkit-commit-queue merged commit 15a8c32 into WebKit:main Jul 8, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 8, 2023

nt1m deleted the eng/CSS-Math-Functions-Correctly-serialize-trigonometric-functions branch July 8, 2023 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CSS Math Functions] Correctly serialize sin/cos/tan functions #15664

[CSS Math Functions] Correctly serialize sin/cos/tan functions #15664

nt1m commented Jul 8, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

darinadler Jul 8, 2023

nt1m Jul 8, 2023

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-commit-queue commented Jul 8, 2023

[CSS Math Functions] Correctly serialize sin/cos/tan functions #15664

[CSS Math Functions] Correctly serialize sin/cos/tan functions #15664

Conversation

nt1m commented Jul 8, 2023 • edited by webkit-early-warning-system

15a8c32

webkit-early-warning-system commented Jul 8, 2023 • edited

webkit-early-warning-system commented Jul 8, 2023 • edited

webkit-early-warning-system commented Jul 8, 2023 • edited

darinadler Jul 8, 2023

Choose a reason for hiding this comment

nt1m Jul 8, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Jul 8, 2023 • edited

webkit-early-warning-system commented Jul 8, 2023 • edited

webkit-commit-queue commented Jul 8, 2023

nt1m commented Jul 8, 2023 •

edited by webkit-early-warning-system

`15a8c32`

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited

webkit-early-warning-system commented Jul 8, 2023 •

edited