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

LayoutTests/ChangeLog

@@ -1,3 +1,16 @@
+2021-04-16  Darin Adler  <darin@apple.com>
+
+        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.
+
+        * 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.
+
 2021-04-16  Ian Gilbert  <iang@apple.com>
 
         Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

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

@@ -8,32 +8,52 @@ PASS successfullyParsed is true
 TEST COMPLETE
 PASS innerWidth is 800
 PASS innerHeight is 600
-PASS getComputedStyle(test).fontSize is "30px"
 PASS getComputedStyle(test).width is "400px"
+PASS getComputedStyle(testfontsize).fontSize is "30px"
+PASS getComputedStyle(testcalc).width is "800px"
+PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
 PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
 PASS getComputedStyle(testpseudo, ':after').paddingRight is "200px"
+PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
+PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "400px"
 PASS innerWidth is 900
 PASS innerHeight is 600
-PASS getComputedStyle(test).fontSize is "30px"
 PASS getComputedStyle(test).width is "450px"
+PASS getComputedStyle(testfontsize).fontSize is "30px"
+PASS getComputedStyle(testcalc).width is "900px"
+PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
 PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
 PASS getComputedStyle(testpseudo, ':after').paddingRight is "225px"
+PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
+PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "450px"
 PASS innerWidth is 900
 PASS innerHeight is 640
-PASS getComputedStyle(test).fontSize is "32px"
 PASS getComputedStyle(test).width is "450px"
+PASS getComputedStyle(testfontsize).fontSize is "32px"
+PASS getComputedStyle(testcalc).width is "900px"
+PASS getComputedStyle(testfontsizecalc).fontSize is "64px"
 PASS getComputedStyle(testpseudo, ':after').marginLeft is "128px"
 PASS getComputedStyle(testpseudo, ':after').paddingRight is "225px"
+PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "256px"
+PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "450px"
 PASS innerWidth is 500
 PASS innerHeight is 640
-PASS getComputedStyle(test).fontSize is "32px"
 PASS getComputedStyle(test).width is "250px"
+PASS getComputedStyle(testfontsize).fontSize is "32px"
+PASS getComputedStyle(testcalc).width is "500px"
+PASS getComputedStyle(testfontsizecalc).fontSize is "64px"
 PASS getComputedStyle(testpseudo, ':after').marginLeft is "100px"
 PASS getComputedStyle(testpseudo, ':after').paddingRight is "160px"
+PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "200px"
+PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "320px"
 PASS innerWidth is 800
 PASS innerHeight is 600
-PASS getComputedStyle(test).fontSize is "30px"
 PASS getComputedStyle(test).width is "400px"
+PASS getComputedStyle(testfontsize).fontSize is "30px"
+PASS getComputedStyle(testcalc).width is "800px"
+PASS getComputedStyle(testfontsizecalc).fontSize is "60px"
 PASS getComputedStyle(testpseudo, ':after').marginLeft is "120px"
 PASS getComputedStyle(testpseudo, ':after').paddingRight is "200px"
+PASS getComputedStyle(testpseudocalc, ':after').marginLeft is "240px"
+PASS getComputedStyle(testpseudocalc, ':after').paddingRight is "400px"
 

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

@@ -1,18 +1,35 @@
 <!DOCTYPE html>
 <style>
 #test {
-    font-size: 5vh;
     width: 50vw;
 }
+#testfontsize {
+    font-size: 5vh;
+}
+#testcalc {
+    width: calc(50vw * 2);
+}
+#testfontsizecalc {
+    font-size: calc(5vh * 2);
+}
 #testpseudo:after {
     margin-left: 20vmin;
     padding-right: 25vmax;
     content: '';
 }
+#testpseudocalc:after {
+    margin-left: calc(20vmin * 2);
+    padding-right: calc(25vmax * 2);
+    content: '';
+}
 </style>
 <body>
     <div id="test"></div>
+    <div id="testfontsize"></div>
+    <div id="testcalc"></div>
+    <div id="testfontsizecalc"></div>
     <div id="testpseudo"></div>
