[ContentChangeObserver] Click event fires immediately on hover menu at seriouseats.com
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 10 Mar 2019 04:48:34 +0000 (04:48 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 10 Mar 2019 04:48:34 +0000 (04:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195520
<rdar://problem/48740098>

Reviewed by Simon Fraser.

Source/WebCore:

Unfortunately seriouseats has a 300ms hover intent delay to deal with accidental menupane pop-ups. This page also hides this
non-fixed width menupane using absolute positioning and negative left.

Test: fast/events/touch/ios/content-observation/move-content-from-offscreen.html

* page/ios/ContentChangeObserver.cpp:
(WebCore::ContentChangeObserver::didInstallDOMTimer):
(WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
(WebCore::ContentChangeObserver::StyleChangeScope::isConsideredHidden const): Content auhtors tend to use x - 1 values (where x = 10^y)

LayoutTests:

* fast/events/touch/ios/content-observation/move-content-from-offscreen-expected.txt: Added.
* fast/events/touch/ios/content-observation/move-content-from-offscreen.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp

index 4d6cac7..57c91dc 100644 (file)
@@ -1,5 +1,16 @@
 2019-03-09  Zalan Bujtas  <zalan@apple.com>
 
+        [ContentChangeObserver] Click event fires immediately on hover menu at seriouseats.com
+        https://bugs.webkit.org/show_bug.cgi?id=195520
+        <rdar://problem/48740098>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/move-content-from-offscreen-expected.txt: Added.
+        * fast/events/touch/ios/content-observation/move-content-from-offscreen.html: Added.
+
+2019-03-09  Zalan Bujtas  <zalan@apple.com>
+
         [ContentChangeObserver] Start observing for content change between touchEnd and mouseMoved start
         https://bugs.webkit.org/show_bug.cgi?id=195510
         <rdar://problem/48735695>
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen-expected.txt
new file mode 100644 (file)
index 0000000..66db4c9
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is not shown below.
+
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen.html b/LayoutTests/fast/events/touch/ios/content-observation/move-content-from-offscreen.html
new file mode 100644 (file)
index 0000000..47e7f5e
--- /dev/null
@@ -0,0 +1,62 @@
+<html>
+<head>
+<title>This tests the case when the absolute positioned, non-fixed width content is offscreen.</title>
+<script src="../../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#becomesVisible {
+       position: absolute;
+       left: -999px;
+       top: 50px;
+    width: 10%;
+    height: 10%;
+    background-color: green;
+}
+</style>
+<script>
+async function test() {
+    if (!window.testRunner || !testRunner.runUIScript)
+        return;
+    if (window.internals)
+        internals.settings.setContentChangeObserverEnabled(true);
+
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+
+    let rect = tapthis.getBoundingClientRect();
+    let x = rect.left + rect.width / 2;
+    let y = rect.top + rect.height / 2;
+
+    await tapAtPoint(x, y);
+}
+</script>
+</head>
+<body onload="test()">
+<div id=tapthis>PASS if 'clicked' text is not shown below.</div>
+<div id=becomesVisible></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mousemove", function( event ) {
+    setTimeout(function() {
+        becomesVisible.style.left = "0px";
+        document.body.offsetHeight;
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, 300);
+}, false);
+
+becomesVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+}, false);
+</script>
+</body>
+</html>
index 7627438..12faadd 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-09  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Click event fires immediately on hover menu at seriouseats.com
+        https://bugs.webkit.org/show_bug.cgi?id=195520
+        <rdar://problem/48740098>
+
+        Reviewed by Simon Fraser.
+
+        Unfortunately seriouseats has a 300ms hover intent delay to deal with accidental menupane pop-ups. This page also hides this
+        non-fixed width menupane using absolute positioning and negative left.  
+
+        Test: fast/events/touch/ios/content-observation/move-content-from-offscreen.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::ContentChangeObserver::didInstallDOMTimer):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::isConsideredHidden const): Content auhtors tend to use x - 1 values (where x = 10^y)
+
 2019-03-09  Chris Dumez  <cdumez@apple.com>
 
         Add assertions to help debug crash under DOMWindowExtension::suspendForPageCache()
index 56a158e..fdd0f12 100644 (file)
@@ -66,7 +66,7 @@ void ContentChangeObserver::didInstallDOMTimer(const DOMTimer& timer, Seconds ti
         return;
     if (m_document.activeDOMObjectsAreSuspended())
         return;
-    if (timeout > 250_ms || !singleShot)
+    if (timeout > 300_ms || !singleShot)
         return;
     if (!isObservingDOMTimerScheduling())
         return;
@@ -317,7 +317,7 @@ ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
     auto changedFromHiddenToVisible = [&] {
         return m_wasHidden && !isConsideredHidden();
     };
-    
+
     if (changedFromHiddenToVisible() && isConsideredClickable())
         m_contentChangeObserver.contentVisibilityDidChange();
 }
@@ -334,20 +334,23 @@ bool ContentChangeObserver::StyleChangeScope::isConsideredHidden() const
     if (style.visibility() == Visibility::Hidden)
         return true;
 
-    auto width = style.width();
-    auto height = style.height();
+    auto width = style.logicalWidth();
+    auto height = style.logicalHeight();
     if ((width.isFixed() && !width.value()) || (height.isFixed() && !height.value()))
         return true;
 
-    auto top = style.top();
-    auto left = style.left();
+    auto top = style.logicalTop();
+    auto left = style.logicalLeft();
     // FIXME: This is trying to check if the element is outside of the viewport. This is incorrect for many reasons.
     if (left.isFixed() && width.isFixed() && -left.value() >= width.value())
         return true;
-
     if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
         return true;
 
+    // It's a common technique used to position content offscreen.
+    if (style.hasOutOfFlowPosition() && left.isFixed() && left.value() <= -999)
+        return true;
+
     // FIXME: Check for other cases like zero height with overflow hidden.
     auto maxHeight = style.maxHeight();
     if (maxHeight.isFixed() && !maxHeight.value())