Attachment 371336 Details for Bug 198541 – Patch

[patch] Patch

bug-198541-20190604142915.patch (text/plain), 11.29 KB, created by zalan on 2019-06-04 14:29:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-06-04 14:29:17 PDT

Size: 11.29 KB

patch

obsolete

>Subversion Revision: 246067
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ce97d1d8dfb9d3d9c0cdb9e031f2b073905c6594..b441549584b3956b032a52e7d87dea485f93ac1d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,24 @@
>+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 NOBODY (OOPS!).
>+
>+        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  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][IFC] Decouple float placement and line shrinking
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.cpp b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>index ffd7a74b9a83a535d07707cd3bbd30819aa31854..803c4599fd688febaae409ba421ef2a53a6ea0c2 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.cpp
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>@@ -83,6 +83,33 @@ static bool isConsideredHidden(const Element& element)
>     return false;
> }
> 
>+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)
>     , m_contentObservationTimer([this] { completeDurationBasedContentObservation(); })
>@@ -159,7 +186,11 @@ void ContentChangeObserver::didFinishTransition(const Element& element, CSSPrope
>         return;
>     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);
> }
> 
> void ContentChangeObserver::didRemoveTransition(const Element& element, CSSPropertyID propertyID)
>@@ -472,7 +503,7 @@ void ContentChangeObserver::adjustObservedState(Event event)
>         if (!isObservationTimeWindowActive())
>             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. 
>         if (m_document.inStyleRecalc()) {
>@@ -481,9 +512,11 @@ void ContentChangeObserver::adjustObservedState(Event event)
>         } else
>             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;
>         else if (!isObservationTimeWindowActive())
>@@ -520,38 +553,10 @@ ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
>         return m_wasHidden && !isConsideredHidden(m_element);
>     };
> 
>-    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;
>-}
>-
> #if ENABLE(TOUCH_EVENTS)
> ContentChangeObserver::TouchEventScope::TouchEventScope(Document& document, PlatformEvent::Type eventType)
>     : m_contentChangeObserver(document.contentChangeObserver())
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.h b/Source/WebCore/page/ios/ContentChangeObserver.h
>index c4c16c9fe6c10c05e9ce2c4d91ba07758a5b35af..1ef64c2b343e89ae76819a0e2d6905d7918f8461 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.h
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.h
>@@ -71,8 +71,6 @@ public:
>         ~StyleChangeScope();
> 
>     private:
>-        bool isConsideredClickable() const;
>-
>         ContentChangeObserver& m_contentChangeObserver;
>         const Element& m_element;
>         bool m_wasHidden { false };
>@@ -189,8 +187,9 @@ private:
>         StartedStyleRecalc,
>         EndedStyleRecalc,
>         AddedTransition,
>-        EndedTransition,
>-        CompletedTransition,
>+        EndedTransitionButFinalStyleIsNotDefiniteYet,
>+        CompletedTransitionWithClickableContent,
>+        CompletedTransitionWithoutClickableContent,
>         CanceledTransition,
>         StartedFixedObservationTimeWindow,
>         EndedFixedObservationTimeWindow,
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index cf836839960695af3ba0fd72003df86729080094..445f5137b39a98d06bb00048a653383f6d0ebf3d 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+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 NOBODY (OOPS!).
>+
>+        * fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html: Added.
>+
> 2019-06-04  Antoine Quint  <graouts@apple.com>
> 
>         The "mouseenter" and "pointerenter" events are fired from the bottom up
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..677e3a31e4d27593aa94dbd637a453af8ceddd78
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt
>@@ -0,0 +1,2 @@
>+PASS if 'clicked' text is shown below.
>+clicked
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html b/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..6e3a920981af26278ab6506409ecb57e852e0c0c
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html
>@@ -0,0 +1,57 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>This tests the case when mousemove triggers a 10ms transition with delay and the new content is not "clickable"</title>
>+<script src="../../../../../resources/basic-gestures.js"></script>
>+<style>
>+#tapthis {
>+    width: 400px;
>+    height: 400px;
>+    border: 1px solid green;
>+}
>+
>+#becomesVisible {
>+	position: absolute;
>+	top: 100px;
>+	left: -1000px;
>+	width: 100px;
>+	height: 100px;
>+	background-color: green;
>+	transition: left 10ms ease-in-out 100ms;
>+}
>+</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 shown below.</div>
>+<div id=becomesVisible></div>
>+<pre id=result></pre>
>+<script>
>+tapthis.addEventListener("mousemove", function( event ) {
>+    becomesVisible.style.left = "10px";
>+}, false);
>+
>+tapthis.addEventListener("click", function( event ) {   
>+    result.innerHTML = "clicked";
>+    if (window.testRunner)
>+        testRunner.notifyDone();
>+}, false);
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 198541: 371336