REGRESSION (r196383): Drop down CSS menus not working on cnet.com, apmex.com
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Aug 2016 18:00:48 +0000 (18:00 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Aug 2016 18:00:48 +0000 (18:00 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic-expected.txt [new file with mode: 0644]
LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic.html [new file with mode: 0644]
LayoutTests/platform/ios-simulator/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/css/SelectorChecker.cpp
Source/WebCore/cssjit/SelectorCompiler.cpp

index 6ab140a..f4942b4 100644 (file)
@@ -1,3 +1,13 @@
+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>
 
         Fetch Response built-ins should use @makeThisTypeError
diff --git a/LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic-expected.txt b/LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic-expected.txt
new file mode 100644 (file)
index 0000000..dd0b958
--- /dev/null
@@ -0,0 +1,5 @@
+Test that dynamically activated :hover rule affecting descendants works.
+Hover to see a green box below.
+Hover to see a green box below.
+Selector compiler: PASS
+Selector checker: PASS
diff --git a/LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic.html b/LayoutTests/fast/selectors/hover-invalidation-descendant-dynamic.html
new file mode 100644 (file)
index 0000000..6ddba31
--- /dev/null
@@ -0,0 +1,85 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+.parent {
+    position: relative;
+    padding: 10px;
+    background-color: silver;
+}
+
+.child {
+    position: absolute;
+    width: 200px;
+    height: 200px;
+    top: 0px;
+    background-color: green;
+    display: none;
+}
+
+#target.enableHover:hover .child {
+    display: block;
+}
+
+/* :matches() is here to disable the selector compiler */
+#target2:matches(:root, :nth-of-type(n), :not(#specificity-trick), :nth-last-of-type(n)).enableHover:hover .child {
+    display: block;
+}
+
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+function runTest(targetSelector) {
+    var target = document.querySelector(targetSelector);
+    var child = target.querySelector('.child');
+
+    target.classList.add('enableHover');
+
+    if (window.eventSender) {
+        var x = target.offsetLeft + target.offsetWidth / 2;
+        var y = target.offsetTop + target.offsetHeight / 2;
+        eventSender.mouseMoveTo(x, y);
+    }
+
+    return child.offsetWidth != 0;
+}
+
+function log(text, pass) {
+    var log = document.querySelector("#log");
+    var line = document.createElement("div");
+    line.innerHTML = text + ": " + (pass ? "PASS" : "FAIL");
+    log.appendChild(line);
+}
+
+function runTests() {
+    var compilerPass = runTest("#target");
+    var checkerPass = runTest("#target2");
+    if (!window.testRunner)
+        return;
+    log("Selector compiler", compilerPass);
+    log("Selector checker", checkerPass);
+
+    testRunner.notifyDone();
+}
+
+</script>
+</head>
+<body onload="runTests()">
+<div>
+Test that dynamically activated :hover rule affecting descendants works.
+</div>
+<div id="target" class="parent">
+    Hover to see a green box below.
+    <div class="child"></div>
+</div>
+<div id="target2" class="parent">
+    Hover to see a green box below.
+    <div class="child"></div>
+</div>
+<div id=log></div>
+</body>
+</html>
index 8e4e4be..0c579d4 100644 (file)
@@ -288,6 +288,7 @@ fast/css/pseudo-active-style-sharing-5.html [ Skip ]
 fast/css/pseudo-active-style-sharing-6.html [ Skip ]
 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 ]
 
index e33b22d..2893d23 100644 (file)
@@ -1,3 +1,39 @@
+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>
 
         [Cocoa] Freeze Objective-C bindings and stop autogenerating them: Step 1 - Convert a single file
index bc747ea..f6debfb 100644 (file)
@@ -951,6 +951,10 @@ bool SelectorChecker::checkOne(CheckingContext& checkingContext, const LocalCont
             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))
                     return true;
             }
index dbd29e9..8171df3 100644 (file)
@@ -3214,10 +3214,22 @@ void SelectorCodeGenerator::generateElementIsHovered(Assembler::JumpList& failur
 
     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);
 }
 
 void SelectorCodeGenerator::generateElementIsInLanguage(Assembler::JumpList& failureCases, const SelectorFragment& fragment)