Attachment 373247 Details for Bug 199349 – Patch

[patch] Patch

bug-199349-20190701121304.patch (text/plain), 12.75 KB, created by zalan on 2019-07-01 12:13:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-07-01 12:13:05 PDT

Size: 12.75 KB

patch

obsolete

>Subversion Revision: 247013
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index fb36b4877b8486b95317e51d80788fda076a6533..86e1e2eb0dacae094cda7d6fe0396ad120fb4f9e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-07-01  Zalan Bujtas  <zalan@apple.com>
>+
>+        [iPadOS] Tapping on the bottom part of youtube video behaves as if controls were visible
>+        https://bugs.webkit.org/show_bug.cgi?id=199349
>+        <rdar://problem/51955744>
>+
>+        Reviewed by Simon Fraser.
>+
>+        Synthetic click event should not be dispatched to a node that is initially hidden (by opacity: 0) and becomes visible by the touchStart event.
>+        While this behaves different from macOS where opacity: 0; content is "clickable", it impoves usability on certain sites like YouTube.com. 
>+
>+        Test: fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2.html
>+
>+        * dom/Node.cpp:
>+        (WebCore::Node::defaultEventHandler):
>+        * page/ios/ContentChangeObserver.cpp:
>+        (WebCore::ContentChangeObserver::isConsideredHidden):
>+        (WebCore::ContentChangeObserver::reset):
>+        (WebCore::isConsideredHidden): Deleted.
>+        * page/ios/ContentChangeObserver.h:
>+        (WebCore::ContentChangeObserver::setHiddenTouchTarget):
>+        (WebCore::ContentChangeObserver::resetHiddenTouchTarget):
>+        (WebCore::ContentChangeObserver::hiddenTouchTarget const):
>+
> 2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index c4a9acc71316eac20e11be74a28399792d941eb5..0e6cc9e6f8e4db044c3dac40d09a39bbcd0409ea 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-07-01  Zalan Bujtas  <zalan@apple.com>
>+
>+        Tapping on the bottom part of youtube video behaves as if controls were visible
>+        https://bugs.webkit.org/show_bug.cgi?id=199349
>+        <rdar://problem/51955744>
>+
>+        Reviewed by Simon Fraser.
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::handleTouchEvent):
>+        * WebProcess/WebPage/WebPage.h:
>+        * WebProcess/WebPage/ios/WebPageIOS.mm:
>+        (WebKit::WebPage::handleSyntheticClick):
>+        (WebKit::WebPage::completePendingSyntheticClickForContentChangeObserver):
>+        (WebKit::WebPage::completeSyntheticClick):
>+        (WebKit::WebPage::potentialTapAtPosition):
>+
> 2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
>index fae18a95d8ba1ac2f57a2d22a47deb8c0ae3ccde..6534378a22f1324eb0e194138cc84e3450dc6a4f 100644
>--- a/Source/WebCore/dom/Node.cpp
>+++ b/Source/WebCore/dom/Node.cpp
>@@ -32,6 +32,9 @@
> #include "CommonVM.h"
> #include "ComposedTreeAncestorIterator.h"
> #include "ContainerNodeAlgorithms.h"
>+#if PLATFORM(IOS_FAMILY)
>+#include "ContentChangeObserver.h"
>+#endif
> #include "ContextMenuController.h"
> #include "DOMWindow.h"
> #include "DataTransfer.h"
>@@ -2452,6 +2455,15 @@ void Node::defaultEventHandler(Event& event)
>                 frame->eventHandler().defaultWheelEventHandler(startNode, downcast<WheelEvent>(event));
> #if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)
>     } else if (is<TouchEvent>(event) && eventNames().isTouchRelatedEventType(document(), eventType)) {
>+        // 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))
>+                contentChangeObserver.setHiddenTouchTarget(downcast<Element>(*this));
>+            else
>+                contentChangeObserver.resetHiddenTouchTarget();
>+        }
>+
>         RenderObject* renderer = this->renderer();
>         while (renderer && (!is<RenderBox>(*renderer) || !downcast<RenderBox>(*renderer).canBeScrolledAndHasScrollableArea()))
>             renderer = renderer->parent();
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.cpp b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>index 803c4599fd688febaae409ba421ef2a53a6ea0c2..089973cc261bbb19837fa5276dab9cd059db8cdd 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.cpp
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.cpp
>@@ -43,12 +43,12 @@ namespace WebCore {
> static const Seconds maximumDelayForTimers { 400_ms };
> static const Seconds maximumDelayForTransitions { 300_ms };
> 
>-static bool isConsideredHidden(const Element& element)
>+bool ContentChangeObserver::isConsideredHidden(const Node& node)
> {
>-    if (!element.renderStyle())
>+    if (!node.renderStyle())
>         return true;
> 
>-    auto& style = *element.renderStyle();
>+    auto& style = *node.renderStyle();
>     if (style.display() == DisplayType::None)
>         return true;
> 
>@@ -80,6 +80,14 @@ static bool isConsideredHidden(const Element& element)
>     if (maxHeight.isFixed() && !maxHeight.value())
>         return true;
> 
>+    // Special case opacity, because a descendant with non-zero opacity should still be considered hidden when one of its ancetors has opacity: 0;
>+    // YouTube.com has this setup with the bottom control bar.
>+    constexpr static unsigned numberOfAncestorsToCheckForOpacity = 4;
>+    unsigned i = 0;
>+    for (auto* parent = node.parentNode(); parent && i < numberOfAncestorsToCheckForOpacity; parent = parent->parentNode(), ++i) {
>+        if (!parent->renderStyle() || !parent->renderStyle()->opacity())
>+            return true;
>+    }
>     return false;
> }
> 
>@@ -324,6 +332,7 @@ void ContentChangeObserver::reset()
> 
>     m_contentObservationTimer.stop();
>     m_elementsWithDestroyedVisibleRenderer.clear();
>+    resetHiddenTouchTarget();
> }
> 
> void ContentChangeObserver::didSuspendActiveDOMObjects()
>@@ -579,6 +588,7 @@ ContentChangeObserver::MouseMovedScope::MouseMovedScope(Document& document)
> ContentChangeObserver::MouseMovedScope::~MouseMovedScope()
> {
>     m_contentChangeObserver.mouseMovedDidFinish();
>+    m_contentChangeObserver.resetHiddenTouchTarget();
> }
> 
> ContentChangeObserver::StyleRecalcScope::StyleRecalcScope(Document& document)
>diff --git a/Source/WebCore/page/ios/ContentChangeObserver.h b/Source/WebCore/page/ios/ContentChangeObserver.h
>index 1ef64c2b343e89ae76819a0e2d6905d7918f8461..32da407e6d43e17ce2427aab6d69250e7c3026d6 100644
>--- a/Source/WebCore/page/ios/ContentChangeObserver.h
>+++ b/Source/WebCore/page/ios/ContentChangeObserver.h
>@@ -35,6 +35,7 @@
> #include "WKContentObservation.h"
> #include <wtf/HashSet.h>
> #include <wtf/Seconds.h>
>+#include <wtf/WeakPtr.h>
> 
> namespace WebCore {
> 
>@@ -48,6 +49,7 @@ public:
> 
>     WEBCORE_EXPORT void startContentObservationForDuration(Seconds duration);
>     WKContentChange observedContentChange() const { return m_observedContentState; }
>+    WEBCORE_EXPORT static bool isConsideredHidden(const Node&);
> 
>     void didInstallDOMTimer(const DOMTimer&, Seconds timeout, bool singleShot);
>     void didRemoveDOMTimer(const DOMTimer&);
>@@ -65,6 +67,10 @@ public:
> 
>     void willDestroyRenderer(const Element&);
> 
>+    void setHiddenTouchTarget(Element& targetElement) { m_hiddenTouchTargetElement = makeWeakPtr(targetElement); }
>+    void resetHiddenTouchTarget() { m_hiddenTouchTargetElement = { }; }
>+    Element* hiddenTouchTarget() const { return m_hiddenTouchTargetElement.get(); }
>+
>     class StyleChangeScope {
>     public:
>         StyleChangeScope(Document&, const Element&);
>@@ -204,6 +210,7 @@ private:
>     HashSet<const Element*> m_elementsWithTransition;
>     HashSet<const Element*> m_elementsWithDestroyedVisibleRenderer;
>     WKContentChange m_observedContentState { WKContentNoChange };
>+    WeakPtr<Element> m_hiddenTouchTargetElement;
>     bool m_touchEventIsBeingDispatched { false };
>     bool m_isWaitingForStyleRecalc { false };
>     bool m_isInObservedStyleRecalc { false };
>diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>index e332e9b6fba3035f29606b4082bc3eb995363ad0..a138f457aa431337b6f70cb3b423f88e9d71bb35 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -664,18 +664,23 @@ void WebPage::handleSyntheticClick(Node& nodeRespondingToClick, const WebCore::F
>     }
> 
>     auto& respondingDocument = nodeRespondingToClick.document();
>+    auto& contentChangeObserver = respondingDocument.contentChangeObserver();
>+    auto targetNodeisConsideredHidden = contentChangeObserver.hiddenTouchTarget() == &nodeRespondingToClick || ContentChangeObserver::isConsideredHidden(nodeRespondingToClick);
>     {
>         LOG_WITH_STREAM(ContentObservation, stream << "handleSyntheticClick: node(" << &nodeRespondingToClick << ") " << location);
>         ContentChangeObserver::MouseMovedScope observingScope(respondingDocument);
>         auto& mainFrame = m_page->mainFrame();
>         dispatchSyntheticMouseMove(mainFrame, location, modifiers, pointerId);
>         mainFrame.document()->updateStyleIfNeeded();
>+        if (m_isClosed)
>+            return;
>     }
> 
>-    if (m_isClosed)
>+    if (targetNodeisConsideredHidden) {
>+        LOG(ContentObservation, "handleSyntheticClick: target node is considered hidden -> hover.");
>         return;
>+    }
> 
>-    auto& contentChangeObserver = respondingDocument.contentChangeObserver();
>     auto observedContentChange = contentChangeObserver.observedContentChange();
>     auto targetNodeTriggersClick = nodeAlwaysTriggersClick(nodeRespondingToClick);
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 0a4eeb9e6881e34e2ad055c99731e3285adcd5fa..77ccc1aa74e2be9b918fb09254104ba23c7d52ac 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-07-01  Zalan Bujtas  <zalan@apple.com>
>+
>+        Tapping on the bottom part of youtube video behaves as if controls were visible
>+        https://bugs.webkit.org/show_bug.cgi?id=199349
>+        <rdar://problem/51955744>
>+
>+        Reviewed by Simon Fraser.
>+
>+        * fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2-expected.txt: Added.
>+        * fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2.html: Added.
>+
> 2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..a35f2c8cf410f8da7b5ca7a054fdd2a5f77865c0
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2-expected.txt
>@@ -0,0 +1,3 @@
>+
>+PASS if 'clicked' text is not shown below.
>+
>diff --git a/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2.html b/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..be39d498bd412ecdb9fdb32ec60f4257813bdd52
>--- /dev/null
>+++ b/LayoutTests/fast/events/touch/ios/content-observation/opacity-change-happens-on-touchstart-with-transition2.html
>@@ -0,0 +1,49 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>This tests the case when the touch target is initially hidden.</title>
>+<script src="../../../../../resources/basic-gestures.js"></script>
>+<style>
>+#tapthis {
>+    opacity: 0;
>+    width: 400px;
>+    height: 400px;
>+    border: 1px solid green;
>+    transition: opacity 20ms ease-out 10ms;
>+}
>+</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()">
>+<button id=tapthis></button>
>+<div>PASS if 'clicked' text is not shown below.</div>
>+<pre id=result></pre>
>+<script>
>+tapthis.addEventListener("touchstart", function( event ) {
>+    tapthis.style.opacity = "1";
>+    if (window.testRunner)
>+        setTimeout("testRunner.notifyDone()", 250);
>+}, false);
>+
>+tapthis.addEventListener("click", function( event ) {   
>+    result.innerHTML = "clicked";
>+}, false);
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 199349: 373192 | 373202 | 373207 | 373208 | 373247