Changeset 203976 in webkit

Timestamp:

Aug 1, 2016 11:00:48 AM (8 years ago)

Author:

Antti Koivisto

Message:

REGRESSION (r196383): Drop down CSS menus not working on cnet.com, apmex.com
​https://bugs.webkit.org/show_bug.cgi?id=160390

Reviewed by Simon Fraser.

Source/WebCore:

The case here is that we have a rule like

.enableHover:hover .child { ... }

and the "enableHover" class is added dynamically. The class change invalidation optimization code would figure out
that nothing needs to be invalidated as the class change doesn't make the rule match (since :hover doesn't match).

However for event driven hover to actually work the hover element needs to have its childrenAffectedByHover bit set.
This bits is set when the selector match is attempted, whether it actually matches or not. Since we optimized away
the style invalidation we never set the bit either.

Fix by treating :hover as always matching (==ignored) when collecting rules for invalidation optimization purposes.
Dynamic pseudo elements are already treated this way for similar reasons.

Test: fast/selectors/hover-invalidation-descendant-dynamic.html

• css/SelectorChecker.cpp:

(WebCore::SelectorChecker::checkOne):

Match always in CollectingRulesIgnoringVirtualPseudoElements mode (now slightly misnamed).

This mode is used for optimization purposes in StyleInvalidationAnalysis (which we care about here) and
StyleSharingResolver. The change is fine for both.

• cssjit/SelectorCompiler.cpp:

(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsHovered):

Same change for the slow path selector checker.

LayoutTests:

• fast/selectors/hover-invalidation-descendant-dynamic-expected.txt: Added.
• fast/selectors/hover-invalidation-descendant-dynamic.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic-expected.txt (added)
• LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic.html (added)
• LayoutTests/platform/ios-simulator/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/css/SelectorChecker.cpp (modified) (1 diff)
• Source/WebCore/cssjit/SelectorCompiler.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-08-01  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r196383): Drop down CSS menus not working on cnet.com, apmex.com
+        https://bugs.webkit.org/show_bug.cgi?id=160390
+
+        Reviewed by Simon Fraser.
+
+        * fast/selectors/hover-invalidation-descendant-dynamic-expected.txt: Added.
+        * fast/selectors/hover-invalidation-descendant-dynamic.html: Added.
+
 2016-07-31  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/platform/ios-simulator/TestExpectations

@@ -289 +289 @@
 fast/css/pseudo-active-with-programmatic-focus.html [ Skip ]
 http/tests/security/history-username-password.html [ Skip ]
+fast/selectors/hover-invalidation-descendant-dynamic.html [ Skip ]
 
 webkit.org/b/148695 fast/shadow-dom [ Pass ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-08-01  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r196383): Drop down CSS menus not working on cnet.com, apmex.com
+        https://bugs.webkit.org/show_bug.cgi?id=160390
+
+        Reviewed by Simon Fraser.
+
+        The case here is that we have a rule like
+
+            .enableHover:hover .child { ... }
+
+        and the "enableHover" class is added dynamically. The class change invalidation optimization code would figure out
+        that nothing needs to be invalidated as the class change doesn't make the rule match (since :hover doesn't match).
+
+        However for event driven hover to actually work the hover element needs to have its childrenAffectedByHover bit set.
+        This bits is set when the selector match is attempted, whether it actually matches or not. Since we optimized away
+        the style invalidation we never set the bit either.
+
+        Fix by treating :hover as always matching (==ignored) when collecting rules for invalidation optimization purposes.
+        Dynamic pseudo elements are already treated this way for similar reasons.
+
+        Test: fast/selectors/hover-invalidation-descendant-dynamic.html
+
+        * css/SelectorChecker.cpp:
+        (WebCore::SelectorChecker::checkOne):
+
+            Match always in CollectingRulesIgnoringVirtualPseudoElements mode (now slightly misnamed).
+
+            This mode is used for optimization purposes in StyleInvalidationAnalysis (which we care about here) and
+            StyleSharingResolver. The change is fine for both.
+
+        * cssjit/SelectorCompiler.cpp:
+        (WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsHovered):
+
+            Same change for the slow path selector checker.
+
 2016-08-01  Darin Adler  <darin@apple.com>
 

trunk/Source/WebCore/css/SelectorChecker.cpp

@@ -951 +951 @@
             if (m_strictParsing || element.isLink() || canMatchHoverOrActiveInQuirksMode(context)) {
                 addStyleRelation(checkingContext, element, Style::Relation::AffectedByHover);
+
+                // See the comment in generateElementIsHovered() in SelectorCompiler.
+                if (checkingContext.resolvingMode == SelectorChecker::Mode::CollectingRulesIgnoringVirtualPseudoElements && !context.isMatchElement)
+                    return true;
 
                 if (element.hovered() || InspectorInstrumentation::forcePseudoState(element, CSSSelector::PseudoClassHover))

trunk/Source/WebCore/cssjit/SelectorCompiler.cpp

@@ -3215 +3215 @@
     generateAddStyleRelationIfResolvingStyle(elementAddressRegister, Style::Relation::AffectedByHover);
 
+    Assembler::JumpList successCases;
+    if (m_selectorContext != SelectorContext::QuerySelector && fragment.relationToRightFragment != FragmentRelation::Rightmost) {
+        // :hover always matches when not in rightmost position when collecting rules for descendant style invalidation optimization.
+        // Resolving style for a matching descendant will set parent childrenAffectedByHover bit even when the element is not currently hovered.
+        // This bit has to be set for the event based :hover invalidation to work.
+        // FIXME: We should just collect style relation bits and apply them as needed when computing style invalidation optimization.
+        LocalRegister checkingContext(m_registerAllocator);
+        successCases.append(branchOnResolvingMode(Assembler::Equal, SelectorChecker::Mode::CollectingRulesIgnoringVirtualPseudoElements, checkingContext));
+    }
+
     FunctionCall functionCall(m_assembler, m_registerAllocator, m_stackAllocator, m_functionCalls);
     functionCall.setFunctionAddress(elementIsHovered);
     functionCall.setOneArgument(elementAddressRegister);
     failureCases.append(functionCall.callAndBranchOnBooleanReturnValue(Assembler::Zero));
+
+    successCases.link(&m_assembler);
 }