[ContentChangeObserver] Dispatch synthetic click when the visibility candidate elemen...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Aug 2019 22:18:56 +0000 (22:18 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Aug 2019 22:18:56 +0000 (22:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200773
<rdar://problem/54351728>

Reviewed by Simon Fraser.

Source/WebCore:

This patch fixes the case when the candidate element (going from hidden to visible) becomes hidden by the end of the observation window. It essentially means that no visible change has happened
and we should proceed with dispatching the synthetic click event.
We now keep track of the candidate element and reset the visiblity state when it loses its renderer.

Test: fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html

* page/ios/ContentChangeObserver.cpp:
(WebCore::ContentChangeObserver::didAddTransition):
(WebCore::ContentChangeObserver::didFinishTransition):
(WebCore::ContentChangeObserver::didInstallDOMTimer):
(WebCore::ContentChangeObserver::reset):
(WebCore::ContentChangeObserver::rendererWillBeDestroyed):
(WebCore::ContentChangeObserver::contentVisibilityDidChange):
(WebCore::ContentChangeObserver::touchEventDidStart):
(WebCore::ContentChangeObserver::touchEventDidFinish):
(WebCore::ContentChangeObserver::mouseMovedDidStart):
(WebCore::ContentChangeObserver::mouseMovedDidFinish):
(WebCore::ContentChangeObserver::adjustObservedState):
(WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
(WebCore::ContentChangeObserver::hasDeterminateState const): Deleted.
* page/ios/ContentChangeObserver.h:
(WebCore::ContentChangeObserver::hasObservedTransition const):
(WebCore::ContentChangeObserver::setTouchEventIsBeingDispatched):
(WebCore::ContentChangeObserver::isTouchEventBeingDispatched const):
(WebCore::ContentChangeObserver::setMouseMovedEventIsBeingDispatched):
(WebCore::ContentChangeObserver::isMouseMovedEventBeingDispatched const):
(WebCore::ContentChangeObserver::isObservingContentChanges const):

LayoutTests:

* fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden-expected.html: Added.
* fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h

index 97f9adf..98ab113 100644 (file)
@@ -1,3 +1,14 @@
+2019-08-15  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Dispatch synthetic click when the visibility candidate element becomes hidden again.
+        https://bugs.webkit.org/show_bug.cgi?id=200773
+        <rdar://problem/54351728>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden-expected.html: Added.
+        * fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html: Added.
+
 2019-08-15  Robin Morisset  <rmorisset@apple.com>
 
         [WHLSL] Don't accept operator&& or operator|| in the Lexer
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden-expected.txt
new file mode 100644 (file)
index 0000000..677e3a3
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is shown below.
+clicked
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html b/LayoutTests/fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html
new file mode 100644 (file)
index 0000000..0951e2c
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html><!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<title>This tests the case when visible and actionable content shows up and gets destroyed right away.</title>
+<script src="../../../../../resources/ui-helper.js"></script>
+<style>
+#tapThis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#willBecomeVisibleMomentarily {
+    display: none;
+    width: 100px;
+    height: 100px;
+    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();
+
+    await UIHelper.activateElement(tapThis);
+}
+</script>
+</head>
+<body onload="test()">
+<div id=tapThis>PASS if 'clicked' text is shown below.</div>
+<div id=willBecomeVisibleMomentarily></div>
+<pre id=result></pre>
+<script>
+tapThis.addEventListener("touchstart", function( event ) {
+       willBecomeVisibleMomentarily.style.display = "block";
+}, false);
+
+tapThis.addEventListener("mouseover", function( event ) {
+       willBecomeVisibleMomentarily.style.display = "none";
+}, false);
+
+willBecomeVisibleMomentarily.addEventListener("click", function( event ) {
+    result.innerHTML = "clicked willBecomeVisibleMomentarily";
+}, false);
+
+tapThis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+    if (window.testRunner)
+        testRunner.notifyDone();
+}, false);
+</script>
+</body>
+</html>
index 7ab16a9..434c1a4 100644 (file)
@@ -1,3 +1,39 @@
+2019-08-15  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Dispatch synthetic click when the visibility candidate element becomes hidden again.
+        https://bugs.webkit.org/show_bug.cgi?id=200773
+        <rdar://problem/54351728>
+
+        Reviewed by Simon Fraser.
+
+        This patch fixes the case when the candidate element (going from hidden to visible) becomes hidden by the end of the observation window. It essentially means that no visible change has happened
+        and we should proceed with dispatching the synthetic click event.
+        We now keep track of the candidate element and reset the visiblity state when it loses its renderer.
+
+        Test: fast/events/touch/ios/content-observation/going-from-hidden-to-visible-and-to-hidden.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::ContentChangeObserver::didAddTransition):
+        (WebCore::ContentChangeObserver::didFinishTransition):
+        (WebCore::ContentChangeObserver::didInstallDOMTimer):
+        (WebCore::ContentChangeObserver::reset):
+        (WebCore::ContentChangeObserver::rendererWillBeDestroyed):
+        (WebCore::ContentChangeObserver::contentVisibilityDidChange):
+        (WebCore::ContentChangeObserver::touchEventDidStart):
+        (WebCore::ContentChangeObserver::touchEventDidFinish):
+        (WebCore::ContentChangeObserver::mouseMovedDidStart):
+        (WebCore::ContentChangeObserver::mouseMovedDidFinish):
+        (WebCore::ContentChangeObserver::adjustObservedState):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::ContentChangeObserver::hasDeterminateState const): Deleted.
+        * page/ios/ContentChangeObserver.h:
+        (WebCore::ContentChangeObserver::hasObservedTransition const):
+        (WebCore::ContentChangeObserver::setTouchEventIsBeingDispatched):
+        (WebCore::ContentChangeObserver::isTouchEventBeingDispatched const):
+        (WebCore::ContentChangeObserver::setMouseMovedEventIsBeingDispatched):
+        (WebCore::ContentChangeObserver::isMouseMovedEventBeingDispatched const):
+        (WebCore::ContentChangeObserver::isObservingContentChanges const):
+
 2019-08-15  Justin Fan  <justin_fan@apple.com>
 
         Unreviewed suggested patch follow-up to https://bugs.webkit.org/show_bug.cgi?id=200740.
