Changeset 198216 in webkit

Timestamp:

Mar 15, 2016 10:26:50 AM (8 years ago)

Author:

Antti Koivisto

Message:

Source/WebCore:
REGRESSION (196383): Class change invalidation does not handle :not correctly
​https://bugs.webkit.org/show_bug.cgi?id=155493
<rdar://problem/24846762>

Reviewed by Andreas Kling.

We fail to invalidate bar style in

:not(.foo) bar { }

when class foo is added or removed.

There is a logic error in the invalidation code. It assumes that class addition can only make new selectors match
and removal make them not match. This is not true when :not is present.

• style/AttributeChangeInvalidation.h:

(WebCore::Style::AttributeChangeInvalidation::AttributeChangeInvalidation):

• style/ClassChangeInvalidation.cpp:

(WebCore::Style::ClassChangeInvalidation::invalidateStyle):

Invalidate style and collect full set of rules that may affect descendant style.

(WebCore::Style::ClassChangeInvalidation::invalidateDescendantStyle):

Invalidate with this set both before and after committing the changes.

(WebCore::Style::ClassChangeInvalidation::computeClassChange): Deleted.

• style/ClassChangeInvalidation.h:

(WebCore::Style::ClassChangeInvalidation::ClassChangeInvalidation):
(WebCore::Style::ClassChangeInvalidation::~ClassChangeInvalidation):

LayoutTests:
Class change invalidation does not handle :not correctly
​https://bugs.webkit.org/show_bug.cgi?id=155493
<rdar://problem/24846762>

Reviewed by Andreas Kling.

• fast/css/style-invalidation-attribute-change-descendants-expected.txt:
• fast/css/style-invalidation-attribute-change-descendants.html:

Also add :not case for attribute changes (which handles this correctly already).

• fast/css/style-invalidation-class-change-descendants-expected.txt:
• fast/css/style-invalidation-class-change-descendants.html:

Add :not case.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/style-invalidation-attribute-change-descendants-expected.txt (modified) (2 diffs)
• LayoutTests/fast/css/style-invalidation-attribute-change-descendants.html (modified) (3 diffs)
• LayoutTests/fast/css/style-invalidation-class-change-descendants-expected.txt (modified) (2 diffs)
• LayoutTests/fast/css/style-invalidation-class-change-descendants.html (modified) (3 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/style/AttributeChangeInvalidation.h (modified) (1 diff)
• Source/WebCore/style/ClassChangeInvalidation.cpp (modified) (5 diffs)
• Source/WebCore/style/ClassChangeInvalidation.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-03-15  Antti Koivisto  <antti@apple.com>
+
+        Class change invalidation does not handle :not correctly
+        https://bugs.webkit.org/show_bug.cgi?id=155493
+        <rdar://problem/24846762>
+
+        Reviewed by Andreas Kling.
+
+        * fast/css/style-invalidation-attribute-change-descendants-expected.txt:
+        * fast/css/style-invalidation-attribute-change-descendants.html:
+
+            Also add :not case for attribute changes (which handles this correctly already).
+
+        * fast/css/style-invalidation-class-change-descendants-expected.txt:
+        * fast/css/style-invalidation-class-change-descendants.html:
+
+            Add :not case.
+
 2016-03-14  Jer Noble  <jer.noble@apple.com>
 

trunk/LayoutTests/fast/css/style-invalidation-attribute-change-descendants-expected.txt

@@ -4 +4 @@
 
 
+Setting attribute 'mynotattr' value ''
 PASS hasExpectedStyle is true
 PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
@@ -144 +145 @@
 PASS testStyleChangeType("inert", "NoStyleChange") is true
 PASS hasExpectedStyle is true
+Removing attribute 'mynotattr'
+PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
+PASS testStyleChangeType("target", "InlineStyleChange") is true
+PASS testStyleChangeType("inert", "NoStyleChange") is true
+PASS hasExpectedStyle is true
+Setting attribute 'mynotattr' value 'value'
+PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
+PASS testStyleChangeType("target", "InlineStyleChange") is true
+PASS testStyleChangeType("inert", "NoStyleChange") is true
+PASS hasExpectedStyle is true
 PASS successfullyParsed is true
 

trunk/LayoutTests/fast/css/style-invalidation-attribute-change-descendants.html

@@ -41 +41 @@
 [myattr2] > target {
     color: rgb(9, 0, 0);
+}
+
+root:not([mynotattr]) target {
+    color: rgb(10, 0, 0);
 }
 
@@ -126 +130 @@
 }
 
