Summary: | Incorrect handling of NaN inside calc() for top-level calculation | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Taylor <miketaylr> | ||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nikos Mouchtaris <nmouchtaris> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jonlee, macpherson, menard, nmouchtaris, seokho, simon.fraser, webkit-bug-importer, youennf | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Mike Taylor
2021-12-10 14:42:00 PST
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
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]. |