[ContentChangeObserver] Ignore reconstructed renderers when checking for visibility...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Apr 2019 19:43:51 +0000 (19:43 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Apr 2019 19:43:51 +0000 (19:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196483
<rdar://problem/49288174>

Reviewed by Simon Fraser.

Source/WebCore:

This patch fixes the cases when the content gets reconstructed in a way that existing and visible elements gain
new renderers within one style recalc. We failed to recognize such cases and ended up detecting the newly constructed renderers
as "visible change" thereby triggering hover.

Test: fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html

* page/ios/ContentChangeObserver.cpp:
(WebCore::ContentChangeObserver::renderTreeUpdateDidStart):
(WebCore::ContentChangeObserver::renderTreeUpdateDidFinish):
(WebCore::ContentChangeObserver::reset):
(WebCore::ContentChangeObserver::willDestroyRenderer):
(WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
(WebCore::ContentChangeObserver::RenderTreeUpdateScope::RenderTreeUpdateScope):
(WebCore::ContentChangeObserver::RenderTreeUpdateScope::~RenderTreeUpdateScope):
* page/ios/ContentChangeObserver.h:
(WebCore::ContentChangeObserver::visibleRendererWasDestroyed const):
* rendering/updating/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::updateRenderTree):
(WebCore::RenderTreeUpdater::tearDownRenderers):

LayoutTests:

* fast/events/touch/ios/content-observation/visible-content-gains-new-renderer-expected.txt: Added.
* fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer.html [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer.html [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h
Source/WebCore/rendering/updating/RenderTreeUpdater.cpp

index 67a3638..e18bbd9 100644 (file)
@@ -1,3 +1,14 @@
+2019-04-02  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Ignore reconstructed renderers when checking for visibility change
+        https://bugs.webkit.org/show_bug.cgi?id=196483
+        <rdar://problem/49288174>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/content-observation/visible-content-gains-new-renderer-expected.txt: Added.
+        * fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html: Added.
+
 2019-04-02  Shawn Roberts  <sroberts@apple.com>
 
         accessibility/mac/press-not-work-for-disabled-menu-list.html is a flaky failure
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer-expected.txt
new file mode 100644 (file)
index 0000000..f905f50
--- /dev/null
@@ -0,0 +1,3 @@
+PASS if 'clicked' text is not shown below.
+content
+
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer.html b/LayoutTests/fast/events/touch/ios/content-observation/non-visible-becomes-visible-and-gains-new-renderer.html
new file mode 100644 (file)
index 0000000..f3a4204
--- /dev/null
@@ -0,0 +1,60 @@
+<html>
+<head>
+<title>This tests the case when non-visible content gets rebuilt and visible and should be considered as new content</title>
+<script src="../../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#becomesVisible {
+    visibility: hidden;
+    display: block;
+    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=visibleParent><div id=becomesVisible>content</div></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    visibleParent.style.display = "inline-block";
+    becomesVisible.style.visibility = "visible";
+    document.body.offsetHeight;
+    if (window.testRunner)
+       setTimeout(testRunner.notifyDone(), 50);
+}, false);
+
+becomesVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+}, false);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer-expected.txt
new file mode 100644 (file)
index 0000000..72f1f97
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is shown below.
+ clicked
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer.html b/LayoutTests/fast/events/touch/ios/content-observation/non-visible-content-gains-new-renderer.html
new file mode 100644 (file)
index 0000000..9301e21
--- /dev/null
@@ -0,0 +1,59 @@
+<html>
+<head>
+<title>This tests the case when non-visible content gets rebuilt and should not be considered as new content</title>
+<script src="../../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#notVisible {
+    visibility: hidden;
+    display: block;
+    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 shown below.</div>
+<div id=visibleParent><div id=notVisible>content</div></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    visibleParent.style.display = "inline-block";
+    document.body.offsetHeight;
+}, false);
+
+notVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+    if (window.testRunner)
+       testRunner.notifyDone();
+}, false);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer-expected.txt b/LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer-expected.txt
new file mode 100644 (file)
index 0000000..aa3fa04
--- /dev/null
@@ -0,0 +1,3 @@
+PASS if 'clicked' text is shown below.
+content
+ clicked
diff --git a/LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html b/LayoutTests/fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html
new file mode 100644 (file)
index 0000000..48220fd
--- /dev/null
@@ -0,0 +1,58 @@
+<html>
+<head>
+<title>This tests the case when visible content gets rebuilt and should not be considered as new content</title>
+<script src="../../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#alreadyVisible {
+    display: block;
+    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 shown below.</div>
+<div id=visibleParent><div id=alreadyVisible>content</div></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    visibleParent.style.display = "inline-block";
+    document.body.offsetHeight;
+}, false);
+
+alreadyVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+    if (window.testRunner)
+       testRunner.notifyDone();
+}, false);
+</script>
+</body>
+</html>
index efb2c88..73484fe 100644 (file)
@@ -1,3 +1,31 @@
+2019-04-02  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Ignore reconstructed renderers when checking for visibility change
+        https://bugs.webkit.org/show_bug.cgi?id=196483
+        <rdar://problem/49288174>
+
+        Reviewed by Simon Fraser.
+
+        This patch fixes the cases when the content gets reconstructed in a way that existing and visible elements gain
+        new renderers within one style recalc. We failed to recognize such cases and ended up detecting the newly constructed renderers
+        as "visible change" thereby triggering hover.
+
+        Test: fast/events/touch/ios/content-observation/visible-content-gains-new-renderer.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::ContentChangeObserver::renderTreeUpdateDidStart):
+        (WebCore::ContentChangeObserver::renderTreeUpdateDidFinish):
+        (WebCore::ContentChangeObserver::reset):
+        (WebCore::ContentChangeObserver::willDestroyRenderer):
+        (WebCore::ContentChangeObserver::StyleChangeScope::StyleChangeScope):
+        (WebCore::ContentChangeObserver::RenderTreeUpdateScope::RenderTreeUpdateScope):
+        (WebCore::ContentChangeObserver::RenderTreeUpdateScope::~RenderTreeUpdateScope):
+        * page/ios/ContentChangeObserver.h:
+        (WebCore::ContentChangeObserver::visibleRendererWasDestroyed const):
+        * rendering/updating/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateRenderTree):
+        (WebCore::RenderTreeUpdater::tearDownRenderers):
+
 2019-04-02  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [CMake] WEBKIT_MAKE_FORWARDING_HEADERS shouldn't use POST_BUILD to copy generated headers
