[ContentChangeObserver] Expand "isConsideredClickable" to descendants
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2019 23:51:05 +0000 (23:51 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2019 23:51:05 +0000 (23:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195478
<rdar://problem/48724935>

Reviewed by Simon Fraser.

Source/WebCore:

In StyleChangeScope we try to figure out whether newly visible content should stick (menu panes etc) by checking if it is clickable.
This works fine as long as all the visible elements are gaining new renderers through this style update processs.
However when an element becomes visible by a change other than display: (not)none, it's not sufficient to just check the element itself,
since it might not respond to click at all, while its descendants do.
A concrete example is a max-height value change on usps.com, where the max-height is on a container (menu pane).
This container itself is not clickable while most of its children are (menu items).

Test: fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html

* page/ios/ContentChangeObserver.cpp:
(WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
(WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
(WebCore::ContentChangeObserver::StyleChangeScope::isConsideredHidden const):
(WebCore::ContentChangeObserver::StyleChangeScope::isConsideredClickable const):
(WebCore::isConsideredHidden): Deleted.
* page/ios/ContentChangeObserver.h:

LayoutTests:

* fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt: Added.
* fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242661 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h

index 2ada2a2..56235c4 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-08  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Expand "isConsideredClickable" to descendants
+        https://bugs.webkit.org/show_bug.cgi?id=195478
+        <rdar://problem/48724935>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt: Added.
+        * fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html: Added.
+
 2019-03-08  Truitt Savell  <tsavell@apple.com>
 
         (r242595) Layout Tests in imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/* are failing
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt
new file mode 100644 (file)
index 0000000..66db4c9
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is not shown below.
+
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html b/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html
new file mode 100644 (file)
index 0000000..967f139
--- /dev/null
@@ -0,0 +1,66 @@
+<html>
+<head>
+<title>This tests the case when we've got all the renderers constructed before they become visible and the container is not clickable.</title>
+<script src="../../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#becomesVisible {
+    max-height: 0px;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+    overflow: hidden;
+}
+
+#becomesVisibleChild {
+    width: 50px;
+    height: 50px;
+    background-color: blue;
+}
+
+</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 not shown below.</div>
+<div id=becomesVisible><div id=becomesVisibleChild></div></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    becomesVisible.style.maxHeight = "100px";
+    document.body.offsetHeight;
+    if (window.testRunner)
+        testRunner.notifyDone();
+}, false);
+
+becomesVisibleChild.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+}, false);
+</script>
+</body>
+</html>
index 81c389d..5a530fa 100644 (file)
@@ -1,5 +1,30 @@
 2019-03-08  Zalan Bujtas  <zalan@apple.com>
 
+        [ContentChangeObserver] Expand "isConsideredClickable" to descendants
+        https://bugs.webkit.org/show_bug.cgi?id=195478
+        <rdar://problem/48724935>
+
+        Reviewed by Simon Fraser.
+
+        In StyleChangeScope we try to figure out whether newly visible content should stick (menu panes etc) by checking if it is clickable.
+        This works fine as long as all the visible elements are gaining new renderers through this style update processs.
+        However when an element becomes visible by a change other than display: (not)none, it's not sufficient to just check the element itself,
+        since it might not respond to click at all, while its descendants do.
+        A concrete example is a max-height value change on usps.com, where the max-height is on a container (menu pane).
+        This container itself is not clickable while most of its children are (menu items).    
+
+        Test: fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::isConsideredHidden const):
+        (WebCore::ContentChangeObserver::StyleChangeScope::isConsideredClickable const):
+        (WebCore::isConsideredHidden): Deleted.
+        * page/ios/ContentChangeObserver.h:
+
+2019-03-08  Zalan Bujtas  <zalan@apple.com>
+
         [ContentChangeObserver] Cleanup adjustObservedState
         https://bugs.webkit.org/show_bug.cgi?id=195470
         <rdar://problem/48717823>
index b1b1af0..bda8258 100644 (file)
@@ -33,6 +33,7 @@
 #include "Logging.h"
 #include "NodeRenderStyle.h"
 #include "Page.h"
+#include "RenderDescendantIterator.h"
 #include "Settings.h"
 
 namespace WebCore {
@@ -295,12 +296,31 @@ void ContentChangeObserver::adjustObservedState(Event event)
     }
 }
 
-static bool isConsideredHidden(const Element& element)
+ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
+    : m_contentChangeObserver(document.contentChangeObserver())
+    , m_element(element)
+    , m_hadRenderer(element.renderer())
+{
+    if (m_contentChangeObserver.isObservingContentChanges() && !m_contentChangeObserver.hasVisibleChangeState())
+        m_wasHidden = isConsideredHidden();
+}
+
+ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
+{
+    auto changedFromHiddenToVisible = [&] {
+        return m_wasHidden && !isConsideredHidden();
+    };
+    
+    if (changedFromHiddenToVisible() && isConsideredClickable())
+        m_contentChangeObserver.contentVisibilityDidChange();
+}
+
+bool ContentChangeObserver::StyleChangeScope::isConsideredHidden() const
 {
-    if (!element.renderStyle())
+    if (!m_element.renderStyle())
         return true;
 
-    auto& style = *element.renderStyle();
+    auto& style = *m_element.renderStyle();
     if (style.display() == DisplayType::None)
         return true;
 
@@ -329,29 +349,21 @@ static bool isConsideredHidden(const Element& element)
     return false;
 }
 
-ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
-    : m_contentChangeObserver(document.contentChangeObserver())
-    , m_element(element)
+bool ContentChangeObserver::StyleChangeScope::isConsideredClickable() const
 {
-    auto qualifiesForVisibilityCheck = [&] {
-        if (m_element.isInUserAgentShadowTree())
-            return false;
-        if (!const_cast<Element&>(m_element).willRespondToMouseClickEvents())
-            return false;
+    if (m_element.isInUserAgentShadowTree())
+        return false;
+    if (!m_hadRenderer)
+        return const_cast<Element&>(m_element).willRespondToMouseClickEvents();
+    ASSERT(m_element.renderer());
+    if (const_cast<Element&>(m_element).willRespondToMouseClickEvents())
         return true;
-    };
-
-    auto needsObserving = m_contentChangeObserver.isObservingContentChanges() && !m_contentChangeObserver.hasVisibleChangeState() && qualifiesForVisibilityCheck();
-    if (needsObserving)
-        m_wasHidden = isConsideredHidden(m_element);
-}
-
-ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
-{
-    if (!m_wasHidden || isConsideredHidden(m_element))
-        return;
-
-    m_contentChangeObserver.contentVisibilityDidChange();
+    // 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>(*m_element.renderer())) {
+        if (descendant.element()->willRespondToMouseClickEvents())
+            return true;
+    }
+    return false;
 }
 
 #if ENABLE(TOUCH_EVENTS)
index 5234a9b..dc25bed 100644 (file)
@@ -58,9 +58,13 @@ public:
         ~StyleChangeScope();
 
     private:
+        bool isConsideredHidden() const;
+        bool isConsideredClickable() const;
+
         ContentChangeObserver& m_contentChangeObserver;
         const Element& m_element;
         bool m_wasHidden { false };
+        bool m_hadRenderer { false };
     };
 
     class TouchEventScope {