Bug 109316

Summary:

REGRESSION: A page can be scrolled with touch when its body has style overflow-x:hidden or overflow-y:hidden

Product:

WebKit

Reporter:

Terry Anderson <tdanderson>

Component:

Layout and Rendering

Assignee:

Terry Anderson <tdanderson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, ap, buildbot, eric, ojan.autocc, rjkroege, rniwa, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

109769

Bug Blocks:

Attachments:

Description	Flags
Example page to reproduce the bug in chromium	none
WIP, not for review	none
WIP, not for review	none
WIP, not for review	none
Page to reproduce the bug. This scrolls with touch but does not scroll with mousewheels in chromium.	none
Patch	eric: review-

Terry Anderson

Reported 2013-02-08 13:01:30 PST

Created attachment 187352 [details] Example page to reproduce the bug in chromium The attached page has style="overflow-y:hidden" on its body. It cannot be scrolled with mousewheels but can be scrolled with touch in chromium.

Attachments
Example page to reproduce the bug in chromium (249 bytes, text/html) 2013-02-08 13:01 PST, Terry Anderson	no flags	Details
WIP, not for review (3.14 KB, patch) 2013-02-10 17:22 PST, Terry Anderson	no flags	Details Formatted Diff Diff
WIP, not for review (3.14 KB, patch) 2013-02-11 08:54 PST, Terry Anderson	no flags	Details Formatted Diff Diff
WIP, not for review (3.16 KB, patch) 2013-02-11 15:54 PST, Terry Anderson	no flags	Details Formatted Diff Diff
Page to reproduce the bug. This scrolls with touch but does not scroll with mousewheels in chromium. (348 bytes, text/html) 2013-02-12 10:52 PST, Terry Anderson	no flags	Details
Patch (31.01 KB, patch) 2013-02-12 14:03 PST, Terry Anderson	eric: review-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2013-02-08 14:33:55 PST

This page scrolls with trackpad on Mac. Behavior is different in Safari 6.0.2 and current WebKit build though.

Terry Anderson

Comment 2 2013-02-10 17:22:46 PST

Created attachment 187497 [details] WIP, not for review

Build Bot

Comment 3 2013-02-10 17:52:33 PST

Comment on attachment 187497 [details] WIP, not for review Attachment 187497 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16471981 New failing tests: fast/css/sticky/inline-sticky-abspos-child.html

Terry Anderson

Comment 4 2013-02-11 08:54:27 PST

Created attachment 187592 [details] WIP, not for review

Terry Anderson

Comment 5 2013-02-11 12:20:48 PST

Comment on attachment 187352 [details] Example page to reproduce the bug in chromium This page does not accurately reproduce the problem in chromium, please ignore.

Terry Anderson

Comment 6 2013-02-11 15:54:58 PST

Created attachment 187704 [details] WIP, not for review

Terry Anderson

Comment 7 2013-02-12 10:52:03 PST

Created attachment 187897 [details] Page to reproduce the bug. This scrolls with touch but does not scroll with mousewheels in chromium.

Terry Anderson

Comment 8 2013-02-12 14:03:09 PST

Created attachment 187930 [details] Patch

Eric Seidel (no email)

Comment 9 2013-02-12 15:13:33 PST

Terry tells me this bug is all about making Gesture scrolling match wheel-event scrolling. Currently <div> with overflow hidden scroll: text-scroll: YES wheel: NO gesture: YES <body> with overflow hidden scroll: text-scroll: YES wheel: NO gesture: YES A few months back, before Terry moved the gesture scroll path off of wheel events gesture and wheel were the same in those above cases. The goal of this change is to make them match again.

Eric Seidel (no email)

Comment 10 2013-02-12 15:15:18 PST

(In reply to comment #9) > Terry tells me this bug is all about making Gesture scrolling match wheel-event scrolling. > > Currently <div> with overflow hidden scroll: > text-scroll: YES > wheel: NO > gesture: YES Sorry, Terry tells me that bug 109087 fixed <div> scrolling for gesture to match wheel event, so that should be gesture: NO for <div> Also, to clarify "text scrolling" I mean selecting text and dragging outside of the scroll region.

Simon Fraser (smfr)

Comment 11 2013-02-12 15:21:00 PST

overflow:hidden should never be scrollable by the user (only programmatically).

Eric Seidel (no email)

Comment 12 2013-02-12 15:30:25 PST

Comment on attachment 187930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187930&action=review > Source/WebCore/rendering/RenderLayer.cpp:2047 > + if (body && body->renderer() && body->hasTagName(bodyTag)) { document->body() always hasTagName(bodyTag)

Eric Seidel (no email)

Comment 13 2013-02-12 16:42:58 PST

Comment on attachment 187930 [details] Patch OK. I spoke with Jamesr and we learned a few things. 1. Wheel events seem to scroll each direction separately, gestures may need to do the same if they aren't already. 2. This appears to be a programatic scroll path. You really want to be heading down the user-scroll paths instead. RenderBox::scroll/RenderLayer::scroll/ScrollableArea::scroll are the ones you want. 3. In the wheel-event case, scrolls are prevented for overflow: none by ScrollableArea::scroll detecting that it doesn't have a scrollbar in the direction you're asking it to scroll and bailing. Gestures should likely go down that same path.

Terry Anderson

Comment 14 2013-02-13 16:38:55 PST

After discussing the matter with Eric, it is clear that we should not even be using RenderLayer::scrollBy() to perform gesture scrolling. Switching code paths will fix this bug, among others. I have filed https://bugs.webkit.org/show_bug.cgi?id=109769 to track.

Terry Anderson

Comment 15 2013-03-01 17:14:56 PST

This issue is resolved by http://trac.webkit.org/changeset/144519.

Note You need to log in before you can comment on or make changes to this bug.