[ContentChangeObserver] Check if max-height change triggers visible content change.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2019 06:08:59 +0000 (06:08 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2019 06:08:59 +0000 (06:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195417
<rdar://problem/48680631>

Reviewed by Simon Fraser.

Source/WebCore:

A fixed max-height non-zero value could indicate visible content change. usps.com uses this technique to show menu panes.

Test: fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html

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

LayoutTests:

* fast/events/touch/ios/content-observation/visibility-change-is-max-height-change-expected.txt: Added.
* fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/content-observation/visibility-change-is-max-height-change-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h

index 508c046..c745405 100644 (file)
@@ -1,5 +1,16 @@
 2019-03-07  Zalan Bujtas  <zalan@apple.com>
 
+        [ContentChangeObserver] Check if max-height change triggers visible content change.
+        https://bugs.webkit.org/show_bug.cgi?id=195417
+        <rdar://problem/48680631>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/visibility-change-is-max-height-change-expected.txt: Added.
+        * fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html: Added.
+
+2019-03-07  Zalan Bujtas  <zalan@apple.com>
+
         [ContentChangeObserver] Click event fires immediately on hover menu at Ebbets.com
         https://bugs.webkit.org/show_bug.cgi?id=195397
 
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/visibility-change-is-max-height-change-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/visibility-change-is-max-height-change-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/visibility-change-is-max-height-change.html b/LayoutTests/fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html
new file mode 100644 (file)
index 0000000..073b97d
--- /dev/null
@@ -0,0 +1,59 @@
+<html>
+<head>
+<title>This tests the case when visible content change happens on touchstart</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;
+}
+</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>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    setTimeout(function() {
+        becomesVisible.style.maxHeight = "100px";
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, 10);
+}, false);
+
+becomesVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+}, false);
+</script>
+</body>
+</html>
index 882f683..d84e167 100644 (file)
@@ -1,3 +1,22 @@
+2019-03-07  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Check if max-height change triggers visible content change.
+        https://bugs.webkit.org/show_bug.cgi?id=195417
+        <rdar://problem/48680631>
+
+        Reviewed by Simon Fraser.
+
+        A fixed max-height non-zero value could indicate visible content change. usps.com uses this technique to show menu panes.  
+
+        Test: fast/events/touch/ios/content-observation/visibility-change-is-max-height-change.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::isConsideredHidden):
+        (WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::elementImplicitVisibility): Deleted.
+        * page/ios/ContentChangeObserver.h:
+
 2019-03-07  Yusuke Suzuki  <ysuzuki@apple.com>
 
         Unreviewed, fix failing EWS build for ios-sim
index 648116a..b8abb78 100644 (file)
@@ -278,50 +278,45 @@ void ContentChangeObserver::adjustObservedState(Event event)
     }
 }
 
-static Visibility elementImplicitVisibility(const Element& element)
+static bool isConsideredHidden(const Element& element)
 {
-    auto* renderer = element.renderer();
-    if (!renderer)
-        return Visibility::Visible;
+    if (!element.renderStyle())
+        return true;
+
+    auto& style = *element.renderStyle();
+    if (style.display() == DisplayType::None)
+        return true;
 
-    auto& style = renderer->style();
+    if (style.visibility() == Visibility::Hidden)
+        return true;
 
     auto width = style.width();
     auto height = style.height();
-    if ((width.isFixed() && width.value() <= 0) || (height.isFixed() && height.value() <= 0))
-        return Visibility::Hidden;
+    if ((width.isFixed() && !width.value()) || (height.isFixed() && !height.value()))
+        return true;
 
     auto top = style.top();
     auto left = style.left();
+    // FIXME: This is trying to check if the element is outside of the viewport. This is incorrect for many reasons.
     if (left.isFixed() && width.isFixed() && -left.value() >= width.value())
-        return Visibility::Hidden;
+        return true;
 
     if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
-        return Visibility::Hidden;
-    return Visibility::Visible;
+        return true;
+
+    // FIXME: Check for other cases like zero height with overflow hidden.
+    auto maxHeight = style.maxHeight();
+    if (maxHeight.isFixed() && !maxHeight.value())
+        return true;
+
+    return false;
 }
 
 ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
     : m_contentChangeObserver(document.contentChangeObserver())
     , m_element(element)
-    , m_needsObserving(m_contentChangeObserver.isObservingContentChanges() && !m_contentChangeObserver.hasVisibleChangeState())
-{
-    if (m_needsObserving) {
-        m_previousDisplay = element.renderStyle() ? element.renderStyle()->display() : DisplayType::None;
-        m_previousVisibility = element.renderStyle() ? element.renderStyle()->visibility() : Visibility::Hidden;
-        m_previousImplicitVisibility = elementImplicitVisibility(element);
-    }
-}
-
-ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
 {
-    if (!m_needsObserving)
-        return;
-
-    auto* style = m_element.renderStyle();
     auto qualifiesForVisibilityCheck = [&] {
-        if (!style)
-            return false;
         if (m_element.isInUserAgentShadowTree())
             return false;
         if (!const_cast<Element&>(m_element).willRespondToMouseClickEvents())
@@ -329,13 +324,17 @@ ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
         return true;
     };
 
-    if (!qualifiesForVisibilityCheck())
+    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;
 
-    if ((m_previousDisplay == DisplayType::None && style->display() != DisplayType::None)
-        || (m_previousVisibility == Visibility::Hidden && style->visibility() != Visibility::Hidden)
-        || (m_previousImplicitVisibility == Visibility::Hidden && elementImplicitVisibility(m_element) == Visibility::Visible))
-        m_contentChangeObserver.contentVisibilityDidChange();
+    m_contentChangeObserver.contentVisibilityDidChange();
 }
 
 #if ENABLE(TOUCH_EVENTS)
index e7db158..350997e 100644 (file)
@@ -60,10 +60,7 @@ public:
     private:
         ContentChangeObserver& m_contentChangeObserver;
         const Element& m_element;
-        bool m_needsObserving { false };
-        DisplayType m_previousDisplay;
-        Visibility m_previousVisibility;
-        Visibility m_previousImplicitVisibility;
+        bool m_wasHidden { false };
     };
 
     class TouchEventScope {