Title: [203985] trunk
Revision
203985
Author
an...@apple.com
Date
2016-08-01 13:49:53 -0700 (Mon, 01 Aug 2016)

Log Message

REGRESSION(r198943): drop-down menu navigation on fiddlevideo.com doesn't appear on iOS, works on OS X
https://bugs.webkit.org/show_bug.cgi?id=160406
Source/WebCore:

rdar://problem/26310261

Reviewed by Simon Fraser.

On iOS we generate synthetic mouse events from taps. Click event is generated on tap only if the move event
doesn't produce visible changes to the document. This is important to make certain types of drop down menus
work.

The information on mutations is passed via WKContentObservation side channel which is updated from varous parts
of the code. Newly visible elements are detected CheckForVisibilityChangeOnRecalcStyle during style resolution.
This got broken by the style refactoring because it assumes that renderer is mutated along with style computation.
However mutation is now a separate step performed by RenderTreeUpdater.

Fix by moving CheckForVisibilityChange to RenderTreeUpdater.

Test: fast/content-observation/click-event-suppression-on-content-change.html

* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::Parent::Parent):
(WebCore::RenderTreeUpdater::updateElementRenderer):
(WebCore::RenderTreeUpdater::tearDownRenderer):
(WebCore::elementImplicitVisibility):
(WebCore::CheckForVisibilityChange::CheckForVisibilityChange):
(WebCore::CheckForVisibilityChange::~CheckForVisibilityChange):
* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):
(WebCore::Style::TreeResolver::pushParent):
(WebCore::Style::TreeResolver::resolveComposedTree):
(WebCore::Style::elementImplicitVisibility): Deleted.
(WebCore::Style::CheckForVisibilityChangeOnRecalcStyle::CheckForVisibilityChangeOnRecalcStyle): Deleted.
(WebCore::Style::CheckForVisibilityChangeOnRecalcStyle::~CheckForVisibilityChangeOnRecalcStyle): Deleted.

LayoutTests:


Reviewed by Simon Fraser.

This stuff has had zero test coverage. Adding a basic UIScript based test.

* TestExpectations:
* fast/content-observation/click-event-suppression-on-content-change-expected.txt: Added.
* fast/content-observation/click-event-suppression-on-content-change.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (203984 => 203985)


--- trunk/LayoutTests/ChangeLog	2016-08-01 20:48:35 UTC (rev 203984)
+++ trunk/LayoutTests/ChangeLog	2016-08-01 20:49:53 UTC (rev 203985)
@@ -1,3 +1,16 @@
+2016-08-01  Antti Koivisto  <an...@apple.com>
+
+        REGRESSION(r198943): drop-down menu navigation on fiddlevideo.com doesn't appear on iOS, works on OS X
+        https://bugs.webkit.org/show_bug.cgi?id=160406
+
+        Reviewed by Simon Fraser.
+
+        This stuff has had zero test coverage. Adding a basic UIScript based test.
+
+        * TestExpectations:
+        * fast/content-observation/click-event-suppression-on-content-change-expected.txt: Added.
+        * fast/content-observation/click-event-suppression-on-content-change.html: Added.
+
 2016-08-01  Eric Carlson  <eric.carl...@apple.com>
 
         [Mac][iOS] Adopt MediaRemote "seek to playback position"

Modified: trunk/LayoutTests/TestExpectations (203984 => 203985)


--- trunk/LayoutTests/TestExpectations	2016-08-01 20:48:35 UTC (rev 203984)
+++ trunk/LayoutTests/TestExpectations	2016-08-01 20:49:53 UTC (rev 203985)
@@ -22,6 +22,7 @@
 fast/events/ios [ Skip ]
 fast/events/touch/ios [ Skip ]
 fast/scrolling/ios [ Skip ]
+fast/content-observation [ Skip ]
 media/mac [ Skip ]
 
 fast/forms/attributed-strings.html [ Skip ]

Added: trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change-expected.txt (0 => 203985)


--- trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change-expected.txt	2016-08-01 20:49:53 UTC (rev 203985)
@@ -0,0 +1,14 @@
+Test that if document is visibly mutated in mouseover handler then synthetic click is not generated until the next tap.
+Tapping once
+mouseover
+mouseclick
+Tapping again
+mouseclick
+Tapping out
+mouseout
+Enabling mutation on mouseover
+Tapping once
+mouseover
+Tapping again
+mouseclick
+

