Changeset 242661 in webkit

Timestamp:

Mar 8, 2019, 3:51:05 PM (7 years ago)

Author:

Alan Bujtas

Message:

[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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container-expected.txt (added)
• LayoutTests/fast/events/touch/ios/content-observation/clickable-content-is-inside-a-container.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/ios/ContentChangeObserver.cpp (modified) (3 diffs)
• Source/WebCore/page/ios/ContentChangeObserver.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/ios/ContentChangeObserver.cpp

@@ -34 +34 @@
 #include "NodeRenderStyle.h"
 #include "Page.h"
+#include "RenderDescendantIterator.h"
 #include "Settings.h"
 
@@ -296 +297 @@
 }
 
-static bool isConsideredHidden(const Element& element)
-{
-    if (!element.renderStyle())
-        return true;
-
-    auto& style = *element.renderStyle();
+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 (!m_element.renderStyle())
+        return true;
+
+    auto& style = *m_element.renderStyle();
     if (style.display() == DisplayType::None)
         return true;
@@ -330 +350 @@
 }
 
-ContentChangeObserver::StyleChangeScope::StyleChangeScope(Document& document, const Element& element)
-    : m_contentChangeObserver(document.contentChangeObserver())
-    , m_element(element)
-{
-    auto qualifiesForVisibilityCheck = [&] {
-        if (m_element.isInUserAgentShadowTree())
-            return false;
-        if (!const_cast<Element&>(m_element).willRespondToMouseClickEvents())
-            return false;
-        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();
+bool ContentChangeObserver::StyleChangeScope::isConsideredClickable() const
+{
+    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;
+    // 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;
 }
 

trunk/Source/WebCore/page/ios/ContentChangeObserver.h

@@ -59 +59 @@
 
     private:
+        bool isConsideredHidden() const;
+        bool isConsideredClickable() const;
+
         ContentChangeObserver& m_contentChangeObserver;
         const Element& m_element;
         bool m_wasHidden { false };
+        bool m_hadRenderer { false };
     };