+setAttribute('mynotattr', '');
+
 checkStyle(0);
 testStyleInvalidation("NoStyleChange");
@@ -236 +242 @@
 setAttribute('myattr2', 'foo');
 testStyleInvalidation("NoStyleChange");
+checkStyle(1);
+
+removeAttribute('mynotattr');
+testStyleInvalidation("InlineStyleChange");
+checkStyle(10);
+
+setAttribute('mynotattr', 'value');
+testStyleInvalidation("InlineStyleChange");
 checkStyle(1);
 

trunk/LayoutTests/fast/css/style-invalidation-class-change-descendants-expected.txt

@@ -4 +4 @@
 
 
+Adding class style5
 PASS hasExpectedStyle is true
 PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
@@ -52 +53 @@
 PASS testStyleChangeType("inert", "NoStyleChange") is true
 PASS hasExpectedStyle is true
+Removing class style5
+PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
+PASS testStyleChangeType("target", "InlineStyleChange") is true
+PASS testStyleChangeType("inert", "NoStyleChange") is true
+PASS hasExpectedStyle is true
+Adding class style5
+PASS testStyleChangeType("root", "NoStyleChange") || testStyleChangeType("root", "InlineStyleChange") is true
+PASS testStyleChangeType("target", "InlineStyleChange") is true
+PASS testStyleChangeType("inert", "NoStyleChange") is true
+PASS hasExpectedStyle is true
 PASS successfullyParsed is true
 

trunk/LayoutTests/fast/css/style-invalidation-class-change-descendants.html

@@ -21 +21 @@
 root.style4 > target inert {
     color: rgb(4, 4, 4);
+}
+
+root:not(.style5) target {
+    color: rgb(5, 5, 5);
 }
 
@@ -106 +110 @@
 }
 
+addClass('style5');
+
 checkStyle(0);
 testStyleInvalidation("NoStyleChange");
@@ -144 +150 @@
 checkStyle(0);
 
+removeClass('style5');
+testStyleInvalidation("InlineStyleChange");
+checkStyle(5);
+
+addClass('style5');
+testStyleInvalidation("InlineStyleChange");
+checkStyle(0);
+
 </script>
 <script src="../../resources/js-test-post.js"></script>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-03-15  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (196383): Class change invalidation does not handle :not correctly
+        https://bugs.webkit.org/show_bug.cgi?id=155493
+        <rdar://problem/24846762>
+
+        Reviewed by Andreas Kling.
+
+        We fail to invalidate bar style in
+
+            :not(.foo) bar { }
+
+        when class foo is added or removed.
+
+        There is a logic error in the invalidation code. It assumes that class addition can only make new selectors match
+        and removal make them not match. This is not true when :not is present.
+
+        * style/AttributeChangeInvalidation.h:
+        (WebCore::Style::AttributeChangeInvalidation::AttributeChangeInvalidation):
+        * style/ClassChangeInvalidation.cpp:
+        (WebCore::Style::ClassChangeInvalidation::invalidateStyle):
+
+            Invalidate style and collect full set of rules that may affect descendant style.
+
+        (WebCore::Style::ClassChangeInvalidation::invalidateDescendantStyle):
+
+            Invalidate with this set both before and after committing the changes.
+
+        (WebCore::Style::ClassChangeInvalidation::computeClassChange): Deleted.
+        * style/ClassChangeInvalidation.h:
+        (WebCore::Style::ClassChangeInvalidation::ClassChangeInvalidation):
+        (WebCore::Style::ClassChangeInvalidation::~ClassChangeInvalidation):
+
 2016-03-14  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebCore/style/AttributeChangeInvalidation.h

