- Revision
- 246096
- Author
- za...@apple.com
- Date
- 2019-06-04 21:15:21 -0700 (Tue, 04 Jun 2019)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (246095 => 246096)
--- trunk/LayoutTests/ChangeLog 2019-06-05 03:14:20 UTC (rev 246095)
+++ trunk/LayoutTests/ChangeLog 2019-06-05 04:15:21 UTC (rev 246096)
@@ -1,3 +1,13 @@
+2019-06-04 Zalan Bujtas <za...@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 <you...@apple.com>
Layout test landed flaky in 245873 [ Release ] http/wpt/service-workers/service-worker-networkprocess-crash.html is a flaky failure
Added: trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt (0 => 246096)
--- trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable-expected.txt 2019-06-05 04:15:21 UTC (rev 246096)
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is shown below.
+clicked
Added: trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html (0 => 246096)
--- trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/content-observation/100ms-delay-10ms-transition-on-mousemove-no-clickable.html 2019-06-05 04:15:21 UTC (rev 246096)
@@ -0,0 +1,57 @@
+<!DOCTYPE html><!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<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=""
+<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>
Modified: trunk/Source/WebCore/ChangeLog (246095 => 246096)
--- trunk/Source/WebCore/ChangeLog 2019-06-05 03:14:20 UTC (rev 246095)
+++ trunk/Source/WebCore/ChangeLog 2019-06-05 04:15:21 UTC (rev 246096)
@@ -1,3 +1,24 @@
+2019-06-04 Zalan Bujtas <za...@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 <mcatanz...@igalia.com>
Fix miscellaneous build warnings
Modified: trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp (246095 => 246096)
--- trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp 2019-06-05 03:14:20 UTC (rev 246095)
+++ trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp 2019-06-05 04:15:21 UTC (rev 246096)
@@ -83,6 +83,33 @@
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 @@
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 @@
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 @@
} 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 @@
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())
Modified: trunk/Source/WebCore/page/ios/ContentChangeObserver.h (246095 => 246096)
--- trunk/Source/WebCore/page/ios/ContentChangeObserver.h 2019-06-05 03:14:20 UTC (rev 246095)
+++ trunk/Source/WebCore/page/ios/ContentChangeObserver.h 2019-06-05 04:15:21 UTC (rev 246096)
@@ -71,8 +71,6 @@
~StyleChangeScope();
private:
- bool isConsideredClickable() const;
-
ContentChangeObserver& m_contentChangeObserver;
const Element& m_element;
bool m_wasHidden { false };
@@ -189,8 +187,9 @@
StartedStyleRecalc,
EndedStyleRecalc,
AddedTransition,
- EndedTransition,
- CompletedTransition,
+ EndedTransitionButFinalStyleIsNotDefiniteYet,
+ CompletedTransitionWithClickableContent,
+ CompletedTransitionWithoutClickableContent,
CanceledTransition,
StartedFixedObservationTimeWindow,
EndedFixedObservationTimeWindow,
