Changeset 246096 in webkit

Timestamp:

Jun 4, 2019 9:15:21 PM (5 years ago)

Author:

Alan Bujtas

Message:

[ContentChangeObserver] Gmail text editing controls require two taps
​https://bugs.webkit.org/show_bug.cgi?id=198541
<rdar://problem/51375055>

Reviewed by Simon Fraser.

Source/WebCore:

When the animation completes we should also check if the newly visible content is also clickable and report it accordingly.
When the animated content is not clickable, we need to proceed with click instead of stopping at hover.

Test: fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html

• page/ios/ContentChangeObserver.cpp:

(WebCore::isConsideredClickable):
(WebCore::ContentChangeObserver::didFinishTransition):
(WebCore::ContentChangeObserver::adjustObservedState):
(WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
(WebCore::ContentChangeObserver::StyleChangeScope::isConsideredClickable const): Deleted. -> Turn it into a static function so that didFinishTransition could call it as well.

• page/ios/ContentChangeObserver.h:

LayoutTests:

• fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt (added)
• LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/ios/ContentChangeObserver.cpp (modified) (5 diffs)
• Source/WebCore/page/ios/ContentChangeObserver.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-06-04  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Gmail text editing controls require two taps
+        https://bugs.webkit.org/show_bug.cgi?id=198541
+        <rdar://problem/51375055>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html: Added.
+
 2019-06-04  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-06-04  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Gmail text editing controls require two taps
+        https://bugs.webkit.org/show_bug.cgi?id=198541
+        <rdar://problem/51375055>
+
+        Reviewed by Simon Fraser.
+
+        When the animation completes we should also check if the newly visible content is also clickable and report it accordingly.
+        When the animated content is not clickable, we need to proceed with click instead of stopping at hover.
+
+        Test: fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::isConsideredClickable):
+        (WebCore::ContentChangeObserver::didFinishTransition):
+        (WebCore::ContentChangeObserver::adjustObservedState):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::isConsideredClickable const): Deleted. -> Turn it into a static function so that didFinishTransition could call it as well.
+        * page/ios/ContentChangeObserver.h:
+
 2019-06-04  Michael Catanzaro  <mcatanzaro@igalia.com>
 

trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp

@@ -84 +84 @@
 }
 
+enum class ElementHadRenderer { No, Yes };
+static bool isConsideredClickable(const Element& newlyVisibleElement, ElementHadRenderer hadRenderer)
+{
+    auto& element = const_cast<Element&>(newlyVisibleElement);
+    if (element.isInUserAgentShadowTree())
+        return false;
+
+    if (is<HTMLIFrameElement>(element))
+        return true;
+
+    if (is<HTMLImageElement>(element)) {
+        // This is required to avoid HTMLImageElement's touch callout override logic. See rdar://problem/48937767.
+        return element.Element::willRespondToMouseClickEvents();
+    }
+
+    auto willRespondToMouseClickEvents = element.willRespondToMouseClickEvents();
+    if (hadRenderer == ElementHadRenderer::No || willRespondToMouseClickEvents)
+        return willRespondToMouseClickEvents;
+    // In case when the visible content already had renderers it's not sufficient to check the "newly visible" element only since it might just be the container for the clickable content.  
+    for (auto& descendant : descendantsOfType<RenderElement>(*element.renderer())) {
+        if (!descendant.element())
+            continue;
+        if (descendant.element()->willRespondToMouseClickEvents())
+            return true;
+    }
+    return false;
+}
 ContentChangeObserver::ContentChangeObserver(Document& document)
     : m_document(document)
@@ -160 +187 @@
     LOG_WITH_STREAM(ContentObservation, stream << "didFinishTransition: transition finished (" << &element << ").");
 
-    adjustObservedState(isConsideredHidden(element) ? Event::EndedTransition : Event::CompletedTransition);
+    if (isConsideredHidden(element)) {
+        adjustObservedState(Event::EndedTransitionButFinalStyleIsNotDefiniteYet);
+        return;
+    }
+    adjustObservedState(isConsideredClickable(element, ElementHadRenderer::Yes) ? Event::CompletedTransitionWithClickableContent : Event::CompletedTransitionWithoutClickableContent);
 }
 
@@ -473 +504 @@
             adjustStateAndNotifyContentChangeIfNeeded();
         break;
-    case Event::EndedTransition:
+    case Event::EndedTransitionButFinalStyleIsNotDefiniteYet:
         // onAnimationEnd can be called while in the middle of resolving the document (synchronously) or
         // asynchronously right before the style update is issued. It also means we don't know whether this animation ends up producing visible content yet. 
@@ -482 +513 @@
             setShouldObserveNextStyleRecalc(true);
         break;
-    case Event::CompletedTransition:
+    case Event::CompletedTransitionWithClickableContent:
         // Set visibility flag on and report visible change synchronously or asynchronously depending whether we are in the middle of style recalc.
         contentVisibilityDidChange();
+        FALLTHROUGH;
+    case Event::CompletedTransitionWithoutClickableContent:
         if (m_document.inStyleRecalc())
             m_isInObservedStyleRecalc = true;
@@ -521 +554 @@
     };
 
-    if (changedFromHiddenToVisible() && isConsideredClickable())
+    if (changedFromHiddenToVisible() && isConsideredClickable(m_element, m_hadRenderer ? ElementHadRenderer::Yes : ElementHadRenderer::No))
         m_contentChangeObserver.contentVisibilityDidChange();
-}
-
-bool ContentChangeObserver::StyleChangeScope::isConsideredClickable() const
-{
-    if (m_element.isInUserAgentShadowTree())
-        return false;
-
-    auto& element = const_cast<Element&>(m_element);
-    if (is<HTMLIFrameElement>(element))
-        return true;
-
-    if (is<HTMLImageElement>(element)) {
-        // This is required to avoid HTMLImageElement's touch callout override logic. See rdar://problem/48937767.
-        return element.Element::willRespondToMouseClickEvents();
-    }
-
-    auto willRespondToMouseClickEvents = element.willRespondToMouseClickEvents();
-    if (!m_hadRenderer || willRespondToMouseClickEvents)
-        return willRespondToMouseClickEvents;
-    // In case when the visible content already had renderers it's not sufficient to check the "newly visible" element only since it might just be the container for the clickable content.  
-    ASSERT(m_element.renderer());
-    for (auto& descendant : descendantsOfType<RenderElement>(*element.renderer())) {
-        if (!descendant.element())
-            continue;
-        if (descendant.element()->willRespondToMouseClickEvents())
-            return true;
-    }
-    return false;
 }
 

trunk/Source/WebCore/page/ios/ContentChangeObserver.h

@@ -72 +72 @@
 
     private:
-        bool isConsideredClickable() const;
-
         ContentChangeObserver& m_contentChangeObserver;
         const Element& m_element;
@@ -190 +188 @@
         EndedStyleRecalc,
         AddedTransition,
-        EndedTransition,
-        CompletedTransition,
+        EndedTransitionButFinalStyleIsNotDefiniteYet,
+        CompletedTransitionWithClickableContent,
+        CompletedTransitionWithoutClickableContent,
         CanceledTransition,
         StartedFixedObservationTimeWindow,