Bug 234176 – Incorrect handling of NaN inside calc() for top-level calculation

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mike Taylor]

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)

[Radar WebKit Bug Importer]

<rdar://problem/86651372>

[Nikos Mouchtaris]

Created attachment 452804 [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

[Jon Lee]

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?

[Nikos Mouchtaris]

Created attachment 452831 [details]
Patch

[Nikos Mouchtaris]

(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.

[Nikos Mouchtaris]

Created attachment 454892 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 454906 [details]
Patch

[Nikos Mouchtaris]

(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.

[Nikos Mouchtaris]

Created attachment 455028 [details]
Patch

[Nikos Mouchtaris]

Created attachment 455281 [details]
Patch

[Nikos Mouchtaris]

Created attachment 455286 [details]
Patch

[Nikos Mouchtaris]

https://github.com/web-platform-tests/wpt/pull/33306

[Nikos Mouchtaris]

Created attachment 455720 [details]
Patch

[Nikos Mouchtaris]

Created attachment 455793 [details]
Patch

[Nikos Mouchtaris]

Created attachment 455794 [details]
Patch

[Simon Fraser (smfr)]

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 }

[Nikos Mouchtaris]

Created attachment 455802 [details]
Patch

[Nikos Mouchtaris]

(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.

[Nikos Mouchtaris]

Created attachment 455808 [details]
Patch

[EWS]

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].