Merge r221501 - Wrong getComputedStyle behavior for pseudo-elements f…

…or layout-dependent properties. https://bugs.webkit.org/show_bug.cgi?id=175936 Patch by Emilio Cobos Álvarez <emilio@crisal.io> on 2017-09-01 Reviewed by Antti Koivisto. LayoutTests/imported/w3c: * web-platform-tests/cssom/getComputedStyle-pseudo-expected.txt: * web-platform-tests/cssom/getComputedStyle-pseudo.html: Sync test with upstream. Source/WebCore: Before this patch we may wrongly end up using the wrong renderer to resolve this. Tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html * css/CSSComputedStyleDeclaration.cpp: (WebCore::ComputedStyleExtractor::styledElement const): (WebCore::ComputedStyleExtractor::styledRenderer const): (WebCore::ComputedStyleExtractor::propertyValue): * css/CSSComputedStyleDeclaration.h:
WebKit · Sep 2, 2017 · 755a7f1 · 755a7f1
1 parent 114f3a0
commit 755a7f1
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 8 deletions.
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,13 @@
+2017-09-01  Emilio Cobos Álvarez  <emilio@crisal.io>
+
+        Wrong getComputedStyle behavior for pseudo-elements for layout-dependent properties.
+        https://bugs.webkit.org/show_bug.cgi?id=175936
+
+        Reviewed by Antti Koivisto.
+
+        * web-platform-tests/cssom/getComputedStyle-pseudo-expected.txt:
+        * web-platform-tests/cssom/getComputedStyle-pseudo.html: Sync test with upstream.
+
 2017-08-14  Chris Dumez  <cdumez@apple.com>
 
         XHR should only fire an abort event if the cancellation was requested by the client

diff --git a/LayoutTests/imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo-expected.txt
@@ -1,4 +1,6 @@
 
 PASS Resolution of width is correct for ::before and ::after pseudo-elements 
 FAIL Resolution of width is correct for ::before and ::after pseudo-elements of display: contents elements assert_equals: expected "50px" but got "auto"
+FAIL Resolution of nonexistent pseudo-element styles assert_equals: Nonexistent :before pseudo-element shouldn't claim to have the same style as the originating element expected "static" but got "relative"
+FAIL Resolution of pseudo-element styles in display: none elements assert_equals: Pseudo-styles of display: none elements should be correct expected "\"Foo\"" but got ""
 
diff --git a/LayoutTests/imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html b/LayoutTests/imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html
@@ -23,9 +23,17 @@
   height: 10px;
   display: block;
 }
+#none {
+  display: none;
+}
+#none::before,
+#none::after {
+  content: "Foo";
+}
 </style>
 <div id="test">
   <div id="contents"></div>
+  <div id="none"></div>
 </div>
 <script>
 test(function() {
@@ -40,4 +48,23 @@
     assert_equals(getComputedStyle(contents, pseudo).width, "50px");
   });
 }, "Resolution of width is correct for ::before and ::after pseudo-elements of display: contents elements");
+test(function() {
+  var has_no_pseudos = document.body;
+  has_no_pseudos.style.position = "relative";
+  [":before", ":after"].forEach(function(pseudo) {
+    assert_equals(getComputedStyle(has_no_pseudos, pseudo).position, "static",
+                  "Nonexistent " + pseudo + " pseudo-element shouldn't claim to have " +
+                  "the same style as the originating element");
+    assert_equals(getComputedStyle(has_no_pseudos, pseudo).width, "auto",
+                  "Nonexistent " + pseudo + " pseudo-element shouldn't claim to have " +
+                  "definite size");
+  });
+}, "Resolution of nonexistent pseudo-element styles");
+test(function() {
+  var none = document.getElementById('none');
+  [":before", ":after"].forEach(function(pseudo) {
+    assert_equals(getComputedStyle(none, pseudo).content, "\"Foo\"",
+                  "Pseudo-styles of display: none elements should be correct");
+  });
+}, "Resolution of pseudo-element styles in display: none elements");
 </script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,21 @@