Added: trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change.html (0 => 203985)


--- trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change.html	                        (rev 0)
+++ trunk/LayoutTests/fast/content-observation/click-event-suppression-on-content-change.html	2016-08-01 20:49:53 UTC (rev 203985)
@@ -0,0 +1,80 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+#neutral { position:absolute; top:0px; left:100px; width:100px; height:100px; border:2px solid blue; }
+#test { position:absolute; top:100px; left:100px; width:100px; height:100px; border:2px solid blue; }
+#test div { border: 2px solid red; width: 10px; height: 10px; }
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+var outputText = "";
+
+function output(text) {
+    outputText += text + '<br>';
+}
+
+var mutateOnMouseOver = false;
+
+function mouseOver() {
+    if (mutateOnMouseOver)
+        document.querySelector('#test').appendChild(document.createElement("div"));
+    output('mouseover');
+}
+function mouseOut() {
+    output('mouseout');
+}
+function mouseClick() {
+    output('mouseclick');
+}
+
+function getTapUIScript(x, y)
+{
+    return `
+    (function() {
+        uiController.singleTapAtPoint(${x}, ${y}, function() {
+            uiController.uiScriptComplete("Done");
+        });
+    })();`
+}
+
+function test() {
+    if (!window.testRunner || !window.testRunner.runUIScript)
+        return;
+    // Test tapping in a div.
+    output("Tapping once");
+    testRunner.runUIScript(getTapUIScript(150, 150), function(result) {
+        output("Tapping again");
+        testRunner.runUIScript(getTapUIScript(150, 150), function(result) {
+            output("Tapping out");
+            testRunner.runUIScript(getTapUIScript(150, 50), function(result) {
+                output("Enabling mutation on mouseover");
+                mutateOnMouseOver = true;
+                output("Tapping once");
+                testRunner.runUIScript(getTapUIScript(150, 150), function(result) {
+                    output("Tapping again");
+                    testRunner.runUIScript(getTapUIScript(150, 150), function(result) {
+                        document.querySelector('#output').innerHTML += outputText;
+                        testRunner.notifyDone();
+                    });
+                });
+            });
+        });
+    });
+}
+
+</script>
+</head>
+<body _onload_="test()">
+    <div>Test that if document is visibly mutated in mouseover handler then synthetic click is not generated until the next tap.</div>
+    <div id='neutral' _onmouseover_='' _onmouseout_='' _onclick_=''>
+    </div>
+    <div id='test' _onmouseover_='mouseOver()' _onmouseout_='mouseOut()' _onclick_='mouseClick()'>
+    </div>
+    <div id='output'></div>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (203984 => 203985)


--- trunk/Source/WebCore/ChangeLog	2016-08-01 20:48:35 UTC (rev 203984)
+++ trunk/Source/WebCore/ChangeLog	2016-08-01 20:49:53 UTC (rev 203985)
@@ -1,3 +1,39 @@
+2016-08-01  Antti Koivisto  <an...@apple.com>
+
+        REGRESSION(r198943): drop-down menu navigation on fiddlevideo.com doesn't appear on iOS, works on OS X
+        https://bugs.webkit.org/show_bug.cgi?id=160406
+        rdar://problem/26310261
+
+        Reviewed by Simon Fraser.
+
+        On iOS we generate synthetic mouse events from taps. Click event is generated on tap only if the move event
+        doesn't produce visible changes to the document. This is important to make certain types of drop down menus
+        work.
+
+        The information on mutations is passed via WKContentObservation side channel which is updated from varous parts
+        of the code. Newly visible elements are detected CheckForVisibilityChangeOnRecalcStyle during style resolution.
+        This got broken by the style refactoring because it assumes that renderer is mutated along with style computation.
+        However mutation is now a separate step performed by RenderTreeUpdater.
+
+        Fix by moving CheckForVisibilityChange to RenderTreeUpdater.
+
+        Test: fast/content-observation/click-event-suppression-on-content-change.html
+
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::Parent::Parent):
+        (WebCore::RenderTreeUpdater::updateElementRenderer):
+        (WebCore::RenderTreeUpdater::tearDownRenderer):
+        (WebCore::elementImplicitVisibility):
+        (WebCore::CheckForVisibilityChange::CheckForVisibilityChange):
+        (WebCore::CheckForVisibilityChange::~CheckForVisibilityChange):
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+        (WebCore::Style::TreeResolver::pushParent):
+        (WebCore::Style::TreeResolver::resolveComposedTree):
+        (WebCore::Style::elementImplicitVisibility): Deleted.
+        (WebCore::Style::CheckForVisibilityChangeOnRecalcStyle::CheckForVisibilityChangeOnRecalcStyle): Deleted.
+        (WebCore::Style::CheckForVisibilityChangeOnRecalcStyle::~CheckForVisibilityChangeOnRecalcStyle): Deleted.
+
 2016-08-01  Eric Carlson  <eric.carl...@apple.com>
 
         [iOS] A video element that does not pause after exiting from fullscreen should be allowed to continue playing inline

