Changeset 271435 in webkit

Timestamp:

Jan 13, 2021, 2:11:25 AM (6 years ago)

Author:

graouts@webkit.org

Message:

REGRESSION (r267571): black line appears upon navigating back from apple.com shopping bag
​https://bugs.webkit.org/show_bug.cgi?id=220550
<rdar://problem/72459816>

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Mark two additional PASS results for ::marker tests.

• web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt:

Source/WebCore:

Test: webanimations/no-transition-on-after-pseudo-element-upon-creation.html

In r267571, we refactored the code to use Styleable instead of Element in pseudo-element resolution code. While there
should have been no behavior change, there was a change in Style::TreeResolver::createAnimatedElementUpdate() that
mistakenly introduced one.

In order to get the "before" style to be used to consider CSS Transitions, we used to simply call Element::renderOrDisplayContentsStyle()
on the element provided to createAnimatedElementUpdate(), which would be either an Element or a PseudoElement in the
case of ::before and ::after. When we switched to using Styleable, we made a change where we'd call renderOrDisplayContentsStyle()
on the Styleable's element, if it didn't a pseudo-element, or try to get the matching PseudoElement in the case of
::before and ::after. However, if we got a nullptr RenderStyle in the PseudoElement case, we'd fall back to using the
style from the host element.

This yielded this regression on apple.com where a transition is started on an ::after pseudo-element which has an
"opacity: 0" style and a "transition" style set for "opacity". The host element is created first, and later the
::after pseudo-element added. While it should not consider starting a transition in this case since upon creation
there is no existing style to work with, it did start a transition since it would use the host element's style
and see "opacity: 1" to start a transition.

In this patch, we address the FIXME we'd left behind in TreeResolver::createAnimatedElementUpdate() and make
Element::renderOrDisplayContentsStyle() take in a PseudoId, defaulting to PseudoId::None. In case we have a
pseudo-element, we first try to call renderOrDisplayContentsStyle() on the matching PseudoElement if it exists,
or we return the existing computed style for this pseudo-element.

If there is no existing computed style, we return nullptr, which means that in the apple.com scenario, no transition
is started because we correctly don't have a "before" style to work within upon creation of the ::after pseudo-element.

• dom/Element.cpp:

(WebCore::beforeOrAfterPseudoElement):
(WebCore::Element::renderOrDisplayContentsStyle const):

• dom/Element.h:
• style/StyleTreeResolver.cpp:

(WebCore::Style::TreeResolver::createAnimatedElementUpdate):

LayoutTests:

Add a test that checks that adding a pseudo-element for an existing host element does not use
the host element's style to consider starting a transition.

• webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html: Added.
• webanimations/no-transition-on-after-pseudo-element-upon-creation.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt (modified) (2 diffs)
• LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html (added)
• LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Element.cpp (modified) (2 diffs)
• Source/WebCore/dom/Element.h (modified) (1 diff)
• Source/WebCore/style/StyleTreeResolver.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-01-12  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION (r267571): black line appears upon navigating back from apple.com shopping bag
+        https://bugs.webkit.org/show_bug.cgi?id=220550
+        <rdar://problem/72459816>
+
+        Reviewed by Antti Koivisto.
+
+        Add a test that checks that adding a pseudo-element for an existing host element does not use
+        the host element's style to consider starting a transition.
+
+        * webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html: Added.
+        * webanimations/no-transition-on-after-pseudo-element-upon-creation.html: Added.
+
 2021-01-12  Lauro Moura  <lmoura@igalia.com>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-01-12  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION (r267571): black line appears upon navigating back from apple.com shopping bag
+        https://bugs.webkit.org/show_bug.cgi?id=220550
+        <rdar://problem/72459816>
+
+        Reviewed by Antti Koivisto.
+
+        Mark two additional PASS results for ::marker tests.
+
+        * web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt:
+
 2021-01-13  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt

@@ -30 +30 @@
 PASS Animation of list-style-type in ::marker
 FAIL Animation of line-height in ::marker assert_equals: expected "normal" but got "35px"
-FAIL Transition of font in ::marker assert_in_array: value "italic small-caps 500 expanded 15px/35px Ahem" not in array ["italic small-caps 500 expanded 15px Ahem", "italic small-caps 500 expanded 15px/normal Ahem"]
+PASS Transition of font in ::marker
 PASS Transition of font-family in ::marker
 PASS Transition of font-feature-settings in ::marker
@@ -59 +59 @@
 PASS Transition of list-style-position in ::marker
 PASS Transition of list-style-type in ::marker
