Bug 145642 – Latching algorithm in findEnclosingOverflowScroll is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

I discovered this problem while debugging Bug 145637.

The logic in findEnclosingOverflowScroll uses the RenderBox "canBeScrolledAndHasScrollableArea" to identify the nearest enclosing element that can be scrolled. However, this method considers horizontal and vertical scrolling as equally relevant. This is a problem when we want to perform a vertical scroll, but the element underneath the mouse pointer can only be scrolled in a horizontal direction.

When we have this scenario, the latching logic chooses the horizontally scrollable region and attempts to latch to it. When it discovers that it cannot be scrolled in the user's gesture direction, it gives up and passes control to the next enclosing latching context. When we are working on a single page, this means the document body, and things generally work as the user expects. But when we have nested iframes, this causes us to pop up to the enclosing iframe, which looks wrong to the user.

The test case I added in Bug 145637 highlights this problem if you modify the <img> style to remove the "max-width:100%" in 'inner_content.html'.

I think the right fix here is to modify 'findEnclosingOverflowScroll' so that it takes the gesture's dominant direction into account.

[Radar WebKit Bug Importer]

<rdar://problem/21242308>

[Brent Fulgham]

Created attachment 254281 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 254281 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:795
> +            if ((verticalScroll && box->hasScrollableOverflowY()) || (!verticalScroll && box->hasScrollableOverflowX()))
> +                return candidate;

I'm not sure I like the idea of adding "predominantly vertical" logic here. The event could have non-zero X delta, and box->hasScrollableOverflowX() could be true, but we would not return that box as a candidate.

[Brent Fulgham]

(In reply to comment #3)
> Comment on attachment 254281 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254281&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:795
> > +            if ((verticalScroll && box->hasScrollableOverflowY()) || (!verticalScroll && box->hasScrollableOverflowX()))
> > +                return candidate;
> 
> I'm not sure I like the idea of adding "predominantly vertical" logic here.
> The event could have non-zero X delta, and box->hasScrollableOverflowX()
> could be true, but we would not return that box as a candidate.

Ah! Good point.

Maybe something like "deltaX && box->hasScrollableOverflowX() || deltaY && box->hasScrollableOverflowY()"

[Brent Fulgham]

Created attachment 254282 [details]
Patch

[Brent Fulgham]

Committed r185208: <http://trac.webkit.org/changeset/185208>

[Alexey Proskuryakov]

This broke platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html

[Said Abou-Hallawa]

@@ -8,7 +8,7 @@
 
 TEST COMPLETE
 PASS Page did not receive wheel events.
-PASS IFrame did not receive wheel events.
+FAIL IFrame consumed wheel events.
 PASS Select consumed wheel events.
 PASS successfullyParsed is true

Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145655

[Alexey Proskuryakov]

Rolling out r185211 too (which just added tests that now fail).

[Brent Fulgham]

(In reply to comment #7)
> This broke
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-select.
> html

Ah! Indeed it does. The <select> case doesn't work the same way because it doesn't treat its scrollable area as overflow. So the "scrollsOverflowY()" returns false for <select>, even though it does satisfy "canBeScrolledAndHasScrollableArea".

[Brent Fulgham]

Created attachment 254323 [details]
Patch

[Brent Fulgham]

Created attachment 254325 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 254325 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:782
> +static ContainerNode* findEnclosingOverflowScrollForDominantDirection(ContainerNode* node, float deltaX, float deltaY)

This isn't really "dominant direction" any more.

[Brent Fulgham]

Committed r185234: <http://trac.webkit.org/changeset/185234>