Modified: trunk/Source/WebCore/style/RenderTreeUpdater.cpp (203984 => 203985)


--- trunk/Source/WebCore/style/RenderTreeUpdater.cpp	2016-08-01 20:48:35 UTC (rev 203984)
+++ trunk/Source/WebCore/style/RenderTreeUpdater.cpp	2016-08-01 20:49:53 UTC (rev 203985)
@@ -34,6 +34,7 @@
 #include "FlowThreadController.h"
 #include "HTMLSlotElement.h"
 #include "InspectorInstrumentation.h"
+#include "NodeRenderStyle.h"
 #include "PseudoElement.h"
 #include "RenderFullScreen.h"
 #include "RenderNamedFlowThread.h"
@@ -40,8 +41,26 @@
 #include "StyleResolver.h"
 #include "StyleTreeResolver.h"
 
+#if PLATFORM(IOS)
+#include "WKContentObservation.h"
+#endif
+
 namespace WebCore {
 
+#if PLATFORM(IOS)
+class CheckForVisibilityChange {
+public:
+    CheckForVisibilityChange(const Element&);
+    ~CheckForVisibilityChange();
+
+private:
+    const Element& m_element;
+    EDisplay m_previousDisplay;
+    EVisibility m_previousVisibility;
+    EVisibility m_previousImplicitVisibility;
+};
+#endif // PLATFORM(IOS)
+
 RenderTreeUpdater::Parent::Parent(ContainerNode& root)
     : element(is<Document>(root) ? nullptr : downcast<Element>(&root))
     , renderTreePosition(RenderTreePosition(*root.renderer()))
@@ -242,6 +261,10 @@
 
 void RenderTreeUpdater::updateElementRenderer(Element& element, Style::ElementUpdate& update)
 {
+#if PLATFORM(IOS)
+    CheckForVisibilityChange checkForVisibilityChange(element);
+#endif
+
     bool shouldTearDownRenderers = update.change == Style::Detach && (element.renderer() || element.isNamedFlowContentElement());
     if (shouldTearDownRenderers)
         tearDownRenderers(element, TeardownType::KeepHoverAndActive);
@@ -566,4 +589,51 @@
     text.setRenderer(nullptr);
 }
 
+#if PLATFORM(IOS)
+static EVisibility elementImplicitVisibility(const Element& element)
+{
+    auto* renderer = element.renderer();
+    if (!renderer)
+        return VISIBLE;
+
+    auto& style = renderer->style();
+
+    auto width = style.width();
+    auto height = style.height();
+    if ((width.isFixed() && width.value() <= 0) || (height.isFixed() && height.value() <= 0))
+        return HIDDEN;
+
+    auto top = style.top();
+    auto left = style.left();
+    if (left.isFixed() && width.isFixed() && -left.value() >= width.value())
+        return HIDDEN;
+
+    if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
+        return HIDDEN;
+    return VISIBLE;
 }
+
+CheckForVisibilityChange::CheckForVisibilityChange(const Element& element)
+    : m_element(element)
+    , m_previousDisplay(element.renderStyle() ? element.renderStyle()->display() : NONE)
+    , m_previousVisibility(element.renderStyle() ? element.renderStyle()->visibility() : HIDDEN)
+    , m_previousImplicitVisibility(WKObservingContentChanges() && WKObservedContentChange() != WKContentVisibilityChange ? elementImplicitVisibility(element) : VISIBLE)
+{
+}
+
+CheckForVisibilityChange::~CheckForVisibilityChange()
+{
+    if (!WKObservingContentChanges())
+        return;
+    if (m_element.isInUserAgentShadowTree())
+        return;
+    auto* style = m_element.renderStyle();
+    if (!style)
+        return;
+    if ((m_previousDisplay == NONE && style->display() != NONE) || (m_previousVisibility == HIDDEN && style->visibility() != HIDDEN)
+        || (m_previousImplicitVisibility == HIDDEN && elementImplicitVisibility(m_element) == VISIBLE))
+        WKSetObservedContentChange(WKContentVisibilityChange);
+}
+#endif
+
+}