+    <div id="testpseudocalc"></div>
 </body>
 <script src="../../resources/js-test-pre.js"></script>
 <script src="resources/resize-test.js"></script>
@@ -21,10 +38,14 @@
 standardResizeTest(function () {
     var min = Math.min(innerWidth, innerHeight);
     var max = Math.max(innerWidth, innerHeight);
-    shouldBeEqualToString("getComputedStyle(test).fontSize", innerHeight / 20 + "px");
     shouldBeEqualToString("getComputedStyle(test).width", innerWidth / 2 + "px");
+    shouldBeEqualToString("getComputedStyle(testfontsize).fontSize", innerHeight / 20 + "px");
+    shouldBeEqualToString("getComputedStyle(testcalc).width", innerWidth + "px");
+    shouldBeEqualToString("getComputedStyle(testfontsizecalc).fontSize", innerHeight / 10 + "px");
     shouldBeEqualToString("getComputedStyle(testpseudo, ':after').marginLeft", min / 5 + "px");
     shouldBeEqualToString("getComputedStyle(testpseudo, ':after').paddingRight", max / 4 + "px");
+    shouldBeEqualToString("getComputedStyle(testpseudocalc, ':after').marginLeft", (min * 2) / 5 + "px");
+    shouldBeEqualToString("getComputedStyle(testpseudocalc, ':after').paddingRight", max / 2 + "px");
 });
 </script>
 <script src="../../resources/js-test-post.js"></script>

Source/WebCore/ChangeLog

@@ -1,3 +1,35 @@
+2021-04-16  Darin Adler  <darin@apple.com>
+
+        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.
+
+        * 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.
+
 2021-04-16  Ian Gilbert  <iang@apple.com>
 
         Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

Source/WebCore/css/CSSToLengthConversionData.cpp