index 41381de..5f08a28 100644 (file)
@@ -248,6 +248,28 @@ void ContentChangeObserver::styleRecalcDidFinish()
     adjustObservedState(Event::EndedStyleRecalc);
 }
 
+void ContentChangeObserver::renderTreeUpdateDidStart()
+{
+    if (!m_document.settings().contentChangeObserverEnabled())
+        return;
+    if (!isObservingContentChanges())
+        return;
+
+    LOG(ContentObservation, "renderTreeUpdateDidStart: RenderTree update started");
+    m_isInObservedRenderTreeUpdate = true;
+    m_elementsWithDestroyedVisibleRenderer.clear();
+}
+
+void ContentChangeObserver::renderTreeUpdateDidFinish()
+{
+    if (!m_isInObservedRenderTreeUpdate)
+        return;
+
+    LOG(ContentObservation, "renderTreeUpdateDidStart: RenderTree update finished");
+    m_isInObservedRenderTreeUpdate = false;
+    m_elementsWithDestroyedVisibleRenderer.clear();
+}
+
 void ContentChangeObserver::stopObservingPendingActivities()
 {
     setShouldObserveNextStyleRecalc(false);
@@ -265,10 +287,12 @@ void ContentChangeObserver::reset()
 
     m_touchEventIsBeingDispatched = false;
     m_isInObservedStyleRecalc = false;
+    m_isInObservedRenderTreeUpdate = false;
     m_observedDomTimerIsBeingExecuted = false;
     m_mouseMovedEventIsBeingDispatched = false;
 
     m_contentObservationTimer.stop();
+    m_elementsWithDestroyedVisibleRenderer.clear();
 }
 
 void ContentChangeObserver::didSuspendActiveDOMObjects()
@@ -283,6 +307,20 @@ void ContentChangeObserver::willDetachPage()
     reset();
 }
 
+void ContentChangeObserver::willDestroyRenderer(const Element& element)
+{ 
+    if (!m_document.settings().contentChangeObserverEnabled())
+        return;
+    if (!m_isInObservedRenderTreeUpdate)
+        return;
+    if (hasVisibleChangeState())
+        return;
+    LOG_WITH_STREAM(ContentObservation, stream << "willDestroyRenderer element: " << &element);
+
+    if (!isConsideredHidden(element))
+        m_elementsWithDestroyedVisibleRenderer.add(&element);
+}
+
 void ContentChangeObserver::contentVisibilityDidChange()
 {
     LOG(ContentObservation, "contentVisibilityDidChange: visible content change did happen.");
@@ -462,12 +500,17 @@ void ContentChangeObserver::adjustObservedState(Event event)
     }
 }
 
