Skip to content

Commit 3d33c9b

Browse files
darinadlerdarinadler
committed
font-size with viewport units in calc() doesn't change when viewport resizes
https://bugs.webkit.org/show_bug.cgi?id=224614 Reviewed by Zalan Bujtas. Source/WebCore: * css/CSSToLengthConversionData.cpp: (WebCore::CSSToLengthConversionData::zoom const): Updated since m_zoom is now optional. We use effectiveZoom when m_zoom is not specified, which is the same semantic that was implemented before with a separate boolean. (WebCore::CSSToLengthConversionData::viewportWidthFactor const): When calling the setHasViewportUnits function as a side effect, use m_viewportDependencyDetectionStyle, rather than always using m_style. This lets us handle the font-size case correctly. Also removed the explicit computingFontSize check for the same reason. (WebCore::CSSToLengthConversionData::viewportHeightFactor const): Ditto. (WebCore::CSSToLengthConversionData::viewportMinFactor const): Ditto. (WebCore::CSSToLengthConversionData::viewportMaxFactor const): Ditto. * css/CSSToLengthConversionData.h: Added a new member, m_viewportDependencyDetectionStyle, which defaults to the same value as m_style. Also changed m_zoom to use Optional instead of a separate boolean and an ignored "must be 1.0" value. Initialized data members in the modern way, allowing us to use the default constructor. * style/StyleBuilderCustom.h: (WebCore::Style::BuilderCustom::applyValueFontSize): Pass in the builder's style as the viewportDependencyDetectionStyle. This does the same thing that the existing code to call setHasViewportUnits did directly, but does it even for more complex cases involving calc(). Also made the isLength and isCalculatedPercentageWithLength cases more similar to each other and left a FIXME behind about taking that a bit further, but doing that probably requires creating some more test cases. LayoutTests: * css3/viewport-percentage-lengths/viewport-percentage-lengths-resize-expected.txt: * css3/viewport-percentage-lengths/viewport-percentage-lengths-resize.html: Added tests that involve calc, and broke rules up into multiple elements so that side effects from one style won't give us false negatives. This now has a subtest that was failing without the fix in this patch. Canonical link: https://commits.webkit.org/236669@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@276187 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 298b8d2 commit 3d33c9b

7 files changed

Lines changed: 118 additions & 37 deletions

File tree

‎LayoutTests/ChangeLog‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,16 @@
1+
2021-04-16 Darin Adler <darin@apple.com>
2+
3+
font-size with viewport units in calc() doesn't change when viewport resizes
4+
https://bugs.webkit.org/show_bug.cgi?id=224614
5+
6+
Reviewed by Zalan Bujtas.
7+
8+
* css3/viewport-percentage-lengths/viewport-percentage-lengths-resize-expected.txt:
9+
* css3/viewport-percentage-lengths/viewport-percentage-lengths-resize.html:
10+
Added tests that involve calc, and broke rules up into multiple elements so that side
11+
effects from one style won't give us false negatives. This now has a subtest that was
12+
failing without the fix in this patch.
13+
114
2021-04-16 Ian Gilbert <iang@apple.com>
215

316
Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

