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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189836 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index b357973..2fdbe9c 100644 (file)
--- 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/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-counted-siblings-expected.txt
new file mode 100644 (file)
index 0000000..648d7f8
--- /dev/null
+++ b/LayoutTests/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
new file mode 100644 (file)
index 0000000..6604da8
--- /dev/null
+++ 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
new file mode 100644 (file)
index 0000000..648d7f8
--- /dev/null
+++ 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
new file mode 100644 (file)
index 0000000..c47f602
--- /dev/null
+++ 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/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt b/LayoutTests/fast/css/style-attribute-invalidation-propagates-to-indirect-siblings-expected.txt
new file mode 100644 (file)
index 0000000..648d7f8
--- /dev/null
+++ b/LayoutTests/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
new file mode 100644 (file)
index 0000000..08b2e20
--- /dev/null
+++ 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
index f7c3b8b..0bca84a 100644 (file)
--- 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
index 7fe8c04..0aa2318 100644 (file)
--- 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
index c12e456..5bee5fd 100644 (file)
--- 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);
 }
     

diff --git a/Source/WebCore/dom/StyledElement.h b/Source/WebCore/dom/StyledElement.h
index f8e0825..eb2be8b 100644 (file)
--- a/Source/WebCore/dom/StyledElement.h
+++ b/Source/WebCore/dom/StyledElement.h
@@ -97,6 +97,7 @@ inline void StyledElement::invalidateStyleAttribute()
 {
     ASSERT(elementData());
     elementData()->setStyleAttributeIsDirty(true);
+    setNeedsStyleRecalc(InlineStyleChange);
 }
 
 inline const StyleProperties* StyledElement::presentationAttributeStyle()

diff --git a/Source/WebCore/style/StyleResolveTree.cpp b/Source/WebCore/style/StyleResolveTree.cpp
index 2d85484..453a7b5 100644 (file)
--- a/Source/WebCore/style/StyleResolveTree.cpp
+++ b/Source/WebCore/style/StyleResolveTree.cpp
@@ -820,7 +820,7 @@ void resolveTree(Element& current, RenderStyle& inheritedStyle, RenderTreePositi
                 if (childElement.styleIsAffectedByPreviousSibling())
                     childElement.setNeedsStyleRecalc();
                 elementNeedingStyleRecalcAffectsNextSiblingElementStyle = childElement.affectsNextSiblingElementStyle();
-            } else if (childElement.styleChangeType() >= FullStyleChange)
+            } else if (childElement.needsStyleRecalc())
                 elementNeedingStyleRecalcAffectsNextSiblingElementStyle = childElement.affectsNextSiblingElementStyle();
             if (change >= Inherit || childElement.childNeedsStyleRecalc() || childElement.needsStyleRecalc()) {
                 parentPusher.push();