Style invalidation affecting siblings does not work with inline-style…

… changes https://bugs.webkit.org/show_bug.cgi?id=149189 Patch by Benjamin Poulain <bpoulain@apple.com> on 2015-09-15 Reviewed by Antti Koivisto. Source/WebCore: Style::resolveTree() made the assumption that inline style changes only affect descendants and should not participate in "StyleRecalcAffectsNextSiblingElementStyle". That was wrong. If the inline style change through CSSOM, it can cause the creation of a style attribute, which is observable through "StyleRecalcAffectsNextSiblingElementStyle". This patch removes the incorrect assumption. Style invalidation is always propagated now. Tests: fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html * css/PropertySetCSSStyleDeclaration.cpp: (WebCore::InlineCSSStyleDeclaration::didMutate): Deleted. * dom/StyledElement.cpp: (WebCore::StyledElement::inlineStyleChanged): * dom/StyledElement.h: (WebCore::StyledElement::invalidateStyleAttribute): Clean up inline-style invalidation a tiny bit. * style/StyleResolveTree.cpp: (WebCore::Style::resolveTree): Fix the bug. LayoutTests: * fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt: Added. * fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html: Added. * fast/css/style-attribute-invalidation-propagates-to-direct-siblings-expected.txt: Added. * fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html: Added. * fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt: Added. * fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html: Added. Canonical link: https://commits.webkit.org/167293@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189836 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Sep 16, 2015 · 14a298f · 14a298f
1 parent 9f99388
commit 14a298f
Show file tree

Hide file tree

Showing 12 changed files with 367 additions and 5 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2015-09-15  Benjamin Poulain  <bpoulain@apple.com>
+
+        Style invalidation affecting siblings does not work with inline-style changes
+        https://bugs.webkit.org/show_bug.cgi?id=149189
+
+        Reviewed by Antti Koivisto.
+
+        * fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt: Added.
+        * fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html: Added.
+        * fast/css/style-attribute-invalidation-propagates-to-direct-siblings-expected.txt: Added.
+        * fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html: Added.
+        * fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt: Added.
+        * fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html: Added.
+
 2015-09-15  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Nested isolates can cause an infinite loop when laying out bidi runs

