In https://drafts.csswg.org/css-values/#calc-type-checking, more specifically in https://drafts.csswg.org/css-values/#top-level-calculation, it states: > If a top-level calculation (a math function not nested inside of another math function) would produce a value whose numeric part is NaN, it instead act as though the numeric part is +∞. Looking at https://wpt.fyi/results/css/css-values/calc-infinity-nan-computed.html?label=master&label=experimental&aligned&q=infinity-nan (source @ https://github.com/web-platform-tests/wpt/blob/master/css/css-values/calc-infinity-nan-computed.html#L19), Safari Tech Preview 136 is resolving "calc(NaN * 1px)" to "0", rather than ~Inifinity, like it does for "calc(infinity * 1px)". Not sure if the spec was changed after the implementation in https://bugs.webkit.org/show_bug.cgi?id=231044, but it looks like a bug right now. (there are other WPT failures, but I haven't looked closely at them)
<rdar://problem/86651372>
Created attachment 452804 [details] Patch
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 on attachment 452804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452804&action=review > Source/WebCore/platform/animation/Animation.h:144 > + void setDuration(double d) { m_duration = d; m_durationSet = true; } Why remove the assert?
Created attachment 452831 [details] Patch
(In reply to Jon Lee from comment #4) > Comment on attachment 452804 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452804&action=review > > > Source/WebCore/platform/animation/Animation.h:144 > > + void setDuration(double d) { m_duration = d; m_durationSet = true; } > > Why remove the assert? This was originally causing the test to crash but that is not the case now with the fix. Fixed.
Created attachment 454892 [details] Patch
Comment on attachment 454892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454892&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-values/calc-infinity-nan-computed.html:48 > +// test_computed_value_greater_or_lower_than("width", "calc(NaN * 1px)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1cm)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(NaN * 1rem)", APPROX_INFINITY); > + > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px - infinity * 1%)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px + infinity * 1%)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(min(NaN * 1px, infinity * 1px) + max(infinity * 1px, -infinity * 1px))", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px - max(infinity * 1%, 0%))", APPROX_INFINITY); > + > +// test_computed_value_greater_or_lower_than("width", "calc(max(infinity * 1px, 10px))", APPROX_INFINITY); > + > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1px)", APPROX_NEGATIVE_INFINITY); > +// test_computed_value_greater_or_lower_than("margin-left", "calc(min(1px, -infinity * 1%))", APPROX_NEGATIVE_INFINITY); > + > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1%)", APPROX_NEGATIVE_INFINITY); > +// test_computed_value_greater_or_lower_than("margin-left", "calc(max(10000px, 0px) + min(-infinity * 1px, infinity * 1px))", APPROX_NEGATIVE_INFINITY); > + > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1px - infinity * 1px)", APPROX_NEGATIVE_INFINITY); > +// test_computed_value_greater_or_lower_than("margin-left", "calc(min(-infinity * 1px, 10px))", APPROX_NEGATIVE_INFINITY); > + > +// // For <time> > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(NaN * 1s)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(infinity * 1s)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(1 / 0 * 1s)", APPROX_INFINITY); > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(max(infinity * 1s, 10s)", APPROX_INFINITY); > + > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(-infinity* 1s)", APPROX_NEGATIVE_INFINITY); > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(max(10000s, 0s) + min(-infinity * 1s, infinity * 1s))", APPROX_NEGATIVE_INFINITY); > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(min(-infinity * 1s, 10s))", APPROX_NEGATIVE_INFINITY); I don't think you meant to leave these all commented out.
Created attachment 454906 [details] Patch
(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 454892 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454892&action=review > > > LayoutTests/imported/w3c/web-platform-tests/css/css-values/calc-infinity-nan-computed.html:48 > > +// test_computed_value_greater_or_lower_than("width", "calc(NaN * 1px)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1cm)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(NaN * 1rem)", APPROX_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px - infinity * 1%)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px + infinity * 1%)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(min(NaN * 1px, infinity * 1px) + max(infinity * 1px, -infinity * 1px))", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("width", "calc(infinity * 1px - max(infinity * 1%, 0%))", APPROX_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("width", "calc(max(infinity * 1px, 10px))", APPROX_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1px)", APPROX_NEGATIVE_INFINITY); > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(min(1px, -infinity * 1%))", APPROX_NEGATIVE_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1%)", APPROX_NEGATIVE_INFINITY); > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(max(10000px, 0px) + min(-infinity * 1px, infinity * 1px))", APPROX_NEGATIVE_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(-infinity * 1px - infinity * 1px)", APPROX_NEGATIVE_INFINITY); > > +// test_computed_value_greater_or_lower_than("margin-left", "calc(min(-infinity * 1px, 10px))", APPROX_NEGATIVE_INFINITY); > > + > > +// // For <time> > > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(NaN * 1s)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(infinity * 1s)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(1 / 0 * 1s)", APPROX_INFINITY); > > +// test_computed_value_greater_or_lower_than("animation-duration", "calc(max(infinity * 1s, 10s)", APPROX_INFINITY); > > + > > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(-infinity* 1s)", APPROX_NEGATIVE_INFINITY); > > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(max(10000s, 0s) + min(-infinity * 1s, infinity * 1s))", APPROX_NEGATIVE_INFINITY); > > +// test_computed_value_greater_or_lower_than("transition-delay", "calc(min(-infinity * 1s, 10s))", APPROX_NEGATIVE_INFINITY); > > I don't think you meant to leave these all commented out. Fixed.
Created attachment 455028 [details] Patch
Created attachment 455281 [details] Patch
Created attachment 455286 [details] Patch
https://github.com/web-platform-tests/wpt/pull/33306
Created attachment 455720 [details] Patch
Created attachment 455793 [details] Patch
Created attachment 455794 [details] Patch
Comment on attachment 455794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455794&action=review > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1524 > + return depth ? result : convertToTopLevelValue(result); Maybe it makes more sense for the caller to do this, rather than passing 'depth' into this function? There should be just one place we know we're at the root, I think? > Source/WebCore/css/calc/CSSCalcOperationNode.h:134 > + static double evaluateOperator(CalcOperator, const Vector<double>&, bool depth); 'depth' sounds integral, not boolean. > Source/WebCore/css/calc/CSSCalcOperationNode.h:147 > bool m_allowsNegativePercentageReference = false; > + bool m_depth = false; = { false } and depth should have a different name, or maybe be an enum class IsRoot : bool { No, Yes }
Created attachment 455802 [details] Patch
(In reply to Simon Fraser (smfr) from comment #18) > Comment on attachment 455794 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455794&action=review > > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1524 > > + return depth ? result : convertToTopLevelValue(result); > > Maybe it makes more sense for the caller to do this, rather than passing > 'depth' into this function? There should be just one place we know we're at > the root, I think? Fixed. > > > Source/WebCore/css/calc/CSSCalcOperationNode.h:134 > > + static double evaluateOperator(CalcOperator, const Vector<double>&, bool depth); > > 'depth' sounds integral, not boolean. Fixed. > > > Source/WebCore/css/calc/CSSCalcOperationNode.h:147 > > bool m_allowsNegativePercentageReference = false; > > + bool m_depth = false; > > = { false } > > and depth should have a different name, or maybe be an enum class IsRoot : > bool { No, Yes } Fixed.
Created attachment 455808 [details] Patch
Committed r291911 (248899@main): <https://commits.webkit.org/248899@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455808 [details].