index a70510f..7f568bf 100644 (file)
@@ -214,6 +214,8 @@ void ContentChangeObserver::didAddTransition(const Element& element, const Anima
         return;
     if (hasVisibleChangeState())
         return;
+    if (!isObservingContentChanges())
+        return;
     if (!isObservingTransitions())
         return;
     if (!transition.isDurationSet() || !transition.isPropertySet())
@@ -251,7 +253,7 @@ void ContentChangeObserver::didFinishTransition(const Element& element, CSSPrope
             return;
         }
         if (isConsideredClickable(*targetElement, ElementHadRenderer::Yes))
-            weakThis->contentVisibilityDidChange();
+            weakThis->contentVisibilityDidChange(*targetElement);
         weakThis->adjustObservedState(Event::CompletedTransition);
     });
 }
@@ -271,14 +273,16 @@ void ContentChangeObserver::didInstallDOMTimer(const DOMTimer& timer, Seconds ti
 {
     if (!m_document.settings().contentChangeObserverEnabled())
         return;
-    if (m_document.activeDOMObjectsAreSuspended())
-        return;
-    if (timeout > maximumDelayForTimers || !singleShot)
+    if (!isObservingContentChanges())
         return;
     if (!isObservingDOMTimerScheduling())
         return;
     if (hasVisibleChangeState())
         return;
+    if (m_document.activeDOMObjectsAreSuspended())
+        return;
+    if (timeout > maximumDelayForTimers || !singleShot)
+        return;
     LOG_WITH_STREAM(ContentObservation, stream << "didInstallDOMTimer: register this timer: (" << &timer << ") and observe when it fires.");
 
     registerDOMTimer(timer);
@@ -359,12 +363,15 @@ void ContentChangeObserver::reset()
 {
     stopObservingPendingActivities();
     setHasNoChangeState();
+
+    setTouchEventIsBeingDispatched(false);
     setIsBetweenTouchEndAndMouseMoved(false);
+    setMouseMovedEventIsBeingDispatched(false);
 
-    m_touchEventIsBeingDispatched = false;
     m_isInObservedStyleRecalc = false;
     m_observedDomTimerIsBeingExecuted = false;
-    m_mouseMovedEventIsBeingDispatched = false;
+
+    m_visibilityCandidateElement = { };
 
     m_contentObservationTimer.stop();
     m_elementsWithDestroyedVisibleRenderer.clear();
@@ -389,17 +396,24 @@ void ContentChangeObserver::rendererWillBeDestroyed(const Element& element)
         return;
     if (!isObservingContentChanges())
         return;
-    if (hasVisibleChangeState())
-        return;
     LOG_WITH_STREAM(ContentObservation, stream << "rendererWillBeDestroyed element: " << &element);
 
     if (!isVisuallyHidden(element))
         m_elementsWithDestroyedVisibleRenderer.add(&element);
+    // Candidate element is no longer visible.
+    if (m_visibilityCandidateElement == &element) {
+        // FIXME: We should also check for other type of visiblity changes.
+        ASSERT(hasVisibleChangeState());
+        m_visibilityCandidateElement = { };
+        setHasIndeterminateState();
+    }
 }
 
-void ContentChangeObserver::contentVisibilityDidChange()
+void ContentChangeObserver::contentVisibilityDidChange(const Element& element)
 {
     LOG(ContentObservation, "contentVisibilityDidChange: visible content change did happen.");
+    // FIXME: This should evolve into a list of candidate elements.
+    m_visibilityCandidateElement = makeWeakPtr(element);
     adjustObservedState(Event::ContentVisibilityChanged);
 }
 
@@ -411,7 +425,7 @@ void ContentChangeObserver::touchEventDidStart(PlatformEvent::Type eventType)
     if (eventType != PlatformEvent::Type::TouchStart)
         return;
     LOG(ContentObservation, "touchEventDidStart: touch start event started.");
-    m_touchEventIsBeingDispatched = true;
+    setTouchEventIsBeingDispatched(true);
     adjustObservedState(Event::StartedTouchStartEventDispatching);
 #else
     UNUSED_PARAM(eventType);
@@ -421,11 +435,11 @@ void ContentChangeObserver::touchEventDidStart(PlatformEvent::Type eventType)
 void ContentChangeObserver::touchEventDidFinish()
 {
 #if ENABLE(TOUCH_EVENTS)
-    if (!m_touchEventIsBeingDispatched)
+    if (!isTouchEventBeingDispatched())
         return;
     ASSERT(m_document.settings().contentChangeObserverEnabled());
     LOG(ContentObservation, "touchEventDidFinish: touch start event finished.");
-    m_touchEventIsBeingDispatched = false;
+    setTouchEventIsBeingDispatched(false);
     adjustObservedState(Event::EndedTouchStartEventDispatching);
 #endif
 }
@@ -435,18 +449,18 @@ void ContentChangeObserver::mouseMovedDidStart()
     if (!m_document.settings().contentChangeObserverEnabled())
         return;
     LOG(ContentObservation, "mouseMovedDidStart: mouseMoved started.");
-    m_mouseMovedEventIsBeingDispatched = true;
+    setMouseMovedEventIsBeingDispatched(true);
     adjustObservedState(Event::StartedMouseMovedEventDispatching);
 }
 
 void ContentChangeObserver::mouseMovedDidFinish()
 {
-    if (!m_mouseMovedEventIsBeingDispatched)
+    if (!isMouseMovedEventBeingDispatched())
         return;
     ASSERT(m_document.settings().contentChangeObserverEnabled());
     LOG(ContentObservation, "mouseMovedDidFinish: mouseMoved finished.");
     adjustObservedState(Event::EndedMouseMovedEventDispatching);
-    m_mouseMovedEventIsBeingDispatched = false;
+    setMouseMovedEventIsBeingDispatched(false);
 }
 
 void ContentChangeObserver::setShouldObserveNextStyleRecalc(bool shouldObserve)
@@ -456,13 +470,6 @@ void ContentChangeObserver::setShouldObserveNextStyleRecalc(bool shouldObserve)
     m_isWaitingForStyleRecalc = shouldObserve;
 }
 
