- Revision
- 271435
- Author
- [email protected]
- Date
- 2021-01-13 02:11:25 -0800 (Wed, 13 Jan 2021)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (271434 => 271435)
--- trunk/LayoutTests/ChangeLog 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/LayoutTests/ChangeLog 2021-01-13 10:11:25 UTC (rev 271435)
@@ -1,3 +1,17 @@
+2021-01-12 Antoine Quint <[email protected]>
+
+ 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 <[email protected]>
[WPE] Gardening Focus-visible failures
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (271434 => 271435)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-01-13 10:11:25 UTC (rev 271435)
@@ -1,3 +1,15 @@
+2021-01-12 Antoine Quint <[email protected]>
+
+ 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 <[email protected]>
Use promise_test in WPT beacon/beacon-common.sub.js instead of async_test
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt (271434 => 271435)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-in-animation-expected.txt 2021-01-13 10:11:25 UTC (rev 271435)
@@ -29,7 +29,7 @@
PASS Animation of list-style-position in ::marker
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
FAIL Transition of font-kerning in ::marker assert_equals: expected "none" but got "auto"
@@ -58,5 +58,5 @@
PASS Transition of list-style-image in ::marker
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
Added: trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html (0 => 271435)
--- trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html (rev 0)
+++ trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation-expected.html 2021-01-13 10:11:25 UTC (rev 271435)
@@ -0,0 +1 @@
+<div style="width: 100px; height: 100px; background-color: green"></div>
\ No newline at end of file
Added: trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation.html (0 => 271435)
--- trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation.html (rev 0)
+++ trunk/LayoutTests/webanimations/no-transition-on-after-pseudo-element-upon-creation.html 2021-01-13 10:11:25 UTC (rev 271435)
@@ -0,0 +1,32 @@
+<body>
+<style>
+
+ div {
+ width: 100px;
+ height: 100px;
+ background-color: green;
+ }
+
+ div.after::after {
+ content: "";
+ display: block;
+ width: 100px;
+ height: 100px;
+ opacity: 0;
+ background-color: red;
+ transition: opacity 60s;
+ }
+
+</style>
+<script>
+
+ // Adding the "after" class on the next frame will yield the ::after pseudo-element.
+ // This should not yield a CSS Transition since there was no previous pseudo-element
+ // style to animate from, although there was a bug in WebKit introduced in r267571
+ // where we would use the host element's current style to consider whether we should
+ // run a CSS Transition here.
+ const element = document.body.appendChild(document.createElement("div"));
+ requestAnimationFrame(() => element.classList.add("after"));
+
+</script>
+</body>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (271434 => 271435)
--- trunk/Source/WebCore/ChangeLog 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/Source/WebCore/ChangeLog 2021-01-13 10:11:25 UTC (rev 271435)
@@ -1,3 +1,45 @@
+2021-01-12 Antoine Quint <[email protected]>
+
+ 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 <[email protected]>
The rootBounds of IntersectionObserverEntry is not correct when {root:document}
Modified: trunk/Source/WebCore/dom/Element.cpp (271434 => 271435)
--- trunk/Source/WebCore/dom/Element.cpp 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/Source/WebCore/dom/Element.cpp 2021-01-13 10:11:25 UTC (rev 271435)
@@ -3293,7 +3293,7 @@
child.ancestorWillEnterFullscreen();
}
-static PseudoElement* beforeOrAfterPseudoElement(Element& host, PseudoId pseudoElementSpecifier)
+static PseudoElement* beforeOrAfterPseudoElement(const Element& host, PseudoId pseudoElementSpecifier)
{
switch (pseudoElementSpecifier) {
case PseudoId::Before:
@@ -3315,8 +3315,20 @@
return renderStyle();
}
-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;
Modified: trunk/Source/WebCore/dom/Element.h (271434 => 271435)
--- trunk/Source/WebCore/dom/Element.h 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/Source/WebCore/dom/Element.h 2021-01-13 10:11:25 UTC (rev 271435)
@@ -552,7 +552,7 @@
LayoutRect absoluteEventHandlerBounds(bool& includesFixedPositionElements) override;
const RenderStyle* existingComputedStyle() const;
- WEBCORE_EXPORT const RenderStyle* renderOrDisplayContentsStyle() const;
+ WEBCORE_EXPORT const RenderStyle* renderOrDisplayContentsStyle(PseudoId = PseudoId::None) const;
void clearBeforePseudoElement();
void clearAfterPseudoElement();
Modified: trunk/Source/WebCore/style/StyleTreeResolver.cpp (271434 => 271435)
--- trunk/Source/WebCore/style/StyleTreeResolver.cpp 2021-01-13 09:50:12 UTC (rev 271434)
+++ trunk/Source/WebCore/style/StyleTreeResolver.cpp 2021-01-13 10:11:25 UTC (rev 271435)
@@ -316,21 +316,8 @@
ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, const Styleable& styleable, Change parentChange)
{
auto& element = styleable.element;
+ auto* oldStyle = element.renderOrDisplayContentsStyle(styleable.pseudoId);
- // 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();
-
OptionSet<AnimationImpact> animationImpact;
// First, we need to make sure that any new CSS animation occuring on this element has a matching WebAnimation