+2017-09-01  Emilio Cobos Álvarez  <emilio@crisal.io>
+
+        Wrong getComputedStyle behavior for pseudo-elements for layout-dependent properties.
+        https://bugs.webkit.org/show_bug.cgi?id=175936
+
+        Reviewed by Antti Koivisto.
+
+        Before this patch we may wrongly end up using the wrong renderer to resolve
+        this.
+
+        Tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html
+
+        * css/CSSComputedStyleDeclaration.cpp:
+        (WebCore::ComputedStyleExtractor::styledElement const):
+        (WebCore::ComputedStyleExtractor::styledRenderer const):
+        (WebCore::ComputedStyleExtractor::propertyValue):
+        * css/CSSComputedStyleDeclaration.h:
+
 2017-09-01  Simon Fraser  <simon.fraser@apple.com>
 
         transformCanLikelyUseFastPath() can read off the end of a string

diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
@@ -2369,7 +2369,7 @@ static bool isLayoutDependent(CSSPropertyID propertyID, const RenderStyle* style
     }
 }
 
-Element* ComputedStyleExtractor::styledElement()
+Element* ComputedStyleExtractor::styledElement() const
 {
     if (!m_element)
         return nullptr;
@@ -2381,6 +2381,17 @@ Element* ComputedStyleExtractor::styledElement()
     return m_element.get();
 }
 
+
+RenderElement* ComputedStyleExtractor::styledRenderer() const
+{
+    auto* element = styledElement();
+    if (!element)
+        return nullptr;
+    if (m_pseudoElementSpecifier != NOPSEUDO && element == m_element.get())
+        return nullptr;
+    return element->renderer();
+}
+
 static bool isImplicitlyInheritedGridOrFlexProperty(CSSPropertyID propertyID)
 {
     // It would be nice if grid and flex worked within normal CSS mechanisms and not invented their own inheritance system.
@@ -2658,7 +2669,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID,
 
     std::unique_ptr<RenderStyle> ownedStyle;
     const RenderStyle* style = nullptr;
-    RenderObject* renderer = nullptr;
+    RenderElement* renderer = nullptr;
     bool forceFullLayout = false;
     if (updateLayout) {
         Document& document = m_element->document();
@@ -2667,7 +2678,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID,
             // Style update may change styledElement() to PseudoElement or back.
             styledElement = this->styledElement();
         }
-        renderer = styledElement->renderer();
+        renderer = styledRenderer();
 
         if (propertyID == CSSPropertyDisplay && !renderer && is<SVGElement>(*styledElement) && !downcast<SVGElement>(*styledElement).isValid())
             return nullptr;
@@ -2687,7 +2698,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID,
 
     if (!updateLayout || forceFullLayout) {
         style = computeRenderStyleForProperty(*styledElement, m_pseudoElementSpecifier, propertyID, ownedStyle);
-        renderer = styledElement->renderer();
+        renderer = styledRenderer();
     }
 
     if (!style)

diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.h b/Source/WebCore/css/CSSComputedStyleDeclaration.h
@@ -38,7 +38,7 @@ class FilterOperations;
 class FontSelectionValue;
 class MutableStyleProperties;
 class Node;
-class RenderObject;
+class RenderElement;
 class RenderStyle;
 class SVGPaint;
 class ShadowData;
@@ -75,9 +75,15 @@ class ComputedStyleExtractor {
     static Ref<CSSFontStyleValue> fontStyleFromStyleValue(FontSelectionValue, FontStyleAxis);
 
 private:
-    // The styled element is either the element passed into computedPropertyValue, or the
-    // PseudoElement for :before and :after if they exist.
-    Element* styledElement();
+    // The styled element is either the element passed into
+    // computedPropertyValue, or the PseudoElement for :before and :after if
+    // they exist.
+    Element* styledElement() const;
+
+    // The renderer we should use for resolving layout-dependent properties.
+    // Note that it differs from styledElement()->renderer() in the case we have
+    // no pseudo-element.
+    RenderElement* styledRenderer() const;
 
     RefPtr<CSSValue> svgPropertyValue(CSSPropertyID, EUpdateLayout);
     RefPtr<CSSValue> adjustSVGPaintForCurrentColor(SVGPaintType, const String& url, const Color&, const Color& currentColor) const;