Skip to content

Commit 0b45a68

Browse files
Calme1709AtkinsSJ
Calme1709
authored and
AtkinsSJ
committed
LibWeb: Avoid early conversion to CSSPixels when simplifying calculation
Converting to CSSPixels caused us to lose precision and the sign of signed zeroes. The values we resolve against in Length::ResolutionContext are still themselves rounded too early but this is in the right direction.
1 parent 43b06cb commit 0b45a68

File tree

6 files changed

+63
-39
lines changed

6 files changed

+63
-39
lines changed

‎Libraries/LibWeb/CSS/Length.cpp‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,74 +38,84 @@ Length Length::percentage_of(Percentage const& percentage) const
3838
}
3939

4040
CSSPixels Length::font_relative_length_to_px(Length::FontMetrics const& font_metrics, Length::FontMetrics const& root_font_metrics) const
41+
{
42+
return CSSPixels::nearest_value_for(font_relative_length_to_px_without_rounding(font_metrics, root_font_metrics));
43+
}
44+
45+
double Length::font_relative_length_to_px_without_rounding(Length::FontMetrics const& font_metrics, Length::FontMetrics const& root_font_metrics) const
4146
{
4247
switch (m_unit) {
4348
case LengthUnit::Em:
44-
return CSSPixels::nearest_value_for(m_value * font_metrics.font_size.to_double());
49+
return m_value * font_metrics.font_size.to_double();
4550
case LengthUnit::Rem:
46-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.font_size.to_double());
51+
return m_value * root_font_metrics.font_size.to_double();
4752
case LengthUnit::Ex:
48-
return CSSPixels::nearest_value_for(m_value * font_metrics.x_height.to_double());
53+
return m_value * font_metrics.x_height.to_double();
4954
case LengthUnit::Rex:
50-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.x_height.to_double());
55+
return m_value * root_font_metrics.x_height.to_double();
5156
case LengthUnit::Cap:
52-
return CSSPixels::nearest_value_for(m_value * font_metrics.cap_height.to_double());
57+
return m_value * font_metrics.cap_height.to_double();
5358
case LengthUnit::Rcap:
54-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.cap_height.to_double());
59+
return m_value * root_font_metrics.cap_height.to_double();
5560
case LengthUnit::Ch:
56-
return CSSPixels::nearest_value_for(m_value * font_metrics.zero_advance.to_double());
61+
return m_value * font_metrics.zero_advance.to_double();
5762
case LengthUnit::Rch:
58-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.zero_advance.to_double());
63+
return m_value * root_font_metrics.zero_advance.to_double();
5964
case LengthUnit::Ic:
6065
// FIXME: Use the "advance measure of the “水” (CJK water ideograph, U+6C34) glyph"
61-
return CSSPixels::nearest_value_for(m_value * font_metrics.font_size.to_double());
66+
return m_value * font_metrics.font_size.to_double();
6267
case LengthUnit::Ric:
6368
// FIXME: Use the "advance measure of the “水” (CJK water ideograph, U+6C34) glyph"
64-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.font_size.to_double());
69+
return m_value * root_font_metrics.font_size.to_double();
6570
case LengthUnit::Lh:
66-
return CSSPixels::nearest_value_for(m_value * font_metrics.line_height.to_double());
71+
return m_value * font_metrics.line_height.to_double();
6772
case LengthUnit::Rlh:
68-
return CSSPixels::nearest_value_for(m_value * root_font_metrics.line_height.to_double());
73+
return m_value * root_font_metrics.line_height.to_double();
6974
default:
7075
VERIFY_NOT_REACHED();
7176
}
7277
}
7378

