Bug 147668 - REGRESSION (r173784): [Mac] Correct latching error for non-scrollable iframe nested inside scrollable div.
Summary: REGRESSION (r173784): [Mac] Correct latching error for non-scrollable iframe ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-04 17:46 PDT by Brent Fulgham
Modified: 2015-08-05 03:50 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.30 KB, patch)
2015-08-04 18:08 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-08-04 17:46:41 PDT
The WebKit latching logic improperly causes the main frame to scroll in the following case:

1. The page contains an iframe that is sized so that its contents do not need to scroll.
2. The iframe is marked as "overflow: hidden"
3. The iframe is contained in a div, which is sized so that it needs to scroll.

When this happens, the latching logic improperly skips the containing div, and passes the wheel events to the main frame.
Comment 1 Brent Fulgham 2015-08-04 17:47:48 PDT
<rdar://problem/21870332>
Comment 2 Brent Fulgham 2015-08-04 18:03:12 PDT
This bug was caused by the following:

1. When latching, we evaluate each new wheel event and decide if we should identify the element to latch to, or if we are continuing a set of gestures that are already tied to an existing element.

2. If we are already latched, we pass wheel events to the latched element until we hit the edge of the scrollable region.

3. However, if we have nested scrolling contexts, and we are processing wheel events in the context of a nested frame, we do not want to pass events to the latched element. Consequently, in EventHandler::platformPrepareForWheelEvents, if we find that latchingIsLockedToAncestorOfThisFrame is true we exit early and do not pass events to the latched element.

The bug was that EventHandler::platformCompleteWheelEvent was not handling the nested latch case properly. It was grabbing the current latching state, regardless of whether it applied to the current frame or not. In the case where we were latched to an element in an enclosing frame, we would reach up to the enclosing frame and pass the wheel events to its enclosing view (in this case, the Main Frame).

Instead, we need to perform the same short-circuit we do when beginning processing the wheel event. If we are latched to an enclosing context, we should skip the logic of finding the scrolling container for the latched element. Instead, we should just give the current element the opportunity to handle the wheel event (in case the user provided handlers for it), and then return.
Comment 3 Brent Fulgham 2015-08-04 18:08:56 PDT
Created attachment 258250 [details]
Patch
Comment 4 Simon Fraser (smfr) 2015-08-04 18:13:56 PDT
Comment on attachment 258250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258250&action=review

> Source/WebCore/ChangeLog:9
> +        Test: platform/mac/fast/scrolling/scroll-div-with-nested-nonscrollable-iframe.html

The new hotness is to put the test in fast/scrolling/scroll-div-with-nested-nonscrollable-iframe.html but I haven't moved platform/mac/fast/scrolling yet so you're OK.
Comment 5 Brent Fulgham 2015-08-04 18:26:25 PDT
Committed r187930: <http://trac.webkit.org/changeset/187930>
Comment 7 Antti Koivisto 2015-08-05 03:50:03 PDT
Comment on attachment 258250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258250&action=review

> LayoutTests/platform/mac/fast/scrolling/scroll-div-with-nested-nonscrollable-iframe.html:87
> +    if (window.eventSender) {
> +        testRunner.waitUntilDone();

Not sure if this is why it is timing out but the correct way to make an async test using js-test-post-pre/js-test-post-post is 

jsTestIsAsync = true;

on top of the test and finishJSTest() to end it. Explicit testRunner.waitUntilDone()/testRunner.notifyDone() are not needed.