‎LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize-expected.txt‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,32 +8,52 @@ PASS successfullyParsed is true
88
TEST COMPLETE
99
PASS innerWidth is 800
1010
PASS innerHeight is 600
11-
PASS getComputedStyle(test).fontSize is "30px"
1211
PASS getComputedStyle(test).width is "400px"
12+
PASS getComputedStyle(testfontsize).fontSize is "30px"
13+
PASS getComputedStyle(testcalc).width is "800px"
14+
PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
1315
PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
1416
PASS getComputedStyle(testpseudo, ':after').paddingRight is "200px"
17+
PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
18+
PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "400px"
1519
PASS innerWidth is 900
1620
PASS innerHeight is 600
17-
PASS getComputedStyle(test).fontSize is "30px"
1821
PASS getComputedStyle(test).width is "450px"
22+
PASS getComputedStyle(testfontsize).fontSize is "30px"
23+
PASS getComputedStyle(testcalc).width is "900px"
24+
PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
1925
PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
2026
PASS getComputedStyle(testpseudo, ':after').paddingRight is "225px"
27+
PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
28+
PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "450px"
2129
PASS innerWidth is 900
2230
PASS innerHeight is 640
23-
PASS getComputedStyle(test).fontSize is "32px"
2431
PASS getComputedStyle(test).width is "450px"
32+
PASS getComputedStyle(testfontsize).fontSize is "32px"
33+
PASS getComputedStyle(testcalc).width is "900px"
34+
PASS getComputedStyle(testfontsizecalc).fontSize is "64px"
2535
PASS getComputedStyle(testpseudo, ':after').marginLeft is "128px"
2636
PASS getComputedStyle(testpseudo, ':after').paddingRight is "225px"
37+
PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "256px"
38+
PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "450px"
2739
PASS innerWidth is 500
2840
PASS innerHeight is 640
29-
PASS getComputedStyle(test).fontSize is "32px"
3041
PASS getComputedStyle(test).width is "250px"
42+
PASS getComputedStyle(testfontsize).fontSize is "32px"
43+
PASS getComputedStyle(testcalc).width is "500px"
44+
PASS getComputedStyle(testfontsizecalc).fontSize is "64px"
3145
PASS getComputedStyle(testpseudo, ':after').marginLeft is "100px"
3246
PASS getComputedStyle(testpseudo, ':after').paddingRight is "160px"
47+
PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "200px"
48+
PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "320px"
3349
PASS innerWidth is 800
3450
PASS innerHeight is 600
35-
PASS getComputedStyle(test).fontSize is "30px"
3651
PASS getComputedStyle(test).width is "400px"
52+
PASS getComputedStyle(testfontsize).fontSize is "30px"
53+
PASS getComputedStyle(testcalc).width is "800px"
54+
PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
3755
PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
3856
PASS getComputedStyle(testpseudo, ':after').paddingRight is "200px"
57+
PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
58+
PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "400px"
3959

‎LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize.html‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,35 @@
11
<!DOCTYPE html>
22
<style>
33
#test {
4-
font-size: 5vh;
54
width: 50vw;
65
}
6+
#testfontsize {
7+
font-size: 5vh;
8+
}
9+
#testcalc {
10+
width: calc(50vw * 2);
11+
}
12+
#testfontsizecalc {
13+
font-size: calc(5vh * 2);
14+
}
715
#testpseudo:after {
816
margin-left: 20vmin;
917
padding-right: 25vmax;
1018
content: '';
1119
}
20+
#testpseudocalc:after {
21+
margin-left: calc(20vmin * 2);
22+
padding-right: calc(25vmax * 2);
23+
content: '';
24+
}
1225
</style>
1326
<body>
1427
<div id="test"></div>
28+
<div id="testfontsize"></div>
29+
<div id="testcalc"></div>
30+
<div id="testfontsizecalc"></div>
1531
<div id="testpseudo"></div>
32+
<div id="testpseudocalc"></div>
1633
</body>
1734
<script src="../../resources/js-test-pre.js"></script>
1835
<script src="resources/resize-test.js"></script>
@@ -21,10 +38,14 @@
2138
standardResizeTest(function () {
2239
var min = Math.min(innerWidth, innerHeight);
2340
var max = Math.max(innerWidth, innerHeight);
24-
shouldBeEqualToString("getComputedStyle(test).fontSize", innerHeight / 20 + "px");
2541
shouldBeEqualToString("getComputedStyle(test).width", innerWidth / 2 + "px");
42+
shouldBeEqualToString("getComputedStyle(testfontsize).fontSize", innerHeight / 20 + "px");
43+
shouldBeEqualToString("getComputedStyle(testcalc).width", innerWidth + "px");
44+
shouldBeEqualToString("getComputedStyle(testfontsizecalc).fontSize", innerHeight / 10 + "px");
2645
shouldBeEqualToString("getComputedStyle(testpseudo, ':after').marginLeft", min / 5 + "px");
2746
shouldBeEqualToString("getComputedStyle(testpseudo, ':after').paddingRight", max / 4 + "px");
47+
shouldBeEqualToString("getComputedStyle(testpseudocalc, ':after').marginLeft", (min * 2) / 5 + "px");
48+
shouldBeEqualToString("getComputedStyle(testpseudocalc, ':after').paddingRight", max / 2 + "px");
2849
});
2950
</script>
3051
<script src="../../resources/js-test-post.js"></script>

