[ContentChangeObserver] Track hidden elements only while transitioning.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Mar 2019 18:38:39 +0000 (18:38 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Mar 2019 18:38:39 +0000 (18:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196050
<rdar://problem/49092037>

Reviewed by Simon Fraser.

Use the existing isConsideredHidden() logic to decide whether the current transition should be tracked.

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

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

Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h

index 7def03c..d151148 100644 (file)
@@ -1,5 +1,23 @@
 2019-03-21  Zalan Bujtas  <zalan@apple.com>
 
+        [ContentChangeObserver] Track hidden elements only while transitioning.
+        https://bugs.webkit.org/show_bug.cgi?id=196050
+        <rdar://problem/49092037>
+
+        Reviewed by Simon Fraser.
+
+        Use the existing isConsideredHidden() logic to decide whether the current transition should be tracked.
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::isConsideredHidden):
+        (WebCore::ContentChangeObserver::didAddTransition):
+        (WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::~StyleChangeScope):
+        (WebCore::ContentChangeObserver::StyleChangeScope::isConsideredHidden const): Deleted.
+        * page/ios/ContentChangeObserver.h:
+
+2019-03-21  Zalan Bujtas  <zalan@apple.com>
+
         [ContentChangeObserver] Add support for observing implicit transitions
         https://bugs.webkit.org/show_bug.cgi?id=195914
         <rdar://problem/49091959>
index 9b7c728..b5e1c2e 100644 (file)
@@ -42,6 +42,43 @@ namespace WebCore {
 static const Seconds maximumDelayForTimers { 300_ms };
 static const Seconds maximumDelayForTransitions { 300_ms };
 
+static bool isConsideredHidden(const Element& element)
+{
+    if (!element.renderStyle())
+        return true;
+
+    auto& style = *element.renderStyle();
+    if (style.display() == DisplayType::None)
+        return true;
+
+    if (style.visibility() == Visibility::Hidden)
+        return true;
+
+    auto width = style.logicalWidth();
+    auto height = style.logicalHeight();
+    if ((width.isFixed() && !width.value()) || (height.isFixed() && !height.value()))
+        return true;
+
+    auto top = style.logicalTop();
+    auto left = style.logicalLeft();
+    // 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 true;
+    if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
+        return true;
+
+    // It's a common technique used to position content offscreen.
+    if (style.hasOutOfFlowPosition() && left.isFixed() && left.value() <= -999)
+        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::ContentChangeObserver(Document& document)
     : m_document(document)
     , m_contentObservationTimer([this] { completeDurationBasedContentObservation(); })
@@ -99,6 +136,8 @@ 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))
+        return;
     LOG_WITH_STREAM(ContentObservation, stream << "didAddTransition: transition created on " << &element << " (" << transitionEnd.milliseconds() << "ms).");
 
     m_elementsWithTransition.add(&element);
@@ -384,56 +423,19 @@ ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, co
     , m_hadRenderer(element.renderer())
 {
     if (m_contentChangeObserver.isObservingContentChanges() && !m_contentChangeObserver.hasVisibleChangeState())
-        m_wasHidden = isConsideredHidden();
+        m_wasHidden = isConsideredHidden(m_element);
 }
 
 ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
 {
     auto changedFromHiddenToVisible = [&] {
-        return m_wasHidden && !isConsideredHidden();
+        return m_wasHidden && !isConsideredHidden(m_element);
     };
 
     if (changedFromHiddenToVisible() && isConsideredClickable())
         m_contentChangeObserver.contentVisibilityDidChange();
 }
 
-bool ContentChangeObserver::StyleChangeScope::isConsideredHidden() const
-{
-    if (!m_element.renderStyle())
-        return true;
-
-    auto& style = *m_element.renderStyle();
-    if (style.display() == DisplayType::None)
-        return true;
-
-    if (style.visibility() == Visibility::Hidden)
-        return true;
-
-    auto width = style.logicalWidth();
-    auto height = style.logicalHeight();
-    if ((width.isFixed() && !width.value()) || (height.isFixed() && !height.value()))
-        return true;
-
-    auto top = style.logicalTop();
-    auto left = style.logicalLeft();
-    // 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 true;
-    if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
-        return true;
-
-    // It's a common technique used to position content offscreen.
-    if (style.hasOutOfFlowPosition() && left.isFixed() && left.value() <= -999)
-        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;
-}
-
 bool ContentChangeObserver::StyleChangeScope::isConsideredClickable() const
 {
     if (m_element.isInUserAgentShadowTree())
index 8eb0b05..fce8676 100644 (file)
@@ -66,7 +66,6 @@ public:
         ~StyleChangeScope();
 
     private:
-        bool isConsideredHidden() const;
         bool isConsideredClickable() const;
 
         ContentChangeObserver& m_contentChangeObserver;