-FAIL Transition of line-height in ::marker assert_equals: expected "normal" but got "35px"
+PASS Transition of line-height in ::marker
 target

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-01-12  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION (r267571): black line appears upon navigating back from apple.com shopping bag
+        https://bugs.webkit.org/show_bug.cgi?id=220550
+        <rdar://problem/72459816>
+
+        Reviewed by Antti Koivisto.
+
+        Test: webanimations/no-transition-on-after-pseudo-element-upon-creation.html
+
+        In r267571, we refactored the code to use Styleable instead of Element in pseudo-element resolution code. While there
+        should have been no behavior change, there was a change in Style::TreeResolver::createAnimatedElementUpdate() that
+        mistakenly introduced one.
+
+        In order to get the "before" style to be used to consider CSS Transitions, we used to simply call Element::renderOrDisplayContentsStyle()
+        on the element provided to createAnimatedElementUpdate(), which would be either an Element or a PseudoElement in the
+        case of ::before and ::after. When we switched to using Styleable, we made a change where we'd call renderOrDisplayContentsStyle()
+        on the Styleable's element, if it didn't a pseudo-element, or try to get the matching PseudoElement in the case of
+        ::before and ::after. However, if we got a nullptr RenderStyle in the PseudoElement case, we'd fall back to using the
+        style from the host element.
+
+        This yielded this regression on apple.com where a transition is started on an ::after pseudo-element which has an
+        "opacity: 0" style and a "transition" style set for "opacity". The host element is created first, and later the
+        ::after pseudo-element added. While it should not consider starting a transition in this case since upon creation
+        there is no existing style to work with, it did start a transition since it would use the host element's style
+        and see "opacity: 1" to start a transition.
+
+        In this patch, we address the FIXME we'd left behind in TreeResolver::createAnimatedElementUpdate() and make
+        Element::renderOrDisplayContentsStyle() take in a PseudoId, defaulting to PseudoId::None. In case we have a
+        pseudo-element, we first try to call renderOrDisplayContentsStyle() on the matching PseudoElement if it exists,
+        or we return the existing computed style for this pseudo-element.
+
+        If there is no existing computed style, we return nullptr, which means that in the apple.com scenario, no transition
+        is started because we correctly don't have a "before" style to work within upon creation of the ::after pseudo-element.
+
+        * dom/Element.cpp:
+        (WebCore::beforeOrAfterPseudoElement):
+        (WebCore::Element::renderOrDisplayContentsStyle const):
+        * dom/Element.h:
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+
 2021-01-12  Cathie Chen  <cathiechen@igalia.com>
 

trunk/Source/WebCore/dom/Element.cpp

@@ -3294 +3294 @@
 }
 
-static PseudoElement* beforeOrAfterPseudoElement(Element& host, PseudoId pseudoElementSpecifier)
+static PseudoElement* beforeOrAfterPseudoElement(const Element& host, PseudoId pseudoElementSpecifier)
 {
     switch (pseudoElementSpecifier) {
@@ -3316 +3316 @@
 }
 
-const RenderStyle* Element::renderOrDisplayContentsStyle() const
-{
+const RenderStyle* Element::renderOrDisplayContentsStyle(PseudoId pseudoId) const
+{
+    if (pseudoId != PseudoId::None) {
+        if (auto* pseudoElement = beforeOrAfterPseudoElement(*this, pseudoId))
+            return pseudoElement->renderOrDisplayContentsStyle();
+
+        if (auto* computedStyle = existingComputedStyle()) {
+            if (auto* cachedPseudoStyle = computedStyle->getCachedPseudoStyle(pseudoId))
+                return cachedPseudoStyle;
+        }
+
+        return nullptr;
+    }
+
     if (auto* style = renderStyle())
         return style;

trunk/Source/WebCore/dom/Element.h

@@ -553 +553 @@
 
     const RenderStyle* existingComputedStyle() const;
-    WEBCORE_EXPORT const RenderStyle* renderOrDisplayContentsStyle() const;
+    WEBCORE_EXPORT const RenderStyle* renderOrDisplayContentsStyle(PseudoId = PseudoId::None) const;
 
     void clearBeforePseudoElement();

trunk/Source/WebCore/style/StyleTreeResolver.cpp

@@ -317 +317 @@
 {
     auto& element = styleable.element;
-
-    // FIXME: Ideally we could just call Element::renderOrDisplayContentsStyle() with a PseudoId
-    // and get the style for any PseudoId, not just PseudoId::Before or PseudoId::After.
-    auto* pseudoElement = [styleable]() -> PseudoElement* {
-        switch (styleable.pseudoId) {
-        case PseudoId::Before:
-            return styleable.element.beforePseudoElement();
-        case PseudoId::After:
-            return styleable.element.afterPseudoElement();
-        default:
-            return nullptr;
-        }
-    }();
-    auto* oldStyle = pseudoElement ? pseudoElement->renderOrDisplayContentsStyle() : element.renderOrDisplayContentsStyle();
+    auto* oldStyle = element.renderOrDisplayContentsStyle(styleable.pseudoId);
 
     OptionSet<AnimationImpact> animationImpact;