7479
CSSPixels Length::viewport_relative_length_to_px(CSSPixelRect const& viewport_rect) const
80+
{
81+
return CSSPixels::nearest_value_for(viewport_relative_length_to_px_without_rounding(viewport_rect));
82+
}
83+
84+
double Length::viewport_relative_length_to_px_without_rounding(CSSPixelRect const& viewport_rect) const
7585
{
7686
switch (m_unit) {
7787
case LengthUnit::Vw:
7888
case LengthUnit::Svw:
7989
case LengthUnit::Lvw:
8090
case LengthUnit::Dvw:
81-
return viewport_rect.width() * (CSSPixels::nearest_value_for(m_value) / 100);
91+
return viewport_rect.width() * m_value / 100;
8292
case LengthUnit::Vh:
8393
case LengthUnit::Svh:
8494
case LengthUnit::Lvh:
8595
case LengthUnit::Dvh:
86-
return viewport_rect.height() * (CSSPixels::nearest_value_for(m_value) / 100);
96+
return viewport_rect.height() * m_value / 100;
8797
case LengthUnit::Vi:
8898
case LengthUnit::Svi:
8999
case LengthUnit::Lvi:
90100
case LengthUnit::Dvi:
91101
// FIXME: Select the width or height based on which is the inline axis.
92-
return viewport_rect.width() * (CSSPixels::nearest_value_for(m_value) / 100);
102+
return viewport_rect.width() * m_value / 100;
93103
case LengthUnit::Vb:
94104
case LengthUnit::Svb:
95105
case LengthUnit::Lvb:
96106
case LengthUnit::Dvb:
97107
// FIXME: Select the width or height based on which is the block axis.
98-
return viewport_rect.height() * (CSSPixels::nearest_value_for(m_value) / 100);
108+
return viewport_rect.height() * m_value / 100;
99109
case LengthUnit::Vmin:
100110
case LengthUnit::Svmin:
101111
case LengthUnit::Lvmin:
102112
case LengthUnit::Dvmin:
103-
return min(viewport_rect.width(), viewport_rect.height()) * (CSSPixels::nearest_value_for(m_value) / 100);
113+
return min(viewport_rect.width(), viewport_rect.height()) * m_value / 100;
104114
case LengthUnit::Vmax:
105115
case LengthUnit::Svmax:
106116
case LengthUnit::Lvmax:
107117
case LengthUnit::Dvmax:
108-
return max(viewport_rect.width(), viewport_rect.height()) * (CSSPixels::nearest_value_for(m_value) / 100);
118+
return max(viewport_rect.width(), viewport_rect.height()) * m_value / 100;
109119
default:
110120
VERIFY_NOT_REACHED();
111121
}

‎Libraries/LibWeb/CSS/Length.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,18 @@ class WEB_API Length {
7878
return to_px_slow_case(node);
7979
}
8080

81+
ALWAYS_INLINE double to_px_without_rounding(ResolutionContext const& context) const
82+
{
83+
if (is_absolute())
84+
return absolute_length_to_px_without_rounding();
85+
if (is_font_relative())
86+
return font_relative_length_to_px_without_rounding(context.font_metrics, context.root_font_metrics);
87+
if (is_viewport_relative())
88+
return viewport_relative_length_to_px_without_rounding(context.viewport_rect);
89+
90+
VERIFY_NOT_REACHED();
91+
}
92+
8193
ALWAYS_INLINE CSSPixels to_px(CSSPixelRect const& viewport_rect, FontMetrics const& font_metrics, FontMetrics const& root_font_metrics) const
8294
{
8395
if (is_absolute())
@@ -108,7 +120,9 @@ class WEB_API Length {
108120
}
109121

110122
CSSPixels font_relative_length_to_px(FontMetrics const& font_metrics, FontMetrics const& root_font_metrics) const;
123+
double font_relative_length_to_px_without_rounding(FontMetrics const& font_metrics, FontMetrics const& root_font_metrics) const;
111124
CSSPixels viewport_relative_length_to_px(CSSPixelRect const& viewport_rect) const;
125+
double viewport_relative_length_to_px_without_rounding(CSSPixelRect const& viewport_rect) const;
112126

113127
// Returns empty optional if it's already absolute.
114128
Optional<Length> absolutize(CSSPixelRect const& viewport_rect, FontMetrics const& font_metrics, FontMetrics const& root_font_metrics) const;

‎Libraries/LibWeb/CSS/StyleValues/CalculatedStyleValue.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2531,7 +2531,7 @@ CalculatedStyleValue::CalculationResult CalculatedStyleValue::CalculationResult:
25312531
return AK::NaN<double>;
25322532
}
25332533

2534-
return length.to_px(context.length_resolution_context.value()).to_double();
2534+
return length.to_px_without_rounding(context.length_resolution_context.value());
25352535
},
25362536
[](Resolution const& resolution) { return resolution.to_dots_per_pixel(); },
25372537
[](Time const& time) { return time.to_seconds(); },
@@ -2900,7 +2900,7 @@ NonnullRefPtr<CalculationNode const> simplify_a_calculation_tree(CalculationNode
29002900
if (length.is_absolute())
29012901
return NumericCalculationNode::create(Length::make_px(length.absolute_length_to_px()).percentage_of(*percentage), context);
29022902
if (resolution_context.length_resolution_context.has_value())
2903-
return NumericCalculationNode::create(Length::make_px(length.to_px(resolution_context.length_resolution_context.value())).percentage_of(*percentage), context);
2903+
return NumericCalculationNode::create(Length::make_px(length.to_px_without_rounding(resolution_context.length_resolution_context.value())).percentage_of(*percentage), context);
29042904
return nullptr;
29052905
},
29062906
[&](Time const& time) -> RefPtr<NumericCalculationNode const> {
@@ -2938,9 +2938,9 @@ NonnullRefPtr<CalculationNode const> simplify_a_calculation_tree(CalculationNode
29382938
if (length.unit() == LengthUnit::Px)
29392939
return nullptr;
29402940
if (length.is_absolute())
2941-
return NumericCalculationNode::create(Length::make_px(length.absolute_length_to_px()), context);
2941+
return NumericCalculationNode::create(Length::make_px(length.absolute_length_to_px_without_rounding()), context);
29422942
if (resolution_context.length_resolution_context.has_value())
2943-
return NumericCalculationNode::create(Length::make_px(length.to_px(resolution_context.length_resolution_context.value())), context);
2943+
return NumericCalculationNode::create(Length::make_px(length.to_px_without_rounding(resolution_context.length_resolution_context.value())), context);
29442944
return nullptr;
29452945
},
29462946
[&](Number const&) -> RefPtr<CalculationNode const> {

‎Tests/LibWeb/Text/expected/wpt-import/css/css-values/round-mod-rem-computed.txt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ Harness status: OK
22

33
Found 229 tests
44

5-
217 Pass
6-
12 Fail
5+
219 Pass
6+
10 Fail
77
Pass round(10,10) should be used-value-equivalent to 10
88
Pass mod(1,1) should be used-value-equivalent to 0
99
Pass rem(1,1) should be used-value-equivalent to 0
@@ -61,8 +61,8 @@ Pass calc(rem(mod(18,5),mod(17,5))) should be used-value-equivalent to 1
6161
Pass calc(mod(-140,-90)) should be used-value-equivalent to -50
6262
Pass calc(mod(rem(1,18)* -1,5)) should be used-value-equivalent to 4
6363
Pass round(10px,6px) should be used-value-equivalent to 12px
64-
Fail round(10cm,6cm) should be used-value-equivalent to 12cm
65-
Fail round(10mm,6mm) should be used-value-equivalent to 12mm
64+
Pass round(10cm,6cm) should be used-value-equivalent to 12cm
65+
Pass round(10mm,6mm) should be used-value-equivalent to 12mm
6666
Pass round(10Q, 6Q) should be used-value-equivalent to 12Q
6767
Pass round(10in,6in) should be used-value-equivalent to 12in
6868
Pass round(10pc,6pc) should be used-value-equivalent to 12pc

‎Tests/LibWeb/Text/expected/wpt-import/css/css-values/signs-abs-computed.txt‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ Harness status: OK
22

33
Found 233 tests
44

5-
221 Pass
6-
12 Fail
5+
227 Pass
6+
6 Fail
77
Pass abs(1) should be used-value-equivalent to 1
88
Pass sign(1) should be used-value-equivalent to 1
99
Pass abs(-1) should be used-value-equivalent to 1
@@ -114,17 +114,17 @@ Pass clamp(-1, calc( 1 / sign(sign(0vmax))), 1) should be used-value-equivalent
114114
Pass calc(sign(-0px)) should be used-value-equivalent to 0
115115
Pass clamp(-1, calc( 1 / sign(sign(-0px))), 1) should be used-value-equivalent to -1
116116
Pass calc(sign(-0cm)) should be used-value-equivalent to 0
117-
Fail clamp(-1, calc( 1 / sign(sign(-0cm))), 1) should be used-value-equivalent to -1
117+
Pass clamp(-1, calc( 1 / sign(sign(-0cm))), 1) should be used-value-equivalent to -1
118118
Pass calc(sign(-0mm)) should be used-value-equivalent to 0
119-
Fail clamp(-1, calc( 1 / sign(sign(-0mm))), 1) should be used-value-equivalent to -1
119+
Pass clamp(-1, calc( 1 / sign(sign(-0mm))), 1) should be used-value-equivalent to -1
120120
Pass calc(sign(-0Q)) should be used-value-equivalent to 0
121-
Fail clamp(-1, calc( 1 / sign(sign(-0Q))), 1) should be used-value-equivalent to -1
121+
Pass clamp(-1, calc( 1 / sign(sign(-0Q))), 1) should be used-value-equivalent to -1
122122
Pass calc(sign(-0in)) should be used-value-equivalent to 0
123-
Fail clamp(-1, calc( 1 / sign(sign(-0in))), 1) should be used-value-equivalent to -1
123+
Pass clamp(-1, calc( 1 / sign(sign(-0in))), 1) should be used-value-equivalent to -1
124124
Pass calc(sign(-0pc)) should be used-value-equivalent to 0
125-
Fail clamp(-1, calc( 1 / sign(sign(-0pc))), 1) should be used-value-equivalent to -1
125+
Pass clamp(-1, calc( 1 / sign(sign(-0pc))), 1) should be used-value-equivalent to -1
126126
Pass calc(sign(-0pt)) should be used-value-equivalent to 0
127-
Fail clamp(-1, calc( 1 / sign(sign(-0pt))), 1) should be used-value-equivalent to -1
127+
Pass clamp(-1, calc( 1 / sign(sign(-0pt))), 1) should be used-value-equivalent to -1
128128
Pass calc(sign(-0em)) should be used-value-equivalent to 0
129129
Pass clamp(-1, calc( 1 / sign(sign(-0em))), 1) should be used-value-equivalent to -1
130130
Pass calc(sign(-0ex)) should be used-value-equivalent to 0

‎Tests/LibWeb/Text/expected/wpt-import/css/css-values/typed_arithmetic.txt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ Harness status: OK
22

33
Found 35 tests
44

5-
29 Pass
6-
6 Fail
5+
32 Pass
6+
3 Fail
77
Pass min(1em, 110px / 10px * 1px) should be used-value-equivalent to 10px
88
Pass max(10px, 110px / 10px * 1px) should be used-value-equivalent to 11px
99
Pass max(1em + 2px, 110px / 10px * 1px) should be used-value-equivalent to 12px
@@ -26,14 +26,14 @@ Pass e.style['width'] = "calc((1% * 1deg) / 1px)" should not set the property va
2626
Pass e.style['width'] = "calc((1% * 1% * 1%) / 1px)" should not set the property value
2727
Pass Property width value 'calc(1px * 10em / 0em)'
2828
Pass Property width value 'calc(1px / 1px * 10em * infinity)'
29-
Fail Property margin-left value 'calc(1px * 10em / -0em)'
29+
Pass Property margin-left value 'calc(1px * 10em / -0em)'
3030
Pass Property z-index value 'calc(10em / 0em)'
3131
Pass sign(-0em / 1px) should be used-value-equivalent to 0
32-
Fail clamp(-1, 1 / sign(-0em / 1px), 1) should be used-value-equivalent to -1
32+
Pass clamp(-1, 1 / sign(-0em / 1px), 1) should be used-value-equivalent to -1
3333
Fail sign( 0cqi / 1px) should be used-value-equivalent to 0
3434
Fail clamp(-1, 1 / sign( 0cqi / 1px), 1) should be used-value-equivalent to 1
3535
Pass sign(atan2(-0cap / 1px, 0em / 1px)) should be used-value-equivalent to 0
36-
Fail clamp(-1, 1 / sign(atan2(-0cap / 1px, 0em / 1px)), 1) should be used-value-equivalent to -1
36+
Pass clamp(-1, 1 / sign(atan2(-0cap / 1px, 0em / 1px)), 1) should be used-value-equivalent to -1
3737
Pass sign(exp(-1vh / 0px)) should be used-value-equivalent to 0
3838
Pass clamp(-1, 1 / sign(exp(-1vh / 0px)), 1) should be used-value-equivalent to 1
3939
Fail calc(20cqw / 1rem) should be used-value-equivalent to 2

0 commit comments

Comments
 (0)