REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both selec...
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jun 2015 18:37:23 +0000 (18:37 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jun 2015 18:37:23 +0000 (18:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145574
<rdar://problem/20966828>

Reviewed by Simon Fraser.

Source/WebCore:

Tested by platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html

When the scroll gesture is started when the latched scrollable container is not at the limit of its
scroll region, we are NOT supposed to propagate the scroll event to the enclosing region. However,
we were doing two things wrong:
(1) When we recognized we were latching, we were using the right wheel event target, but not using
    the latched scrollable container.
(2) Likewise, we were not using latched ScrollableArea when handling wheel events.

Instead, we were using the current scrollable container and ScrollableArea under the mouse pointer,
which could be different from the point we started latching as the content scrolled.

The fix was to properly track the scrollable container and scrollable area during latching.

I attempted to store the latched ScrollableArea in the latchingState object, like we already do for the
scrollable container, but found that this did not work properly. I think the life cycle of the
ScrollableArea may not match the scrollable container, and since they are not reference counted I
simply retrieve the ScrollableArea when needed.

* page/mac/EventHandlerMac.mm:
(WebCore::scrollableAreaForContainerNode): Helper function to return the correct ScrollableArea
for the two types of RenderBox elements.
(WebCore::latchedToFrameOrBody): Helper predicate to identify Frame and Body elements.
(WebCore::EventHandler::platformPrepareForWheelEvents): Use the correct ScrollableArea for the given
ContainerNode. When latching, make sure to use the ScrollableArea that is related to the latched scrollable
container, not the area currently underneath the mouse pointer.

LayoutTests:

* platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select-expected.txt: Added.
* platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html: Added.
* platform/mac-wk2/tiled-drawing/scrolling/frames/select_iframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html [new file with mode: 0644]
LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/resources/select_iframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/mac/EventHandlerMac.mm

index a7b51e5..bf561eb 100644 (file)
@@ -1,3 +1,15 @@
+2015-06-03  Brent Fulgham  <bfulgham@apple.com>
+
+        REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both select and iframe
+        https://bugs.webkit.org/show_bug.cgi?id=145574
+        <rdar://problem/20966828>
+
+        Reviewed by Simon Fraser.
+
+        * platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select-expected.txt: Added.
+        * platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html: Added.
+        * platform/mac-wk2/tiled-drawing/scrolling/frames/select_iframe.html: Added.
+
 2015-06-03  Brady Eidson  <beidson@apple.com>
 
         REGRESSION (r183498): Certain types of frame loads in iframes with <base target="_blank"> can open urls in new window/tabs
diff --git a/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select-expected.txt b/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select-expected.txt
new file mode 100644 (file)
index 0000000..307da46
--- /dev/null
@@ -0,0 +1,16 @@
+
+Tests that iframe doesn't consume wheel events when scrolling a select in an iframe.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS Page did not receive wheel events.
+PASS IFrame did not receive wheel events.
+PASS Select consumed wheel events.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html b/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html
new file mode 100644 (file)
index 0000000..b6a7463
--- /dev/null
@@ -0,0 +1,99 @@
+<!DOCTYPE html>
+<html>
+<head>
+<link rel="help" href="http://www.w3.org/TR/DOM-Level-3-Events/#events-WheelEvent">
+<script src="../../../../resources/js-test-pre.js"></script>
+</head>
+<body onload="setupTopLevel();">
+<script>
+var pageScrollPositionBefore;
+var iframeTarget;
+var iFrameScrollPositionBefore;
+var selectTarget;
+var selectTargetScrollPositionBefore;
+
+function checkForScroll()
+{
+    // The IFrame should not have scrolled at all.
+    var pageScrollPositionAfter = document.body.scrollTop;
+    if (pageScrollPositionAfter != pageScrollPositionAfter)
+        testFailed("Page consumed wheel events.");
+    else
+        testPassed("Page did not receive wheel events.");
+
+    var iFrameScrollPositionAfter = window.frames['target'].document.body.scrollTop;
+    if (iFrameScrollPositionBefore != iFrameScrollPositionAfter)
+        testFailed("IFrame consumed wheel events.");
+    else
+        testPassed("IFrame did not receive wheel events.");
+
+    var selectTargetScrollPositionAfter = selectTarget.scrollTop;
+    if (selectTargetScrollPositionBefore != selectTargetScrollPositionAfter)
+        testPassed("Select consumed wheel events.");
+    else
+        testFailed("Select did not receive wheel events.");
+
+    finishJSTest();
+    testRunner.notifyDone();
+}
+
+function scrollTest()
+{
+    pageScrollPositionBefore = document.body.scrollTop;
+    iFrameScrollPositionBefore = window.frames['target'].document.body.scrollTop;
+
+    iframeTarget = document.getElementById('target');
+
+    selectTarget = window.frames['target'].document.getElementById('selectTarget');
+    selectTargetScrollPositionBefore = selectTarget.scrollTop;
+
+    // Scroll the #source until we reach the #target.
+    var startPosX = Math.round(selectTarget.offsetLeft) + 10;
+    var startPosY = Math.round(selectTarget.offsetTop) + 10; // Slightly more than one wheel scroll away from the IFrame
+    eventSender.mouseMoveTo(startPosX, startPosY);
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'none', 'begin');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'none', 'continue');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'none', 'continue');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'none', 'continue');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'none', 'continue');
+    eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'none', 'end');
+    eventSender.callAfterScrollingCompletes(checkForScroll);
+
+    // We should finish via the scroll event; this will fire in the case of failure when the page doesn't scroll.
+}
+
+function setupTopLevel()
+{
+    if (window.eventSender) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+
+        eventSender.monitorWheelEvents();
+        setTimeout(scrollTest, 0);
+    } else {
+        var messageLocation = document.getElementById('parent');
+        var message = document.createElement('div');
+        message.innerHTML = "<p>This test is better run under DumpRenderTree. To manually test it, place the mouse pointer<br/>"
+            + "at the top of the page, and then use the mouse wheel or a two-finger swipe to scroll the<br/>"
+            + "down past the iframe.<br/><br/>"
+            + "The iframe should not scroll.</p>";
+        messageLocation.appendChild(message);
+    }
+}
+
+</script>
+<div id="parent" style="height: 2000px;">
+    <iframe id="target" name="target" height="500" width="600" src="resources/select_iframe.html"></iframe>
+    </iframe>
+</div>
+<div id="console"></div>
+<script>
+description("Tests that iframe doesn't consume wheel events when scrolling a select in an iframe.");
+</script>
+<script src="../../../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/resources/select_iframe.html b/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/resources/select_iframe.html
new file mode 100644 (file)
index 0000000..64dffac
--- /dev/null
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head> 
+<title>Page Title</title>
+</head>
+<body>
+
+<h1>This is a Heading</h1>
+<p>This is a paragraph.</p>
+ <select id="selectTarget" size="5" multiple="multiple" name="driver[]">
+        <option value="" >(No Driver Filter)</option>
+        <option value="drivername=''"></option>
+        <option value="drivername='alex'">alex</option>
+        <option value="drivername='marc'">marc</option>
+        <option value="drivername='frank'">frank</option>
+        <option value="drivername='james'">james</option>
+    <option value="drivername='michael'">michael</option>
+</select>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+<p>This is a paragraph.</p>
+
+
+</body>
+</html>
\ No newline at end of file
index de323e9..6c317a7 100644 (file)
@@ -1,3 +1,38 @@
+2015-06-03  Brent Fulgham  <bfulgham@apple.com>
+
+        REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both select and iframe
+        https://bugs.webkit.org/show_bug.cgi?id=145574
+        <rdar://problem/20966828>
+
+        Reviewed by Simon Fraser.
+
+        Tested by platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html
+
+        When the scroll gesture is started when the latched scrollable container is not at the limit of its
+        scroll region, we are NOT supposed to propagate the scroll event to the enclosing region. However,
+        we were doing two things wrong:
+        (1) When we recognized we were latching, we were using the right wheel event target, but not using
+            the latched scrollable container.
+        (2) Likewise, we were not using latched ScrollableArea when handling wheel events.
+
+        Instead, we were using the current scrollable container and ScrollableArea under the mouse pointer,
+        which could be different from the point we started latching as the content scrolled.
+        
+        The fix was to properly track the scrollable container and scrollable area during latching.
+
+        I attempted to store the latched ScrollableArea in the latchingState object, like we already do for the
+        scrollable container, but found that this did not work properly. I think the life cycle of the
+        ScrollableArea may not match the scrollable container, and since they are not reference counted I
+        simply retrieve the ScrollableArea when needed.
+
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::scrollableAreaForContainerNode): Helper function to return the correct ScrollableArea
+        for the two types of RenderBox elements.
+        (WebCore::latchedToFrameOrBody): Helper predicate to identify Frame and Body elements.
+        (WebCore::EventHandler::platformPrepareForWheelEvents): Use the correct ScrollableArea for the given
+        ContainerNode. When latching, make sure to use the ScrollableArea that is related to the latched scrollable
+        container, not the area currently underneath the mouse pointer.
+
 2015-06-03  Brady Eidson  <beidson@apple.com>
 
         REGRESSION (r183498): Certain types of frame loads in iframes with <base target="_blank"> can open urls in new window/tabs
