Diff
Modified: trunk/LayoutTests/ChangeLog (227955 => 227956)
--- trunk/LayoutTests/ChangeLog 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/ChangeLog 2018-02-01 09:01:01 UTC (rev 227956)
@@ -1,3 +1,18 @@
+2018-02-01 Antti Koivisto <[email protected]>
+
+ Invalidate style for sibling combinators accurately on class change
+ https://bugs.webkit.org/show_bug.cgi?id=182336
+
+ Reviewed by Zalan Bujtas.
+
+ Adapt to progressions.
+
+ * fast/css/direct-adjacent-style-update-optimization-expected.txt:
+ * fast/css/direct-adjacent-style-update-optimization.html:
+ * fast/css/indirect-adjacent-style-update-optimization-expected.txt:
+ * fast/css/indirect-adjacent-style-update-optimization.html:
+ * fast/css/nth-last-child-of-style-update-optimization.html:
+
2018-01-31 Antoine Quint <[email protected]>
[Modern Media Controls] Turn media/modern-media-controls/macos-inline-media-controls back on
Modified: trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization-expected.txt (227955 => 227956)
--- trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization-expected.txt 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization-expected.txt 2018-02-01 09:01:01 UTC (rev 227956)
@@ -18,8 +18,8 @@
PASS getComputedStyle(document.querySelectorAll("target")[1]).backgroundColor is "rgb(1, 2, 3)"
PASS elementsWithTagnameNeedsStyleRecalc([]) is true
Let's remove the class "property".
-Source should require a style recalc.
-PASS elementsWithTagnameNeedsStyleRecalc(["SOURCE"]) is true
+Target should require a style recalc.
+PASS elementsWithTagnameNeedsStyleRecalc(["TARGET"]) is true
We should no longer have a match. Let's add back the class "property". Then we get the style of everything except <source> and <target>. Since none of the style touched above are related to the dirty elements, <source> should still require a style recalc.
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[1]).backgroundColor is "rgb(255, 254, 253)"
@@ -33,7 +33,7 @@
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[9]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll("target")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll("target")[1]).backgroundColor is "rgb(255, 254, 253)"
-PASS elementsWithTagnameNeedsStyleRecalc(["SOURCE"]) is true
+PASS elementsWithTagnameNeedsStyleRecalc(["TARGET"]) is true
We should match again now that property is back.
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[1]).backgroundColor is "rgb(255, 254, 253)"
Modified: trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization.html (227955 => 227956)
--- trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization.html 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/fast/css/direct-adjacent-style-update-optimization.html 2018-02-01 09:01:01 UTC (rev 227956)
@@ -63,8 +63,8 @@
for (var i = 0; i < allSources.length; ++i)
allSources[i].classList.remove("property");
-debug("Source should require a style recalc.");
-shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["SOURCE"])');
+debug("Target should require a style recalc.");
+shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["TARGET"])');
debug("We should no longer have a match. Let's add back the class \"property\". Then we get the style of everything except <source> and <target>. Since none of the style touched above are related to the dirty elements, <source> should still require a style recalc.");
@@ -82,7 +82,7 @@
debug("Something horribly wrong is happening.");
}
-shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["SOURCE"])');
+shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["TARGET"])');
debug("We should match again now that property is back.");
testColor(true);
Modified: trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization-expected.txt (227955 => 227956)
--- trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization-expected.txt 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization-expected.txt 2018-02-01 09:01:01 UTC (rev 227956)
@@ -18,8 +18,8 @@
PASS getComputedStyle(document.querySelectorAll("target")[1]).backgroundColor is "rgb(1, 2, 3)"
PASS elementsWithTagnameNeedsStyleRecalc([]) is true
Let's remove the class "property".
-Source should require a style recalc.
-PASS elementsWithTagnameNeedsStyleRecalc(["SOURCE"]) is true
+Target should require a style recalc.
+PASS elementsWithTagnameNeedsStyleRecalc(["TARGET"]) is true
We should no longer have a match. Let's add back the class "property". Then we get the style of everything except <source> and <target>. Since none of the style touched above are related to the dirty elements, <source> should still require a style recalc.
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[1]).backgroundColor is "rgb(255, 254, 253)"
@@ -33,7 +33,7 @@
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[9]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll("target")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll("target")[1]).backgroundColor is "rgb(255, 254, 253)"
-PASS elementsWithTagnameNeedsStyleRecalc(["SOURCE"]) is true
+PASS elementsWithTagnameNeedsStyleRecalc(["TARGET"]) is true
We should match again now that property is back.
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[0]).backgroundColor is "rgb(255, 254, 253)"
PASS getComputedStyle(document.querySelectorAll(".test-case, .test-case :not(target)")[1]).backgroundColor is "rgb(255, 254, 253)"
Modified: trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization.html (227955 => 227956)
--- trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization.html 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/fast/css/indirect-adjacent-style-update-optimization.html 2018-02-01 09:01:01 UTC (rev 227956)
@@ -63,8 +63,8 @@
for (var i = 0; i < allSources.length; ++i)
allSources[i].classList.remove("property");
-debug("Source should require a style recalc.");
-shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["SOURCE"])');
+debug("Target should require a style recalc.");
+shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["TARGET"])');
debug("We should no longer have a match. Let's add back the class \"property\". Then we get the style of everything except <source> and <target>. Since none of the style touched above are related to the dirty elements, <source> should still require a style recalc.");
@@ -82,7 +82,7 @@
debug("Something horribly wrong is happening.");
}
-shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["SOURCE"])');
+shouldBeTrue('elementsWithTagnameNeedsStyleRecalc(["TARGET"])');
debug("We should match again now that property is back.");
testColor(true);
Modified: trunk/LayoutTests/fast/css/nth-last-child-of-style-update-optimization.html (227955 => 227956)
--- trunk/LayoutTests/fast/css/nth-last-child-of-style-update-optimization.html 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/LayoutTests/fast/css/nth-last-child-of-style-update-optimization.html 2018-02-01 09:01:01 UTC (rev 227956)
@@ -51,8 +51,8 @@
for (var i = 0; i < allContent.length; ++i) {
var node = allContent[i];
if (internals.nodeNeedsStyleRecalc(node)) {
- if (node !== target1 && node !== target1.parentNode) {
- testFailed("Invalidating the style of target1 should never invalidate more than target1 and its parent.");
+ if (node !== target1 && node !== target1.previousElementSibling) {
+ testFailed("Invalidating the style of target1 should never invalidate more than target1 and its siblings.");
break;
}
}
@@ -68,8 +68,8 @@
for (var i = 0; i < allContent.length; ++i) {
var node = allContent[i];
if (internals.nodeNeedsStyleRecalc(node)) {
- if (node !== target2 && node !== target2.parentNode) {
- testFailed("Invalidating the style of target1 should never invalidate more than target1 and its parent.");
+ if (node !== target2 && node !== target2.previousElementSibling) {
+ testFailed("Invalidating the style of target2 should never invalidate more than target1 and its siblings.");
break;
}
}
Modified: trunk/Source/WebCore/ChangeLog (227955 => 227956)
--- trunk/Source/WebCore/ChangeLog 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/Source/WebCore/ChangeLog 2018-02-01 09:01:01 UTC (rev 227956)
@@ -1,3 +1,39 @@
+2018-02-01 Antti Koivisto <[email protected]>
+
+ Invalidate style for sibling combinators accurately on class change
+ https://bugs.webkit.org/show_bug.cgi?id=182336
+
+ Reviewed by Zalan Bujtas.
+
+ Use Style::Invalidator to invalidate only those elements that may be affected by a class
+ change for sibling combinators and nth pseudo classes.
+
+ * css/RuleFeature.cpp:
+
+ Add new AllSiblings MatchElement to use for nth pseudo classes with subselectors.
+
+ (WebCore::isSiblingOrSubject):
+
+ Add a helper.
+
+ (WebCore::RuleFeatureSet::computeNextMatchElement):
+ (WebCore::RuleFeatureSet::computeSubSelectorMatchElement):
+
+ Compute and propage MatchElement::AllSiblings.
+
+ * css/RuleFeature.h:
+ * dom/Node.cpp:
+ (WebCore::Node::updateAncestorsForStyleRecalc):
+
+ Don't need to test for childrenAffectedByPropertyBasedBackwardPositionalRules anymore (an oddly named bit for nth pseudo classes).
+
+ * style/StyleInvalidator.cpp:
+ (WebCore::Style::Invalidator::invalidateStyleWithMatchElement):
+
+ Invalidate only the potentially affected elements.
+ The old code would just unconditionally invalidate the current element. This would propagate to descedants of siblings via
+ affectedByPreviousSibling bits. That mechanism can be removed when everything has been switched to accurate invalidation.
+
2018-01-31 Simon Fraser <[email protected]>
Use different debug red colors for different contexts
Modified: trunk/Source/WebCore/css/RuleFeature.cpp (227955 => 227956)
--- trunk/Source/WebCore/css/RuleFeature.cpp 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/Source/WebCore/css/RuleFeature.cpp 2018-02-01 09:01:01 UTC (rev 227956)
@@ -35,9 +35,28 @@
namespace WebCore {
+static bool isSiblingOrSubject(MatchElement matchElement)
+{
+ switch (matchElement) {
+ case MatchElement::Subject:
+ case MatchElement::IndirectSibling:
+ case MatchElement::DirectSibling:
+ case MatchElement::AnySibling:
+ return true;
+ case MatchElement::Parent:
+ case MatchElement::Ancestor:
+ case MatchElement::ParentSibling:
+ case MatchElement::AncestorSibling:
+ case MatchElement::Host:
+ return false;
+ }
+ ASSERT_NOT_REACHED();
+ return false;
+}
+
MatchElement RuleFeatureSet::computeNextMatchElement(MatchElement matchElement, CSSSelector::RelationType relation)
{
- if (matchElement == MatchElement::Subject || matchElement == MatchElement::IndirectSibling || matchElement == MatchElement::DirectSibling) {
+ if (isSiblingOrSubject(matchElement)) {
switch (relation) {
case CSSSelector::Subselector:
return matchElement;
@@ -46,8 +65,12 @@
case CSSSelector::Child:
return MatchElement::Parent;
case CSSSelector::IndirectAdjacent:
+ if (matchElement == MatchElement::AnySibling)
+ return MatchElement::AnySibling;
return MatchElement::IndirectSibling;
case CSSSelector::DirectAdjacent:
+ if (matchElement == MatchElement::AnySibling)
+ return MatchElement::AnySibling;
return matchElement == MatchElement::Subject ? MatchElement::DirectSibling : MatchElement::IndirectSibling;
case CSSSelector::ShadowDescendant:
return MatchElement::Host;
@@ -76,9 +99,8 @@
if (selector.match() == CSSSelector::PseudoClass) {
auto type = selector.pseudoClassType();
// For :nth-child(n of .some-subselector) where an element change may affect other elements similar to sibling combinators.
- // FIXME: This is not entirely accurate but good enough for current users.
if (type == CSSSelector::PseudoClassNthChild || type == CSSSelector::PseudoClassNthLastChild)
- return MatchElement::IndirectSibling;
+ return MatchElement::AnySibling;
// Similarly for :host().
if (type == CSSSelector::PseudoClassHost)
Modified: trunk/Source/WebCore/css/RuleFeature.h (227955 => 227956)
--- trunk/Source/WebCore/css/RuleFeature.h 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/Source/WebCore/css/RuleFeature.h 2018-02-01 09:01:01 UTC (rev 227956)
@@ -33,7 +33,7 @@
class RuleData;
class StyleRule;
-enum class MatchElement { Subject, Parent, Ancestor, DirectSibling, IndirectSibling, ParentSibling, AncestorSibling, Host };
+enum class MatchElement { Subject, Parent, Ancestor, DirectSibling, IndirectSibling, AnySibling, ParentSibling, AncestorSibling, Host };
constexpr unsigned matchElementCount = static_cast<unsigned>(MatchElement::Host) + 1;
struct RuleFeature {
Modified: trunk/Source/WebCore/dom/Node.cpp (227955 => 227956)
--- trunk/Source/WebCore/dom/Node.cpp 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/Source/WebCore/dom/Node.cpp 2018-02-01 09:01:01 UTC (rev 227956)
@@ -774,9 +774,6 @@
if (it != end) {
it->setDirectChildNeedsStyleRecalc();
- if (it->childrenAffectedByPropertyBasedBackwardPositionalRules())
- it->adjustStyleValidity(Style::Validity::SubtreeInvalid, Style::InvalidationMode::Normal);
-
for (; it != end; ++it) {
// Iterator skips over shadow roots.
if (auto* shadowRoot = it->shadowRoot())
Modified: trunk/Source/WebCore/style/StyleInvalidator.cpp (227955 => 227956)
--- trunk/Source/WebCore/style/StyleInvalidator.cpp 2018-02-01 08:52:43 UTC (rev 227955)
+++ trunk/Source/WebCore/style/StyleInvalidator.cpp 2018-02-01 09:01:01 UTC (rev 227956)
@@ -227,11 +227,29 @@
break;
}
case MatchElement::DirectSibling:
+ if (auto* sibling = element.nextElementSibling())
+ invalidateIfNeeded(*sibling, nullptr);
+ break;
case MatchElement::IndirectSibling:
+ for (auto* sibling = element.nextElementSibling(); sibling; sibling = sibling->nextElementSibling())
+ invalidateIfNeeded(*sibling, nullptr);
+ break;
+ case MatchElement::AnySibling: {
+ auto parentChildren = childrenOfType<Element>(*element.parentNode());
+ for (auto& parentChild : parentChildren)
+ invalidateIfNeeded(parentChild, nullptr);
+ break;
+ }
case MatchElement::ParentSibling:
+ for (auto* sibling = element.nextElementSibling(); sibling; sibling = sibling->nextElementSibling()) {
+ auto siblingChildren = childrenOfType<Element>(*sibling);
+ for (auto& siblingChild : siblingChildren)
+ invalidateIfNeeded(siblingChild, nullptr);
+ }
+ break;
case MatchElement::AncestorSibling:
- // FIXME: Currently sibling invalidation happens during style resolution tree walk. It should be done here too.
- element.invalidateStyle();
+ for (auto* sibling = element.nextElementSibling(); sibling; sibling = sibling->nextElementSibling())
+ invalidateStyleForDescendants(*sibling, nullptr);
break;
case MatchElement::Host:
// FIXME: Handle this here as well.