@@ -47 +47 @@
     Element& m_element;
 
-    RuleSet* m_descendantInvalidationRuleSet { nullptr };
+    const RuleSet* m_descendantInvalidationRuleSet { nullptr };
 };
 

trunk/Source/WebCore/style/ClassChangeInvalidation.cpp

@@ -35 +35 @@
 namespace Style {
 
-auto ClassChangeInvalidation::collectClasses(const SpaceSplitString& classes) -> ClassChangeVector
+using ClassChangeVector = Vector<AtomicStringImpl*, 4>;
+
+static ClassChangeVector collectClasses(const SpaceSplitString& classes)
 {
     ClassChangeVector result;
@@ -44 +46 @@
 }
 
-void ClassChangeInvalidation::computeClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses)
+static ClassChangeVector computeClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses)
 {
     unsigned oldSize = oldClasses.size();
     unsigned newSize = newClasses.size();
 
-    if (!oldSize) {
-        m_addedClasses = collectClasses(newClasses);
-        return;
-    }
-    if (!newSize) {
-        m_removedClasses = collectClasses(oldClasses);
-        return;
-    }
+    if (!oldSize)
+        return collectClasses(newClasses);
+    if (!newSize)
+        return collectClasses(oldClasses);
+
+    ClassChangeVector changedClasses;
 
     BitVector remainingClassBits;
@@ -71 +71 @@
         if (foundFromBoth)
             continue;
-        m_addedClasses.append(newClasses[i].impl());
+        changedClasses.append(newClasses[i].impl());
     }
     for (unsigned i = 0; i < oldSize; ++i) {
@@ -77 +77 @@
         if (remainingClassBits.quickGet(i))
             continue;
-        m_removedClasses.append(oldClasses[i].impl());
+        changedClasses.append(oldClasses[i].impl());
     }
+
+    return changedClasses;
 }
 
-void ClassChangeInvalidation::invalidateStyle(const ClassChangeVector& changedClasses)
+void ClassChangeInvalidation::invalidateStyle(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses)
 {
+    auto changedClasses = computeClassChange(oldClasses, newClasses);
+
     auto& ruleSets = m_element.styleResolver().ruleSets();
 
-    Vector<AtomicStringImpl*, 4> changedClassesAffectingStyle;
+    ClassChangeVector changedClassesAffectingStyle;
     for (auto* changedClass : changedClasses) {
         if (ruleSets.features().classesInRules.contains(changedClass))
@@ -108 +112 @@
         if (!ancestorClassRules)
             continue;
+        m_descendantInvalidationRuleSets.append(ancestorClassRules);
+    }
+}
+
+void ClassChangeInvalidation::invalidateDescendantStyle()
+{
+    for (auto* ancestorClassRules : m_descendantInvalidationRuleSets) {
         StyleInvalidationAnalysis invalidationAnalysis(*ancestorClassRules);
         invalidationAnalysis.invalidateStyle(m_element);

trunk/Source/WebCore/style/ClassChangeInvalidation.h

@@ -45 +45 @@
 
 private:
-    using ClassChangeVector = Vector<AtomicStringImpl*, 4>;
-
-    void computeClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses);
-    void invalidateStyle(const ClassChangeVector&);
-
-    static ClassChangeVector collectClasses(const SpaceSplitString&);
+    void invalidateStyle(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses);
+    void invalidateDescendantStyle();
 
     const bool m_isEnabled;
     Element& m_element;
 
-    ClassChangeVector m_addedClasses;
-    ClassChangeVector m_removedClasses;
+    Vector<const RuleSet*, 4> m_descendantInvalidationRuleSets;
 };
 
@@ -66 +61 @@
     if (!m_isEnabled)
         return;
-    computeClassChange(oldClasses, newClasses);
-    invalidateStyle(m_removedClasses);
+    invalidateStyle(oldClasses, newClasses);
+    invalidateDescendantStyle();
 }
 
@@ -74 +69 @@
     if (!m_isEnabled)
         return;
-    invalidateStyle(m_addedClasses);
+    invalidateDescendantStyle();
 }