‎Source/WebCore/ChangeLog‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,35 @@
1+
2021-04-16 Darin Adler <darin@apple.com>
2+
3+
font-size with viewport units in calc() doesn't change when viewport resizes
4+
https://bugs.webkit.org/show_bug.cgi?id=224614
5+
6+
Reviewed by Zalan Bujtas.
7+
8+
* css/CSSToLengthConversionData.cpp:
9+
(WebCore::CSSToLengthConversionData::zoom const): Updated since m_zoom is now optional.
10+
We use effectiveZoom when m_zoom is not specified, which is the same semantic that was
11+
implemented before with a separate boolean.
12+
(WebCore::CSSToLengthConversionData::viewportWidthFactor const): When calling the
13+
setHasViewportUnits function as a side effect, use m_viewportDependencyDetectionStyle,
14+
rather than always using m_style. This lets us handle the font-size case correctly.
15+
Also removed the explicit computingFontSize check for the same reason.
16+
(WebCore::CSSToLengthConversionData::viewportHeightFactor const): Ditto.
17+
(WebCore::CSSToLengthConversionData::viewportMinFactor const): Ditto.
18+
(WebCore::CSSToLengthConversionData::viewportMaxFactor const): Ditto.
19+
20+
* css/CSSToLengthConversionData.h: Added a new member, m_viewportDependencyDetectionStyle,
21+
which defaults to the same value as m_style. Also changed m_zoom to use Optional instead
22+
of a separate boolean and an ignored "must be 1.0" value. Initialized data members in
23+
the modern way, allowing us to use the default constructor.
24+
25+
* style/StyleBuilderCustom.h:
26+
(WebCore::Style::BuilderCustom::applyValueFontSize): Pass in the builder's style as the
27+
viewportDependencyDetectionStyle. This does the same thing that the existing code to
28+
call setHasViewportUnits did directly, but does it even for more complex cases involving
29+
calc(). Also made the isLength and isCalculatedPercentageWithLength cases more similar
30+
to each other and left a FIXME behind about taking that a bit further, but doing that
31+
probably requires creating some more test cases.
32+
133
2021-04-16 Ian Gilbert <iang@apple.com>
234

335
Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