diff --git a/...utTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt b/...utTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt
@@ -0,0 +1,19 @@
+Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html
@@ -0,0 +1,91 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+target {
+    color: black;
+}
+target:nth-child(2 of target, trigger[style]) {
+    color: pink;
+}
+</style>
+</head>
+<body>
+    <div>
+        <!-- With renderer -->
+        <trigger></trigger>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <target></target>
+    </div>
+    <div style="display:none;">
+        <!-- Without renderer -->
+        <trigger></trigger>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <target></target>
+    </div>
+</body>
+<script>
+
+description('Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.');
+
+function testTargetColor(expectedColor) {
+    let allTargets = document.querySelectorAll("target");
+    for (let i = 0; i < allTargets.length; ++i) {
+        shouldBeEqualToString('getComputedStyle(document.querySelectorAll("target")[' + i + ']).color', expectedColor);
+    }
+}
+
+testTargetColor('rgb(0, 0, 0)');
+
+// Set a style through CSS OM.
+function setPropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.cursor = "auto";
+    }
+}
+setPropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+
+// If we remove the attribute, we should be back to normal.
+function removeStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].removeAttribute("style");
+    }
+}
+removeStyleAttribute();
+testTargetColor('rgb(0, 0, 0)');
+
+// Setting the style through a style attribute directly.
+function setStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].setAttribute("style", "alt: \"WebKit!\"");
+    }
+}
+setStyleAttribute();
+testTargetColor('rgb(255, 192, 203)');
+
+// Removing the property on the style attribute through CSSOM.
+// This leaves an empty style attribute.
+function removePropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.removeProperty("alt");
+    }
+}
+removePropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</html>
diff --git a/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-direct-siblings-expected.txt b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-direct-siblings-expected.txt
@@ -0,0 +1,19 @@
+Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html
@@ -0,0 +1,81 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+target {
+    color: black;
+}
+trigger[style] + target {
+    color: pink;
+}
+</style>
+</head>
+<body>
+    <div>
+        <!-- With renderer -->
+        <trigger></trigger>
+        <target></target>
+    </div>
+    <div style="display:none;">
+        <!-- Without renderer -->
+        <trigger></trigger>
+        <target></target>
+    </div>
+</body>
+<script>
+
+description('Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.');
+
+function testTargetColor(expectedColor) {
+    let allTargets = document.querySelectorAll("target");
+    for (let i = 0; i < allTargets.length; ++i) {
+        shouldBeEqualToString('getComputedStyle(document.querySelectorAll("target")[' + i + ']).color', expectedColor);
+    }
+}
+
+testTargetColor('rgb(0, 0, 0)');
+
+// Set a style through CSS OM.
+function setPropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.cursor = "auto";
+    }
+}
+setPropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+
+// If we remove the attribute, we should be back to normal.
+function removeStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].removeAttribute("style");
+    }
+}
+removeStyleAttribute();
+testTargetColor('rgb(0, 0, 0)');
+
+// Setting the style through a style attribute directly.
+function setStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].setAttribute("style", "alt: \"WebKit!\"");
+    }
+}
+setStyleAttribute();
+testTargetColor('rgb(255, 192, 203)');
+
+// Removing the property on the style attribute through CSSOM.
+// This leaves an empty style attribute.
+function removePropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.removeProperty("alt");
+    }
+}
+removePropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</html>
diff --git a/...tTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt b/...tTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt
@@ -0,0 +1,19 @@
+Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(0, 0, 0)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[0]).color is "rgb(255, 192, 203)"
+PASS getComputedStyle(document.querySelectorAll("target")[1]).color is "rgb(255, 192, 203)"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html
@@ -0,0 +1,91 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+target {
+    color: black;
+}
+trigger[style] ~ target {
+    color: pink;
+}
+</style>
+</head>
+<body>
+    <div>
+        <!-- With renderer -->
+        <trigger></trigger>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <target></target>
+    </div>
+    <div style="display:none;">
+        <!-- Without renderer -->
+        <trigger></trigger>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <padding></padding>
+        <target></target>
+    </div>
+</body>
+<script>
+
+description('Test that style invalidation through inline-style propagates correctly to siblings when a combinator requires it.');
+
+function testTargetColor(expectedColor) {
+    let allTargets = document.querySelectorAll("target");
+    for (let i = 0; i < allTargets.length; ++i) {
+        shouldBeEqualToString('getComputedStyle(document.querySelectorAll("target")[' + i + ']).color', expectedColor);
+    }
+}
+
+testTargetColor('rgb(0, 0, 0)');
+
+// Set a style through CSS OM.
+function setPropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.cursor = "auto";
+    }
+}
+setPropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+
+// If we remove the attribute, we should be back to normal.
+function removeStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].removeAttribute("style");
+    }
+}
+removeStyleAttribute();
+testTargetColor('rgb(0, 0, 0)');
+
+// Setting the style through a style attribute directly.
+function setStyleAttribute() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].setAttribute("style", "alt: \"WebKit!\"");
+    }
+}
+setStyleAttribute();
+testTargetColor('rgb(255, 192, 203)');
+
+// Removing the property on the style attribute through CSSOM.
+// This leaves an empty style attribute.
+function removePropertyWithCSSOM() {
+    let allTriggers = document.querySelectorAll("trigger");
+    for (let i = 0; i < allTriggers.length; ++i) {
+        allTriggers[i].style.removeProperty("alt");
+    }
+}
+removePropertyWithCSSOM()
+testTargetColor('rgb(255, 192, 203)');
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,33 @@
+2015-09-15  Benjamin Poulain  <bpoulain@apple.com>
+
+        Style invalidation affecting siblings does not work with inline-style changes
+        https://bugs.webkit.org/show_bug.cgi?id=149189
+
+        Reviewed by Antti Koivisto.
+
+        Style::resolveTree() made the assumption that inline style changes only affect
+        descendants and should not participate in "StyleRecalcAffectsNextSiblingElementStyle".
+        That was wrong. If the inline style change through CSSOM, it can cause the creation
+        of a style attribute, which is observable through "StyleRecalcAffectsNextSiblingElementStyle".
+
+        This patch removes the incorrect assumption. Style invalidation is always propagated now.
+
+        Tests: fast/css/style-attribute-invalidation-propagates-to-counted-siblings.html
+               fast/css/style-attribute-invalidation-propagates-to-direct-siblings.html
+               fast/css/style-attribute-invalidation-propagates-to-indirect-siblings.html
+
+        * css/PropertySetCSSStyleDeclaration.cpp:
+        (WebCore::InlineCSSStyleDeclaration::didMutate): Deleted.
+        * dom/StyledElement.cpp:
+        (WebCore::StyledElement::inlineStyleChanged):
+        * dom/StyledElement.h:
+        (WebCore::StyledElement::invalidateStyleAttribute):
+        Clean up inline-style invalidation a tiny bit.
+
+        * style/StyleResolveTree.cpp:
+        (WebCore::Style::resolveTree):
+        Fix the bug.
+
 2015-09-15  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Paused Debugger prevents page reload

diff --git a/Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp b/Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp
@@ -365,7 +365,6 @@ void InlineCSSStyleDeclaration::didMutate(MutationType type)
     if (!m_parentElement)
         return;
 
-    m_parentElement->setNeedsStyleRecalc(InlineStyleChange);
     m_parentElement->invalidateStyleAttribute();
     StyleAttributeMutationScope(this).didInvalidateStyleAttr();
 }

diff --git a/Source/WebCore/dom/StyledElement.cpp b/Source/WebCore/dom/StyledElement.cpp
@@ -214,9 +214,7 @@ void StyledElement::styleAttributeChanged(const AtomicString& newStyleString, At
 
 void StyledElement::inlineStyleChanged()
 {
-    setNeedsStyleRecalc(InlineStyleChange);
-    ASSERT(elementData());
-    elementData()->setStyleAttributeIsDirty(true);
+    invalidateStyleAttribute();
     InspectorInstrumentation::didInvalidateStyleAttr(document(), *this);
 }