Attachment 373596 Details for Bug 199542 – Patch

[patch] Patch

bug-199542-20190707064704.patch (text/plain), 14.39 KB, created by zalan on 2019-07-07 06:47:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-07-07 06:47:05 PDT

Size: 14.39 KB

patch

obsolete

>Subversion Revision: 247197
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 313c764a0400edb61ef02fca05787ac3f80d7377..648003939de1a71b5dfea23f4366cca08454a959 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-07-07  Zalan Bujtas  <zalan@apple.com>
>+
>+        [ContentChangeObserver] Difficult to control videos on iqiyi.com as the actions are mouse hover
>+        https://bugs.webkit.org/show_bug.cgi?id=199542
>+        <rdar://problem/51886813>
>+
>+        Reviewed by Simon Fraser.
>+
>+        Decouple isVisuallyHidden and isConsideredVisible. Just because an element is not visually hidden (1px wide content)
>+        it is not necessarily qualified to be visible in the context of hover heuristic (e.g. iqiyi.com brings up a 1px wide
>+        clickable element when hovering over the scrubber. This element is clearly not designed to be actionable.)
>+
>+        Tests: fast/events/touch/ios/content-observation/tap-on-1px-height-content.html
>+               fast/events/touch/ios/content-observation/tap-on-1px-width-content.html
>+
>+        * dom/Node.cpp:
>+        (WebCore::Node::defaultEventHandler):
>+        * page/ios/ContentChangeObserver.cpp:
>+        (WebCore::ContentChangeObserver::isVisuallyHidden):
>+        (WebCore::ContentChangeObserver::isConsideredVisible):
>+        (WebCore::ContentChangeObserver::didAddTransition):
>+        (WebCore::ContentChangeObserver::didFinishTransition):
>+        (WebCore::ContentChangeObserver::willDestroyRenderer):
>+        (WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
>+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
>+        (WebCore::ContentChangeObserver::isConsideredHidden): Deleted.
>+        * page/ios/ContentChangeObserver.h:
>+
> 2019-07-06  Simon Fraser  <simon.fraser@apple.com>
> 
>         Long hang when loading a cnn.com page on iOS
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6801a1b1051fc698dde38a45dcd182d74c9c1f1c..f50f49e6bba7edf93e50fe99e14bfa7b19171433 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-07-07  Zalan Bujtas  <zalan@apple.com>
>+
>+        [ContentChangeObserver] Difficult to control videos on iqiyi.com as the actions are mouse hover
>+        https://bugs.webkit.org/show_bug.cgi?id=199542
>+        <rdar://problem/51886813>
>+
>+        Reviewed by Simon Fraser.
>+
>+        * WebProcess/WebPage/ios/WebPageIOS.mm:
>+        (WebKit::WebPage::handleSyntheticClick):
>+
> 2019-07-06  Antoine Quint  <graouts@apple.com>
> 
>         [Pointer Events] Use a gesture recognizer to prevent pinch-to-zoom behavior
>diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
>index 6534378a22f1324eb0e194138cc84e3450dc6a4f..5591a388830a8c3b7bca7b31d7a931102550148f 100644
>--- a/Source/WebCore/dom/Node.cpp
>+++ b/Source/WebCore/dom/Node.cpp
>@@ -2458,7 +2458,7 @@ void Node::defaultEventHandler(Event& event)
>         // Capture the target node's visibility state before dispatching touchStart.
>         if (is<Element>(*this) && eventType == eventNames().touchstartEvent) {
>             auto& contentChangeObserver = document().contentChangeObserver(); 
>-            if (ContentChangeObserver::isConsideredHidden(*this))
>+            if (ContentChangeObserver::isVisuallyHidden(*this))
>                 contentChangeObserver.setHiddenTouchTarget(downcast<Element>(*this));
>             else
>                 contentChangeObserver.resetHiddenTouchTarget();
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.cpp b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>index 2862e5c5722a04cda2b35f83fbc76d6b54e5adf4..8eb047e32b6ebd9d3c00474e316ded39d2e46fa3 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.cpp
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>@@ -43,7 +43,7 @@ namespace WebCore {
> static const Seconds maximumDelayForTimers { 400_ms };
> static const Seconds maximumDelayForTransitions { 300_ms };
> 
>-bool ContentChangeObserver::isConsideredHidden(const Node& node)
>+bool ContentChangeObserver::isVisuallyHidden(const Node& node)
> {
>     if (!node.renderStyle())
>         return true;
>@@ -91,6 +91,24 @@ bool ContentChangeObserver::isConsideredHidden(const Node& node)
>     return false;
> }
> 
>+bool ContentChangeObserver::isConsideredVisible(const Node& node)
>+{
>+    if (isVisuallyHidden(node))
>+        return false;
>+
>+    auto& style = *node.renderStyle();
>+    auto width = style.logicalWidth();
>+    // 1px width or height content is not considered visible.
>+    if (width.isFixed() && width.value() <= 1)
>+        return false;
>+
>+    auto height = style.logicalHeight();
>+    if (height.isFixed() && height.value() <= 1)
>+        return false;
>+
>+    return true;
>+}
>+
> enum class ElementHadRenderer { No, Yes };
> static bool isConsideredClickable(const Element& newlyVisibleElement, ElementHadRenderer hadRenderer)
> {
>@@ -175,7 +193,7 @@ void ContentChangeObserver::didAddTransition(const Element& element, const Anima
>     auto transitionEnd = Seconds { transition.duration() + std::max<double>(0, transition.isDelaySet() ? transition.delay() : 0) };
>     if (transitionEnd > maximumDelayForTransitions)
>         return;
>-    if (!isConsideredHidden(element))
>+    if (!isVisuallyHidden(element))
>         return;
>     // In case of multiple transitions, the first tranistion wins (and it has to produce a visible content change in order to show up as hover).
>     if (m_elementsWithTransition.contains(&element))
>@@ -198,7 +216,7 @@ void ContentChangeObserver::didFinishTransition(const Element& element, CSSPrope
>     callOnMainThread([weakThis = makeWeakPtr(*this), targetElement = makeWeakPtr(element)] {
>         if (!weakThis || !targetElement)
>             return;
>-        if (isConsideredHidden(*targetElement)) {
>+        if (isVisuallyHidden(*targetElement)) {
>             weakThis->adjustObservedState(Event::EndedTransitionButFinalStyleIsNotDefiniteYet);
>             return;
>         }
>@@ -362,7 +380,7 @@ void ContentChangeObserver::willDestroyRenderer(const Element& element)
>         return;
>     LOG_WITH_STREAM(ContentObservation, stream << "willDestroyRenderer element: " << &element);
> 
>-    if (!isConsideredHidden(element))
>+    if (!isVisuallyHidden(element))
>         m_elementsWithDestroyedVisibleRenderer.add(&element);
> }
> 
>@@ -558,13 +576,13 @@ ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, co
>     , m_hadRenderer(element.renderer())
> {
>     if (m_contentChangeObserver.shouldObserveVisibilityChangeForElement(element))
>-        m_wasHidden = isConsideredHidden(m_element);
>+        m_wasHidden = isVisuallyHidden(m_element);
> }
> 
> ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
> {
>     auto changedFromHiddenToVisible = [&] {
>-        return m_wasHidden && !isConsideredHidden(m_element);
>+        return m_wasHidden && isConsideredVisible(m_element);
>     };
> 
>     if (changedFromHiddenToVisible() && isConsideredClickable(m_element, m_hadRenderer ? ElementHadRenderer::Yes : ElementHadRenderer::No))
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.h b/Source/WebCore/page/ios/ContentChangeObserver.h
>index 722740417b2d163bb8cd0ed8a2d54b147e52a2be..8526af8be9add4684fa9f673b3b84c86c3a57f8b 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.h
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.h
>@@ -49,7 +49,8 @@ public:
> 
>     WEBCORE_EXPORT void startContentObservationForDuration(Seconds duration);
>     WKContentChange observedContentChange() const { return m_observedContentState; }
>-    WEBCORE_EXPORT static bool isConsideredHidden(const Node&);
>+    WEBCORE_EXPORT static bool isConsideredVisible(const Node&);
>+    static bool isVisuallyHidden(const Node&);
> 
>     void didInstallDOMTimer(const DOMTimer&, Seconds timeout, bool singleShot);
>     void didRemoveDOMTimer(const DOMTimer&);
>diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>index a4999bd148cfb2925c8a878d86153915be297c49..65db808f4ea953b2e91a115b4afbb23abad659af 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -669,7 +669,7 @@ void WebPage::handleSyntheticClick(Node& nodeRespondingToClick, const WebCore::F
> 
>     auto& respondingDocument = nodeRespondingToClick.document();
>     auto& contentChangeObserver = respondingDocument.contentChangeObserver();
>-    auto targetNodeWentFromHiddenToVisible = contentChangeObserver.hiddenTouchTarget() == &nodeRespondingToClick && !ContentChangeObserver::isConsideredHidden(nodeRespondingToClick);
>+    auto targetNodeWentFromHiddenToVisible = contentChangeObserver.hiddenTouchTarget() == &nodeRespondingToClick && ContentChangeObserver::isConsideredVisible(nodeRespondingToClick);
>     {
>         LOG_WITH_STREAM(ContentObservation, stream << "handleSyntheticClick: node(" << &nodeRespondingToClick << ") " << location);
>         ContentChangeObserver::MouseMovedScope observingScope(respondingDocument);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 8a92fc271e77d14879115a2881c67e8b0e23354f..668c3504d1983581cb1c990b609259203ce43873 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-07-07  Zalan Bujtas  <zalan@apple.com>
>+
>+        [ContentChangeObserver] Difficult to control videos on iqiyi.com as the actions are mouse hover
>+        https://bugs.webkit.org/show_bug.cgi?id=199542
>+        <rdar://problem/51886813>
>+
>+        Reviewed by Simon Fraser.
>+
>+        * fast/events/touch/ios/content-observation/tap-on-1px-height-content-expected.txt: Added.
>+        * fast/events/touch/ios/content-observation/tap-on-1px-height-content.html: Added.
>+        * fast/events/touch/ios/content-observation/tap-on-1px-width-content-expected.txt: Added.
>+        * fast/events/touch/ios/content-observation/tap-on-1px-width-content.html: Added.
>+
> 2019-07-06  Cathie Chen  <cathiechen@igalia.com>
> 
>         Import css/cssom-view testcases from WPT.
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..677e3a31e4d27593aa94dbd637a453af8ceddd78
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content-expected.txt
>@@ -0,0 +1,2 @@
>+PASS if 'clicked' text is shown below.
>+clicked
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content.html b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..0d4e0cd2968519668513c55c06d0f382081762f0
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-height-content.html
>@@ -0,0 +1,54 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>This tests the case when visible content change happens on mouseover but the newly visible (and clickable) content is too small.</title>
>+<script src="../../../../../resources/ui-helper.js"></script>
>+<style>
>+#tapThis {
>+    width: 400px;
>+    height: 400px;
>+    border: 1px solid green;
>+}
>+
>+#becomesVisible {
>+    display: none;
>+    width: 10px;
>+    height: 1px;
>+    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=becomesVisible></div>
>+<pre id=result></pre>
>+<script>
>+tapThis.addEventListener("mouseover", function( event ) {
>+    becomesVisible.style.display = "block";
>+}, false);
>+
>+becomesVisible.addEventListener("click", function( event ) {
>+    result.innerHTML = "clicked hidden";
>+}, false);
>+
>+tapThis.addEventListener("click", function( event ) {   
>+    result.innerHTML = "clicked";
>+    if (window.testRunner)
>+        testRunner.notifyDone();
>+}, false);
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..677e3a31e4d27593aa94dbd637a453af8ceddd78
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content-expected.txt
>@@ -0,0 +1,2 @@
>+PASS if 'clicked' text is shown below.
>+clicked
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content.html b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..ef2c1c13d074dda127a4f80dcde953d23ce18cf2
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/tap-on-1px-width-content.html
>@@ -0,0 +1,54 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>This tests the case when visible content change happens on mouseover but the newly visible (and clickable) content is too small.</title>
>+<script src="../../../../../resources/ui-helper.js"></script>
>+<style>
>+#tapThis {
>+    width: 400px;
>+    height: 400px;
>+    border: 1px solid green;
>+}
>+
>+#becomesVisible {
>+    display: none;
>+    width: 1px;
>+    height: 10px;
>+    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=becomesVisible></div>
>+<pre id=result></pre>
>+<script>
>+tapThis.addEventListener("mouseover", function( event ) {
>+    becomesVisible.style.display = "block";
>+}, false);
>+
>+becomesVisible.addEventListener("click", function( event ) {
>+    result.innerHTML = "clicked hidden";
>+}, 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 199542: 373562 | 373563 | 373596