<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[242661] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/242661">242661</a></dd>
<dt>Author</dt> <dd>zalan@apple.com</dd>
<dt>Date</dt> <dd>2019-03-08 15:51:05 -0800 (Fri, 08 Mar 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>[ContentChangeObserver] Expand "isConsideredClickable" to descendants
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.</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorepageiosContentChangeObservercpp">trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp</a></li>
<li><a href="#trunkSourceWebCorepageiosContentChangeObserverh">trunk/Source/WebCore/page/ios/ContentChangeObserver.h</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfasteventstouchioscontentobservationclickablecontentisinsideacontainerexpectedtxt">trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt</a></li>
<li><a href="#trunkLayoutTestsfasteventstouchioscontentobservationclickablecontentisinsideacontainerhtml">trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (242660 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2019-03-08 23:31:38 UTC (rev 242660)
+++ trunk/LayoutTests/ChangeLog 2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -1,3 +1,14 @@
</span><ins>+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.
+
</ins><span class="cx"> 2019-03-08  Truitt Savell  <tsavell@apple.com>
</span><span class="cx"> 
</span><span class="cx">         (r242595) Layout Tests in imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/* are failing
</span></span></pre></div>
<a id="trunkLayoutTestsfasteventstouchioscontentobservationclickablecontentisinsideacontainerexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt (0 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt                         (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt    2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -0,0 +1,2 @@
</span><ins>+PASS if 'clicked' text is not shown below.
+
</ins></span></pre></div>
<a id="trunkLayoutTestsfasteventstouchioscontentobservationclickablecontentisinsideacontainerhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html (0 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html                         (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html    2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -0,0 +1,66 @@
</span><ins>+<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>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (242660 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2019-03-08 23:31:38 UTC (rev 242660)
+++ trunk/Source/WebCore/ChangeLog      2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -1,5 +1,30 @@
</span><span class="cx"> 2019-03-08  Zalan Bujtas  <zalan@apple.com>
</span><span class="cx"> 
</span><ins>+        [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>
+
</ins><span class="cx">         [ContentChangeObserver] Cleanup adjustObservedState
</span><span class="cx">         https://bugs.webkit.org/show_bug.cgi?id=195470
</span><span class="cx">         <rdar://problem/48717823>
</span></span></pre></div>
<a id="trunkSourceWebCorepageiosContentChangeObservercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp (242660 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp  2019-03-08 23:31:38 UTC (rev 242660)
+++ trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp     2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -33,6 +33,7 @@
</span><span class="cx"> #include "Logging.h"
</span><span class="cx"> #include "NodeRenderStyle.h"
</span><span class="cx"> #include "Page.h"
</span><ins>+#include "RenderDescendantIterator.h"
</ins><span class="cx"> #include "Settings.h"
</span><span class="cx"> 
</span><span class="cx"> namespace WebCore {
</span><span class="lines">@@ -295,12 +296,31 @@
</span><span class="cx">     }
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-static bool isConsideredHidden(const Element& element)
</del><ins>+ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
+    : m_contentChangeObserver(document.contentChangeObserver())
+    , m_element(element)
+    , m_hadRenderer(element.renderer())
</ins><span class="cx"> {
</span><del>-    if (!element.renderStyle())
</del><ins>+    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 (!m_element.renderStyle())
</ins><span class="cx">         return true;
</span><span class="cx"> 
</span><del>-    auto& style = *element.renderStyle();
</del><ins>+    auto& style = *m_element.renderStyle();
</ins><span class="cx">     if (style.display() == DisplayType::None)
</span><span class="cx">         return true;
</span><span class="cx"> 
</span><span class="lines">@@ -329,31 +349,23 @@
</span><span class="cx">     return false;
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
-    : m_contentChangeObserver(document.contentChangeObserver())
-    , m_element(element)
</del><ins>+bool ContentChangeObserver::StyleChangeScope::isConsideredClickable() const
</ins><span class="cx"> {
</span><del>-    auto qualifiesForVisibilityCheck = [&] {
-        if (m_element.isInUserAgentShadowTree())
-            return false;
-        if (!const_cast<Element&>(m_element).willRespondToMouseClickEvents())
-            return false;
</del><ins>+    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())
</ins><span class="cx">         return true;
</span><del>-    };
-
-    auto needsObserving = m_contentChangeObserver.isObservingContentChanges() && !m_contentChangeObserver.hasVisibleChangeState() && qualifiesForVisibilityCheck();
-    if (needsObserving)
-        m_wasHidden = isConsideredHidden(m_element);
</del><ins>+    // 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;
</ins><span class="cx"> }
</span><span class="cx"> 
</span><del>-ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
-{
-    if (!m_wasHidden || isConsideredHidden(m_element))
-        return;
-
-    m_contentChangeObserver.contentVisibilityDidChange();
-}
-
</del><span class="cx"> #if ENABLE(TOUCH_EVENTS)
</span><span class="cx"> ContentChangeObserver::TouchEventScope::TouchEventScope(Document& document, PlatformEvent::Type eventType)
</span><span class="cx">     : m_contentChangeObserver(document.contentChangeObserver())
</span></span></pre></div>
<a id="trunkSourceWebCorepageiosContentChangeObserverh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/ios/ContentChangeObserver.h (242660 => 242661)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/ios/ContentChangeObserver.h    2019-03-08 23:31:38 UTC (rev 242660)
+++ trunk/Source/WebCore/page/ios/ContentChangeObserver.h       2019-03-08 23:51:05 UTC (rev 242661)
</span><span class="lines">@@ -58,9 +58,13 @@
</span><span class="cx">         ~StyleChangeScope();
</span><span class="cx"> 
</span><span class="cx">     private:
</span><ins>+        bool isConsideredHidden() const;
+        bool isConsideredClickable() const;
+
</ins><span class="cx">         ContentChangeObserver& m_contentChangeObserver;
</span><span class="cx">         const Element& m_element;
</span><span class="cx">         bool m_wasHidden { false };
</span><ins>+        bool m_hadRenderer { false };
</ins><span class="cx">     };
</span><span class="cx"> 
</span><span class="cx">     class TouchEventScope {
</span></span></pre>
</div>
</div>

</body>
</html>