Modified: trunk/Source/WebCore/style/StyleTreeResolver.cpp (203984 => 203985)


--- trunk/Source/WebCore/style/StyleTreeResolver.cpp	2016-08-01 20:48:35 UTC (rev 203984)
+++ trunk/Source/WebCore/style/StyleTreeResolver.cpp	2016-08-01 20:49:53 UTC (rev 203985)
@@ -47,10 +47,6 @@
 #include "StyleResolver.h"
 #include "Text.h"
 
-#if PLATFORM(IOS)
-#include "WKContentObservation.h"
-#endif
-
 namespace WebCore {
 
 namespace Style {
@@ -265,60 +261,6 @@
     return update;
 }
 
-#if PLATFORM(IOS)
-static EVisibility elementImplicitVisibility(const Element* element)
-{
-    RenderObject* renderer = element->renderer();
-    if (!renderer)
-        return VISIBLE;
-
-    auto& style = renderer->style();
-
-    Length width(style.width());
-    Length height(style.height());
-    if ((width.isFixed() && width.value() <= 0) || (height.isFixed() && height.value() <= 0))
-        return HIDDEN;
-
-    Length top(style.top());
-    Length left(style.left());
-    if (left.isFixed() && width.isFixed() && -left.value() >= width.value())
-        return HIDDEN;
-
-    if (top.isFixed() && height.isFixed() && -top.value() >= height.value())
-        return HIDDEN;
-    return VISIBLE;
-}
-
-class CheckForVisibilityChangeOnRecalcStyle {
-public:
-    CheckForVisibilityChangeOnRecalcStyle(Element* element, const RenderStyle* currentStyle)
-        : m_element(element)
-        , m_previousDisplay(currentStyle ? currentStyle->display() : NONE)
-        , m_previousVisibility(currentStyle ? currentStyle->visibility() : HIDDEN)
-        , m_previousImplicitVisibility(WKObservingContentChanges() && WKObservedContentChange() != WKContentVisibilityChange ? elementImplicitVisibility(element) : VISIBLE)
-    {
-    }
-    ~CheckForVisibilityChangeOnRecalcStyle()
-    {
-        if (!WKObservingContentChanges())
-            return;
-        if (m_element->isInUserAgentShadowTree())
-            return;
-        auto* style = m_element->renderStyle();
-        if (!style)
-            return;
-        if ((m_previousDisplay == NONE && style->display() != NONE) || (m_previousVisibility == HIDDEN && style->visibility() != HIDDEN)
-            || (m_previousImplicitVisibility == HIDDEN && elementImplicitVisibility(m_element.get()) == VISIBLE))
-            WKSetObservedContentChange(WKContentVisibilityChange);
-    }
-private:
-    RefPtr<Element> m_element;
-    EDisplay m_previousDisplay;
-    EVisibility m_previousVisibility;
-    EVisibility m_previousImplicitVisibility;
-};
-#endif // PLATFORM(IOS)
-
 void TreeResolver::pushParent(Element& element, const RenderStyle& style, Change change)
 {
     scope().selectorFilter.pushParent(&element);
@@ -448,9 +390,6 @@
 
         bool shouldResolve = shouldResolveElement(element, parent.change) || affectedByPreviousSibling;
         if (shouldResolve) {
-#if PLATFORM(IOS)
-            CheckForVisibilityChangeOnRecalcStyle checkForVisibilityChange(&element, element.renderStyle());
-#endif
             element.resetComputedStyle();
 
             if (element.hasCustomStyleResolveCallbacks()) {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to