index 7e71dcf..35de1c1 100644 (file)
@@ -883,6 +883,26 @@ static bool latchingIsLockedToAncestorOfThisFrame(const Frame& frame)
     return false;
 }
 
+static ScrollableArea* scrollableAreaForContainerNode(ContainerNode& container)
+{
+    ScrollableArea* scrollableArea = nullptr;
+
+    if (RenderBox* box = container.renderBox()) {
+        if (is<RenderListBox>(*box))
+            scrollableArea = downcast<RenderListBox>(box);
+        else
+            scrollableArea = box->layer();
+    }
+
+    return scrollableArea;
+}
+
+static bool latchedToFrameOrBody(ContainerNode& container)
+{
+    // FIXME(106133): We might need to add or switch to is<HTMLDocumentElement> when this bug is fixed.
+    return is<HTMLFrameSetElement>(container) || is<HTMLBodyElement>(container);
+}
+
 void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& wheelEvent, const HitTestResult& result, RefPtr<Element>& wheelEventTarget, RefPtr<ContainerNode>& scrollableContainer, ScrollableArea*& scrollableArea, bool& isOverWidget)
 {
     FrameView* view = m_frame.view();
@@ -897,14 +917,9 @@ void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& wheel
             scrollableArea = scrollViewForEventTarget(wheelEventTarget.get());
         } else {
             scrollableContainer = findEnclosingOverflowScroll(wheelEventTarget.get());
-            if (scrollableContainer) {
-                if (RenderBox* box = scrollableContainer->renderBox()) {
-                    if (is<RenderListBox>(*box))
-                        scrollableArea = downcast<RenderListBox>(box);
-                    else
-                        scrollableArea = box->layer();
-                }
-            } else {
+            if (scrollableContainer)
+                scrollableArea = scrollableAreaForContainerNode(*scrollableContainer);
+            else {
                 scrollableContainer = view->frame().document()->bodyOrFrameset();
                 scrollableArea = view;
             }
@@ -944,6 +959,12 @@ void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent& wheel
 
         wheelEventTarget = latchingState->wheelEventElement();
         isOverWidget = latchingState->widgetIsLatched();
+        scrollableContainer = latchingState->scrollableContainer();
+
+        if (scrollableContainer) {
+            if (!latchedToFrameOrBody(*scrollableContainer) && !latchingState->widgetIsLatched())
+                scrollableArea = scrollableAreaForContainerNode(*scrollableContainer);
+        }
     }
 }