‎Source/WebCore/css/CSSToLengthConversionData.cpp‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,15 +38,15 @@ namespace WebCore {
3838

3939
float CSSToLengthConversionData::zoom() const
4040
{
41-
if (m_useEffectiveZoom)
41+
if (!m_zoom)
4242
return m_style ? m_style->effectiveZoom() : 1;
43-
return m_zoom;
43+
return *m_zoom;
4444
}
4545

4646
double CSSToLengthConversionData::viewportWidthFactor() const
4747
{
48-
if (m_style && !computingFontSize())
49-
const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
48+
if (m_viewportDependencyDetectionStyle)
49+
m_viewportDependencyDetectionStyle->setHasViewportUnits();
5050

5151
if (!m_renderView)
5252
return 0;
@@ -56,8 +56,8 @@ double CSSToLengthConversionData::viewportWidthFactor() const
5656

5757
double CSSToLengthConversionData::viewportHeightFactor() const
5858
{
59-
if (m_style && !computingFontSize())
60-
const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
59+
if (m_viewportDependencyDetectionStyle)
60+
m_viewportDependencyDetectionStyle->setHasViewportUnits();
6161

6262
if (!m_renderView)
6363
return 0;
@@ -67,8 +67,8 @@ double CSSToLengthConversionData::viewportHeightFactor() const
6767

6868
double CSSToLengthConversionData::viewportMinFactor() const
6969
{
70-
if (m_style && !computingFontSize())
71-
const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
70+
if (m_viewportDependencyDetectionStyle)
71+
m_viewportDependencyDetectionStyle->setHasViewportUnits();
7272

7373
if (!m_renderView)
7474
return 0;
@@ -79,8 +79,8 @@ double CSSToLengthConversionData::viewportMinFactor() const
7979

8080
double CSSToLengthConversionData::viewportMaxFactor() const
8181
{
82-
if (m_style && !computingFontSize())
83-
const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
82+
if (m_viewportDependencyDetectionStyle)
83+
m_viewportDependencyDetectionStyle->setHasViewportUnits();
8484

8585
if (!m_renderView)
8686
return 0;

‎Source/WebCore/css/CSSToLengthConversionData.h‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,13 @@ class RenderView;
4141

4242
class CSSToLengthConversionData {
4343
public:
44-
CSSToLengthConversionData(const RenderStyle* style, const RenderStyle* rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float zoom, Optional<CSSPropertyID> propertyToCompute = WTF::nullopt)
44+
CSSToLengthConversionData(const RenderStyle* style, const RenderStyle* rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float zoom, Optional<CSSPropertyID> propertyToCompute = WTF::nullopt, RenderStyle* viewportDependencyDetectionStyle = nullptr)
4545
: m_style(style)
4646
, m_rootStyle(rootStyle)
4747
, m_parentStyle(parentStyle)
48+
, m_viewportDependencyDetectionStyle(viewportDependencyDetectionStyle ? viewportDependencyDetectionStyle : const_cast<RenderStyle*>(style))
4849
, m_renderView(renderView)
4950
, m_zoom(zoom)
50-
, m_useEffectiveZoom(false)
5151
, m_propertyToCompute(propertyToCompute)
5252
{
5353
ASSERT(zoom > 0);
@@ -57,17 +57,13 @@ class CSSToLengthConversionData {
5757
: m_style(style)
5858
, m_rootStyle(rootStyle)
5959
, m_parentStyle(parentStyle)
60+
, m_viewportDependencyDetectionStyle(const_cast<RenderStyle*>(style))
6061
, m_renderView(renderView)
61-
, m_zoom(1)
62-
, m_useEffectiveZoom(true)
6362
, m_propertyToCompute(propertyToCompute)
6463
{
6564
}
6665

67-
CSSToLengthConversionData()
68-
: CSSToLengthConversionData(nullptr, nullptr, nullptr, nullptr)
69-
{
70-
}
66+
CSSToLengthConversionData() = default;
7167

7268
const RenderStyle* style() const { return m_style; }
7369
const RenderStyle* rootStyle() const { return m_rootStyle; }
@@ -94,12 +90,12 @@ class CSSToLengthConversionData {
9490
}
9591

9692
private:
97-
const RenderStyle* m_style;
98-
const RenderStyle* m_rootStyle;
99-
const RenderStyle* m_parentStyle;
100-
const RenderView* m_renderView;
101-
float m_zoom;
102-
bool m_useEffectiveZoom;
93+
const RenderStyle* m_style { nullptr };
94+
const RenderStyle* m_rootStyle { nullptr };
95+
const RenderStyle* m_parentStyle { nullptr };
96+
RenderStyle* m_viewportDependencyDetectionStyle { nullptr };
97+
const RenderView* m_renderView { nullptr };
98+
Optional<float> m_zoom;
10399
Optional<CSSPropertyID> m_propertyToCompute;
104100
};
105101

‎Source/WebCore/style/StyleBuilderCustom.h‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1831,15 +1831,14 @@ inline void BuilderCustom::applyValueFontSize(BuilderState& builderState, CSSVal
18311831
} else {
18321832
fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize || !(primitiveValue.isPercentage() || primitiveValue.isFontRelativeLength()));
18331833
if (primitiveValue.isLength()) {
1834-
size = primitiveValue.computeLength<float>(CSSToLengthConversionData(&builderState.parentStyle(), builderState.rootElementStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize));
1835-
if (primitiveValue.isViewportPercentageLength())
1836-
builderState.style().setHasViewportUnits();
1834+
CSSToLengthConversionData conversionData { &builderState.parentStyle(), builderState.rootElementStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize, &builderState.style() };
1835+
size = primitiveValue.computeLength<float>(conversionData);
18371836
} else if (primitiveValue.isPercentage())
18381837
size = (primitiveValue.floatValue() * parentSize) / 100.0f;
18391838
else if (primitiveValue.isCalculatedPercentageWithLength()) {
1840-
const auto& conversionData = builderState.cssToLengthConversionData();
1841-
CSSToLengthConversionData parentConversionData { &builderState.parentStyle(), conversionData.rootStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize };
1842-
size = primitiveValue.cssCalcValue()->createCalculationValue(parentConversionData)->evaluate(parentSize);
1839+
// FIXME: Why does this need a different root style than the isLength case above?
1840+
CSSToLengthConversionData conversionData { &builderState.parentStyle(), builderState.cssToLengthConversionData().rootStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize, &builderState.style() };
1841+
size = primitiveValue.cssCalcValue()->createCalculationValue(conversionData)->evaluate(parentSize);
18431842
} else
18441843
return;
18451844
}

0 commit comments

Comments
 (0)