@@ -38,15 +38,15 @@ namespace WebCore {
 
 float CSSToLengthConversionData::zoom() const
 {
-    if (m_useEffectiveZoom)
+    if (!m_zoom)
         return m_style ? m_style->effectiveZoom() : 1;
-    return m_zoom;
+    return *m_zoom;
 }
 
 double CSSToLengthConversionData::viewportWidthFactor() const
 {
-    if (m_style && !computingFontSize())
-        const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
+    if (m_viewportDependencyDetectionStyle)
+        m_viewportDependencyDetectionStyle->setHasViewportUnits();
 
     if (!m_renderView)
         return 0;
@@ -56,8 +56,8 @@ double CSSToLengthConversionData::viewportWidthFactor() const
 
 double CSSToLengthConversionData::viewportHeightFactor() const
 {
-    if (m_style && !computingFontSize())
-        const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
+    if (m_viewportDependencyDetectionStyle)
+        m_viewportDependencyDetectionStyle->setHasViewportUnits();
 
     if (!m_renderView)
         return 0;
@@ -67,8 +67,8 @@ double CSSToLengthConversionData::viewportHeightFactor() const
 
 double CSSToLengthConversionData::viewportMinFactor() const
 {
-    if (m_style && !computingFontSize())
-        const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
+    if (m_viewportDependencyDetectionStyle)
+        m_viewportDependencyDetectionStyle->setHasViewportUnits();
 
     if (!m_renderView)
         return 0;
@@ -79,8 +79,8 @@ double CSSToLengthConversionData::viewportMinFactor() const
 
 double CSSToLengthConversionData::viewportMaxFactor() const
 {
-    if (m_style && !computingFontSize())
-        const_cast<RenderStyle*>(m_style)->setHasViewportUnits();
+    if (m_viewportDependencyDetectionStyle)
+        m_viewportDependencyDetectionStyle->setHasViewportUnits();
 
     if (!m_renderView)
         return 0;

Source/WebCore/css/CSSToLengthConversionData.h

@@ -41,13 +41,13 @@ class RenderView;
 
 class CSSToLengthConversionData {
 public:
-    CSSToLengthConversionData(const RenderStyle* style, const RenderStyle* rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float zoom, Optional<CSSPropertyID> propertyToCompute = WTF::nullopt)
+    CSSToLengthConversionData(const RenderStyle* style, const RenderStyle* rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float zoom, Optional<CSSPropertyID> propertyToCompute = WTF::nullopt, RenderStyle* viewportDependencyDetectionStyle = nullptr)
         : m_style(style)
         , m_rootStyle(rootStyle)
         , m_parentStyle(parentStyle)
+        , m_viewportDependencyDetectionStyle(viewportDependencyDetectionStyle ? viewportDependencyDetectionStyle : const_cast<RenderStyle*>(style))
         , m_renderView(renderView)
         , m_zoom(zoom)
-        , m_useEffectiveZoom(false)
         , m_propertyToCompute(propertyToCompute)
     {
         ASSERT(zoom > 0);
@@ -57,17 +57,13 @@ class CSSToLengthConversionData {
         : m_style(style)
         , m_rootStyle(rootStyle)
         , m_parentStyle(parentStyle)
+        , m_viewportDependencyDetectionStyle(const_cast<RenderStyle*>(style))
         , m_renderView(renderView)
-        , m_zoom(1)
-        , m_useEffectiveZoom(true)
         , m_propertyToCompute(propertyToCompute)
     {
     }
 
-    CSSToLengthConversionData()
-        : CSSToLengthConversionData(nullptr, nullptr, nullptr, nullptr)
-    {
-    }
+    CSSToLengthConversionData() = default;
 
     const RenderStyle* style() const { return m_style; }
     const RenderStyle* rootStyle() const { return m_rootStyle; }
@@ -94,12 +90,12 @@ class CSSToLengthConversionData {
     }
 
 private:
-    const RenderStyle* m_style;
-    const RenderStyle* m_rootStyle;
-    const RenderStyle* m_parentStyle;
-    const RenderView* m_renderView;
-    float m_zoom;
-    bool m_useEffectiveZoom;
+    const RenderStyle* m_style { nullptr };
+    const RenderStyle* m_rootStyle { nullptr };
+    const RenderStyle* m_parentStyle { nullptr };
+    RenderStyle* m_viewportDependencyDetectionStyle { nullptr };
+    const RenderView* m_renderView { nullptr };
+    Optional<float> m_zoom;
     Optional<CSSPropertyID> m_propertyToCompute;
 };
 

Source/WebCore/style/StyleBuilderCustom.h

@@ -1831,15 +1831,14 @@ inline void BuilderCustom::applyValueFontSize(BuilderState& builderState, CSSVal
     } else {
         fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize || !(primitiveValue.isPercentage() || primitiveValue.isFontRelativeLength()));
         if (primitiveValue.isLength()) {
-            size = primitiveValue.computeLength<float>(CSSToLengthConversionData(&builderState.parentStyle(), builderState.rootElementStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize));
-            if (primitiveValue.isViewportPercentageLength())
-                builderState.style().setHasViewportUnits();
+            CSSToLengthConversionData conversionData { &builderState.parentStyle(), builderState.rootElementStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize, &builderState.style() };
+            size = primitiveValue.computeLength<float>(conversionData);
         } else if (primitiveValue.isPercentage())
             size = (primitiveValue.floatValue() * parentSize) / 100.0f;
         else if (primitiveValue.isCalculatedPercentageWithLength()) {
-            const auto& conversionData = builderState.cssToLengthConversionData();
-            CSSToLengthConversionData parentConversionData { &builderState.parentStyle(), conversionData.rootStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize };
-            size = primitiveValue.cssCalcValue()->createCalculationValue(parentConversionData)->evaluate(parentSize);
+            // FIXME: Why does this need a different root style than the isLength case above?
+            CSSToLengthConversionData conversionData { &builderState.parentStyle(), builderState.cssToLengthConversionData().rootStyle(), &builderState.parentStyle(), builderState.document().renderView(), 1.0f, CSSPropertyFontSize, &builderState.style() };
+            size = primitiveValue.cssCalcValue()->createCalculationValue(conversionData)->evaluate(parentSize);
         } else
             return;
     }