+bool ContentChangeObserver::shouldObserveVisibilityChangeForElement(const Element& element)
+{
+    return isObservingContentChanges() && !hasVisibleChangeState() && !visibleRendererWasDestroyed(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())
+    if (m_contentChangeObserver.shouldObserveVisibilityChangeForElement(element))
         m_wasHidden = isConsideredHidden(m_element);
 }
 
@@ -558,6 +601,17 @@ ContentChangeObserver::DOMTimerScope::~DOMTimerScope()
         m_contentChangeObserver->domTimerExecuteDidFinish(m_domTimer);
 }
 
+ContentChangeObserver::RenderTreeUpdateScope::RenderTreeUpdateScope(Document& document)
+    : m_contentChangeObserver(document.contentChangeObserver())
+{
+    m_contentChangeObserver.renderTreeUpdateDidStart();
+}
+
+ContentChangeObserver::RenderTreeUpdateScope::~RenderTreeUpdateScope()
+{
+    m_contentChangeObserver.renderTreeUpdateDidFinish();
+}
+
 }
 
 #endif // PLATFORM(IOS_FAMILY)
index d6a6920..39d4dec 100644 (file)
@@ -61,6 +61,8 @@ public:
     void didSuspendActiveDOMObjects();
     void willDetachPage();
 
+    void willDestroyRenderer(const Element&);
+
     class StyleChangeScope {
     public:
         StyleChangeScope(Document&, const Element&);
@@ -91,6 +93,14 @@ public:
         ContentChangeObserver& m_contentChangeObserver;
     };
 
+    class RenderTreeUpdateScope {
+    public:
+        RenderTreeUpdateScope(Document&);
+        ~RenderTreeUpdateScope();
+    private:
+        ContentChangeObserver& m_contentChangeObserver;
+    };
+
     class StyleRecalcScope {
     public:
         StyleRecalcScope(Document&);
@@ -160,6 +170,11 @@ private:
     void completeDurationBasedContentObservation();
     void setObservedContentState(WKContentChange);
 
+    void renderTreeUpdateDidStart();
+    void renderTreeUpdateDidFinish();
+    bool visibleRendererWasDestroyed(const Element& element) const { return m_elementsWithDestroyedVisibleRenderer.contains(&element); }
+    bool shouldObserveVisibilityChangeForElement(const Element&);
+
     enum class Event {
         StartedTouchStartEventDispatching,
         EndedTouchStartEventDispatching,
@@ -187,6 +202,7 @@ private:
     HashSet<const DOMTimer*> m_DOMTimerList;
     // FIXME: Move over to WeakHashSet when it starts supporting const.
     HashSet<const Element*> m_elementsWithTransition;
+    HashSet<const Element*> m_elementsWithDestroyedVisibleRenderer;
     WKContentChange m_observedContentState { WKContentNoChange };
     bool m_touchEventIsBeingDispatched { false };
     bool m_isWaitingForStyleRecalc { false };
@@ -196,6 +212,7 @@ private:
     bool m_mouseMovedEventIsBeingDispatched { false };
     bool m_isBetweenTouchEndAndMouseMoved { false };
     bool m_isObservingTransitions { false };
+    bool m_isInObservedRenderTreeUpdate { false };
 };
 
 inline void ContentChangeObserver::setObservedContentState(WKContentChange observedContentChange)
index dc1c41f..f7545a5 100644 (file)
@@ -136,6 +136,10 @@ static bool shouldCreateRenderer(const Element& element, const RenderElement& pa
 
 void RenderTreeUpdater::updateRenderTree(ContainerNode& root)
 {
+#if PLATFORM(IOS_FAMILY)
+    ContentChangeObserver::RenderTreeUpdateScope observingScope(m_document);
+#endif
+
     ASSERT(root.renderer());
     ASSERT(m_parentStack.isEmpty());
 
@@ -556,6 +560,9 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
             GeneratedContent::removeAfterPseudoElement(element, builder);
 
             if (auto* renderer = element.renderer()) {
+#if PLATFORM(IOS_FAMILY)
+                document.contentChangeObserver().willDestroyRenderer(element);
+#endif
                 builder.destroyAndCleanUpAnonymousWrappers(*renderer);
                 element.setRenderer(nullptr);
             }