Wrong getComputedStyle behavior for pseudo-elements for layout-depend…

…ent 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:

Canonical link: https://commits.webkit.org/192901@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@221501 268f45cc-cd09-0410-ab3c-d52691b4dbfc

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-31  Sam Weinig  <sam@webkit.org>
 
         Implement DOMMatrix2DInit for setTransform()/addPath()

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 ""
 

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>

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  Per Arne Vollan  <pvollan@apple.com>
 
         [Win] Compile error, 'Cache' is not declared.

Source/WebCore/css/CSSComputedStyleDeclaration.cpp

@@ -2370,7 +2370,7 @@ static bool isLayoutDependent(CSSPropertyID propertyID, const RenderStyle* style
     }
 }
 
-Element* ComputedStyleExtractor::styledElement()
+Element* ComputedStyleExtractor::styledElement() const
 {
     if (!m_element)
         return nullptr;
@@ -2382,6 +2382,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.
@@ -2659,7 +2670,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();
@@ -2668,7 +2679,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;
@@ -2688,7 +2699,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID,
 
     if (!updateLayout || forceFullLayout) {
         style = computeRenderStyleForProperty(*styledElement, m_pseudoElementSpecifier, propertyID, ownedStyle);
-        renderer = styledElement->renderer();
+        renderer = styledRenderer();
     }
 
     if (!style)

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;