-bool ContentChangeObserver::hasDeterminateState() const
-{
-    if (hasVisibleChangeState())
-        return true;
-    return observedContentChange() == WKContentNoChange && !hasPendingActivity();
-}
-
 void ContentChangeObserver::adjustObservedState(Event event)
 {
     auto resetToStartObserving = [&] {
@@ -477,27 +484,34 @@ void ContentChangeObserver::adjustObservedState(Event event)
     };
 
     auto notifyClientIfNeeded = [&] {
-        // First demote to "no change" if there's no pending activity anymore.
-        if (observedContentChange() == WKContentIndeterminateChange && !hasPendingActivity())
-            setHasNoChangeState();
-
+        if (isTouchEventBeingDispatched()) {
+            LOG(ContentObservation, "notifyClientIfNeeded: Touch event is being dispatched. No need to notify the client.");
+            return;
+        }
         if (isBetweenTouchEndAndMouseMoved()) {
-            LOG(ContentObservation, "adjustStateAndNotifyContentChangeIfNeeded: Not reached mouseMoved yet. No need to notify the client.");
+            LOG(ContentObservation, "notifyClientIfNeeded: Not reached mouseMoved yet. No need to notify the client.");
             return;
         }
-        if (m_mouseMovedEventIsBeingDispatched) {
-            LOG(ContentObservation, "adjustStateAndNotifyContentChangeIfNeeded: in mouseMoved call. No need to notify the client.");
+        if (isMouseMovedEventBeingDispatched()) {
+            LOG(ContentObservation, "notifyClientIfNeeded: in mouseMoved call. No need to notify the client.");
             return;
         }
         if (isObservationTimeWindowActive()) {
-            LOG(ContentObservation, "adjustStateAndNotifyContentChangeIfNeeded: Inside the fixed window observation. No need to notify the client.");
+            LOG(ContentObservation, "notifyClientIfNeeded: Inside the fixed window observation. No need to notify the client.");
             return;
         }
-        if (!hasDeterminateState()) {
-            LOG(ContentObservation, "adjustStateAndNotifyContentChangeIfNeeded: not in a determined state yet.");
+
+        // The fixed observation window (which is the final step in content observation) is closed and now we check if are still waiting for timers or animations to finish.
+        if (hasPendingActivity()) {
+            LOG(ContentObservation, "notifyClientIfNeeded: We are still waiting on some events.");
             return;
         }
-        LOG_WITH_STREAM(ContentObservation, stream << "adjustStateAndNotifyContentChangeIfNeeded: sending observedContentChange ->" << observedContentChange());
+
+        // First demote to "no change" because we've got no pending activity anymore.
+        if (observedContentChange() == WKContentIndeterminateChange)
+            setHasNoChangeState();
+
+        LOG_WITH_STREAM(ContentObservation, stream << "notifyClientIfNeeded: sending observedContentChange ->" << observedContentChange());
         ASSERT(m_document.page());
         ASSERT(m_document.frame());
         m_document.page()->chrome().client().didFinishContentChangeObserving(*m_document.frame(), observedContentChange());
@@ -595,7 +609,7 @@ void ContentChangeObserver::adjustObservedState(Event event)
     }
     // Either the page decided to call preventDefault on the touch action or the tap gesture evolved to some other gesture (long press, double tap). 
     if (event == Event::WillNotProceedWithClick) {
-        reset();
+        stopContentObservation();
         return;
     }
     // The page produced an visible change on an actionable content.
@@ -628,7 +642,7 @@ ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
     };
 
     if (changedFromHiddenToVisible() && isConsideredClickable(m_element, m_hadRenderer ? ElementHadRenderer::Yes : ElementHadRenderer::No))
-        m_contentChangeObserver.contentVisibilityDidChange();
+        m_contentChangeObserver.contentVisibilityDidChange(m_element);
 }
 
 #if ENABLE(TOUCH_EVENTS)
index 1c2b4d0..3094e28 100644 (file)
@@ -128,7 +128,7 @@ private:
 
     void didRecognizeLongPress();
 
-    void contentVisibilityDidChange();
+    void contentVisibilityDidChange(const Element&);
 
     void setShouldObserveDOMTimerSchedulingAndTransitions(bool);
     bool isObservingDOMTimerScheduling() const { return m_isObservingDOMTimerScheduling; }
@@ -159,11 +159,16 @@ private:
     bool hasVisibleChangeState() const { return observedContentChange() == WKContentVisibilityChange; }
     bool hasObservedDOMTimer() const { return !m_DOMTimerList.isEmpty(); }
     bool hasObservedTransition() const { return !m_elementsWithTransition.isEmpty(); }
-    bool hasDeterminateState() const;
 
     void setIsBetweenTouchEndAndMouseMoved(bool isBetween) { m_isBetweenTouchEndAndMouseMoved = isBetween; }
     bool isBetweenTouchEndAndMouseMoved() const { return m_isBetweenTouchEndAndMouseMoved; }
 
+    void setTouchEventIsBeingDispatched(bool dispatching) { m_touchEventIsBeingDispatched = dispatching; }
+    bool isTouchEventBeingDispatched() const { return m_touchEventIsBeingDispatched; }
+
+    void setMouseMovedEventIsBeingDispatched(bool dispatching) { m_mouseMovedEventIsBeingDispatched = dispatching; }
+    bool isMouseMovedEventBeingDispatched() const { return m_mouseMovedEventIsBeingDispatched; }
+
     bool hasPendingActivity() const { return hasObservedDOMTimer() || hasObservedTransition() || m_isWaitingForStyleRecalc || isObservationTimeWindowActive(); }
     bool isObservationTimeWindowActive() const { return m_contentObservationTimer.isActive(); }
 
@@ -202,6 +207,7 @@ private:
     HashSet<const Element*> m_elementsWithDestroyedVisibleRenderer;
     WKContentChange m_observedContentState { WKContentNoChange };
     WeakPtr<Element> m_hiddenTouchTargetElement;
+    WeakPtr<Element> m_visibilityCandidateElement;
     bool m_touchEventIsBeingDispatched { false };
     bool m_isWaitingForStyleRecalc { false };
     bool m_isInObservedStyleRecalc { false };
@@ -214,9 +220,9 @@ private:
 
 inline bool ContentChangeObserver::isObservingContentChanges() const
 {
-    return m_touchEventIsBeingDispatched
-        || m_isBetweenTouchEndAndMouseMoved
-        || m_mouseMovedEventIsBeingDispatched
+    return isTouchEventBeingDispatched()
+        || isBetweenTouchEndAndMouseMoved()
+        || isMouseMovedEventBeingDispatched()
         || m_observedDomTimerIsBeingExecuted
         || m_isInObservedStyleRecalc
         || isObservationTimeWindowActive();