Bug 47036 – Wheel scrolls and autoscrolls should not scroll overflow:hidden

• Component: WebCore Misc.
• Status: REOPENED

[Peter Kasting]

If a ScrollView has more content than is visible in some dimension, but there is no scrollbar for that dimension, then user events such as down arrow/scroll wheel/middle-mouse-autoscroll ("pan-scroll") should not scroll it into view.  The arrow keys already don't.  This is about fixing the wheel and autoscroll code.

There is one bit of weirdness here.  Consider a scrollable page containing an iframe with overflow:hidden content.  EventHandler::handleWheelEvent() will first try to dispatch a wheel event to the inner frame.  (My patch will fix the ScrollView for this frame to correctly report that it's not scrollable.)  When that doesn't accept the event, it will then dispatch to the containing frame (the outer document).  This is how wheel scrolls scroll the whole page once a hovered element reaches the end of its scroll area.

Now consider the same case, but with middle-mouse-autoscroll.  We dispatch a mousedown event to the inner frame, which gets a chance to prevent it.  If it doesn't, the code in Node::defaultEventHandler() checks if the inner frame is scrollable.  When it's not, we terminate without doing anything.  It seems to me that it would be nice to scroll the outer page in this case.  But this raises a lot of questions.  First, should I dispatch a mousedown event to the containing frame too, like wheel events do?  Second, should I duplicate the wheel event's behavior of scrolling the outer page for scrollable inner frames that are at the end of their scroll area, or only do this for cases where the inner frames aren't scrollable at all?  Both of these seem like they could be weird.

hyatt, if you have any thoughts, I'd appreciate them...

[Peter Kasting]

Created attachment 69555 [details]
WIP patch

Here's a patch which basically fixes things.  The LayoutTest isn't done yet -- it works, but it's a manual test, I haven't yet scripted it or added expected results.  I'm not sure what the right behavior for middle-mouse autoscroll is and I haven't added a test for that.

It'd be nice to get feedback on whether my implementation here, especially w.r.t "hasAScrollbar()", is good or crazy.

[Peter Kasting]

Created attachment 69557 [details]
WIP patch w/test

Oops, forgot to svn add the LayoutTest.

[Peter Kasting]

Created attachment 69874 [details]
patch v1

[WebKit Review Bot]

Attachment 69874 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/ScrollView.cpp:696:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:697:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:698:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:702:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 69876 [details]
patch v1.1

Man, none of those was even my fault.

[Peter Kasting]

Created attachment 69988 [details]
Reduced patch for 517 branch

This is a trimmed-down version of the patch for the 517 branch, that doesn't attempt to address pan-scrolling issues.

[James Robinson]

On the reduced patch the test is not right, it shouldn't be a pixel test.  Also there are some spurious svn properties set.  That said, the bug fix is pretty critical so I'm gonna land just that part and leave the bug open to land a proper test and for the rest of the logic changes.

[James Robinson]

Comment on attachment 69988 [details]
Reduced patch for 517 branch

Landing by hand

[James Robinson]

Committed r69257: <http://trac.webkit.org/changeset/69257>

[James Robinson]

Not fixed yet

[Peter Kasting]

Comment on attachment 69988 [details]
Reduced patch for 517 branch

Marking obsolete as this was (partly) landed

[Peter Kasting]

Comment on attachment 69876 [details]
patch v1.1

Obsoleting this patch.  I'll produce another with a better test and without the bits that have already landed.

[James Robinson]

Created attachment 70021 [details]
include WebCore.exp.in changes so this builds on mac

[James Robinson]

Comment on attachment 70021 [details]
include WebCore.exp.in changes so this builds on mac

Sigh, this patch does not belong to this bug

[Eric Seidel (no email)]

Comment on attachment 69988 [details]
Reduced patch for 517 branch

Cleared James Robinson's review+ from obsolete attachment 69988 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Peter Kasting]

Created attachment 72387 [details]
Followon patch v1

This patches the pan scroll part (the part that didn't already land) and converts the wheel event layout test from a pixel test to a text test.  It also fixes the svn:executable bit being set.

[Peter Beverloo]

You might want to take note of bug 22769, which also is related to scrolling content which has overflow:hidden set. While the gesture is slightly different than the ones you're describing, the effect is similar.

[David Levin]

Comment on attachment 72387 [details]
Followon patch v1

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

> WebCore/dom/Node.cpp:2929
> +            // scrolling the entire page.  We could fix this by trying to

Single space after periods in comments (in WebKit).

> WebCore/rendering/RenderLayer.cpp:1275
> +    if (renderer()->hasOverflowClip() && (!renderer()->parent() || renderer()->parent()->style()->lineClamp().isNone())) {

This long clause is repeated in two places. Should it be a method?

Also, this appears like it is unrelated to the fix. It simply changes the form of the code to something that is less clear (to me).

> WebCore/rendering/RenderLayer.cpp:1306
> +bool RenderLayer::hasAScrollbar(bool vertical, bool horizontal)

This would be be better as two functions:


hasVerticalScrollbar
hasHorizontalScrollbar

> WebCore/rendering/RenderLayer.h:234
> +    // directions.  For renderers without an overflow clip (documents), this

Single space after .

> LayoutTests/fast/events/wheelevent-bubble.html:57
>  \ No newline at end of file

Please fix.

[Peter Kasting]

Comment on attachment 72387 [details]
Followon patch v1

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

New patch to be uploaded momentarily.

>> WebCore/dom/Node.cpp:2929
>> +            // scrolling the entire page.  We could fix this by trying to
> 
> Single space after periods in comments (in WebKit).

Fixed.

>> WebCore/rendering/RenderLayer.cpp:1275
>> +    if (renderer()->hasOverflowClip() && (!renderer()->parent() || renderer()->parent()->style()->lineClamp().isNone())) {
> 
> This long clause is repeated in two places. Should it be a method?
> 
> Also, this appears like it is unrelated to the fix. It simply changes the form of the code to something that is less clear (to me).

Factored to a separate function, which all other code now calls, and exploded that function a little for readability.

>> WebCore/rendering/RenderLayer.cpp:1306
>> +bool RenderLayer::hasAScrollbar(bool vertical, bool horizontal)
> 
> This would be be better as two functions:
> 
> 
> hasVerticalScrollbar
> hasHorizontalScrollbar

Fixed.

>> WebCore/rendering/RenderLayer.h:234
>> +    // directions.  For renderers without an overflow clip (documents), this
> 
> Single space after .

This comment is now changed.

>> LayoutTests/fast/events/wheelevent-bubble.html:57
>>  \ No newline at end of file
> 
> Please fix.

Fixed.

[Peter Kasting]

Created attachment 80522 [details]
Followon patch v2

[Adele Peterson]

Will this break scrolling in text fields, which use overflow:hidden?

[Peter Kasting]

(In reply to comment #21)
> Will this break scrolling in text fields, which use overflow:hidden?

They do?  Testing on this comment box gives me a scrollbar when my text gets too big.  Am I testing the wrong thing?

[Adele Peterson]

That's a textarea.  I was talking about <input>s.

(In reply to comment #22)
> (In reply to comment #21)
> > Will this break scrolling in text fields, which use overflow:hidden?
> 
> They do?  Testing on this comment box gives me a scrollbar when my text gets too big.  Am I testing the wrong thing?

[Peter Kasting]

(In reply to comment #23)
> That's a textarea.  I was talking about <input>s.

So, single-line entries?  Hmm, I didn't check.  My Windows box doesn't have anything with native horizontal scrolling, I'd need to try a trackpad or something.  I didn't even know you could wheel-scroll such things.

[James Robinson]

On my mac I can't currently two-finger scroll horizontally within an <input> that contains a large amount of text, so I don't think this would be a behavior change.

[Adele Peterson]

What about autoscrolling?

[Peter Kasting]

(In reply to comment #26)
> What about autoscrolling?

I kinda feel like if we currently allow autoscrolling in James' case, it's a bug.  Shouldn't we ought to allow wheel and autoscrolls in the same cases?

[Adele Peterson]

hmm maybe I'm not talking about the same thing here - when you say autoscroll, do you mean, pan-scroll?  Because I don't :)

[Peter Kasting]

(In reply to comment #28)
> hmm maybe I'm not talking about the same thing here - when you say autoscroll, do you mean, pan-scroll?  Because I don't :)

Yes.  From comment 0: "...user events such as down arrow/scroll wheel/middle-mouse-autoscroll ("pan-scroll")..."

This will have no effect at all on whether the field scrolls e.g. as you type characters.

[Adele Peterson]

Ok sounds like this isn't an issue then.  Just for future reference though - autoscroll != pan-scroll in WebKit.

[Eric Seidel (no email)]

Comment on attachment 80522 [details]
Followon patch v2

I suspect this patch no longer applies since it's 4 months old now.

[Eric Seidel (no email)]

Comment on attachment 80522 [details]
Followon patch v2

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

This seems reasonable to me now that I actually read it.  I'm sad it was ignored so long.

> Source/WebCore/rendering/RenderLayer.h:242
> +    bool hasVerticalScrollbar();
> +    bool hasHorizontalScrollbar();

Should these be const?

[Peter Kasting]

Comment on attachment 80522 [details]
Followon patch v2

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

>> Source/WebCore/rendering/RenderLayer.h:242
>> +    bool hasHorizontalScrollbar();
> 
> Should these be const?

Yes, they should.  cq- so I can fix that.

[Antonio Gomes]

Comment on attachment 80522 [details]
Followon patch v2

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

> Source/WebCore/rendering/RenderBox.cpp:638
> +    if (!canBeProgramaticallyScrolled(false))

Since you are here, could you also add a comment styled "false /*XXX*/" to make clear for readers of the code what the parameter is?

[Peter Kasting]

Comment on attachment 80522 [details]
Followon patch v2

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

>> Source/WebCore/rendering/RenderBox.cpp:638
>> +    if (!canBeProgramaticallyScrolled(false))
> 
> Since you are here, could you also add a comment styled "false /*XXX*/" to make clear for readers of the code what the parameter is?

Actually I think that arg is dead.

[Peter Kasting]

Created attachment 94518 [details]
Followon patch v3

This version constifies the two functions Eric asked about, and removes unused args from a few other functions.

[WebKit Commit Bot]

Comment on attachment 94518 [details]
Followon patch v3

Rejecting attachment 94518 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2

Last 500 characters of output:
electors .......................................................................................................
fast/spatial-navigation .....
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html -> failed
............
fast/spatial-navigation/snav-iframe-with-offscreen-focusable-element.html -> failed

Exiting early after 20 failures. 11150 tests run.
293.10s total testing time

11130 test cases (99%) succeeded
20 test cases (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8729415

[WebKit Commit Bot]

Created attachment 94575 [details]
Archive of layout-test-results from cr-jail-7

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-7  Port: Mac  Platform: Mac OS X 10.6.7

[Kenneth Rohde Christiansen]

Some platforms, like iOS uses two finger panning as a way to emulate wheel events and thus be able to scroll divs with overflow. They might be using a separate code path, but I thought that I should let you aware of this.

[Kenneth Rohde Christiansen]

(In reply to comment #39)
> Some platforms, like iOS uses two finger panning as a way to emulate wheel events and thus be able to scroll divs with overflow. They might be using a separate code path, but I thought that I should let you aware of this.

Sorry too early here :-) I didn't notice the :hidden

[Peter Kasting]

Created attachment 94862 [details]
Followon patch v4

This version reverts an unintentional change that broke calculation of |parentLayer| in RenderLayer::scrollRectToVisible().

[WebKit Review Bot]

Attachment 94862 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 94862 [details]
Followon patch v4

Attachment 94862 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8737076

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html

[WebKit Review Bot]

Created attachment 94917 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Peter Kasting]

Created attachment 95039 [details]
Followon patch v5

This reduces the scope of the change to only affect pan scrolls specifically, and avoid affecting anyone else who's currently checking for whether an area can be programmatically scrolled.

By now the patch has changed enough from the reviewed version that I'm more comfortable asking for re-review.

[WebKit Review Bot]

Attachment 95039 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/EventHandler.cpp:533:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 95041 [details]
Followon patch v6

Fix style nit.

[Antonio Gomes]

Comment on attachment 95041 [details]
Followon patch v6

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

Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.

Can we go for it?

> Source/WebCore/ChangeLog:7
> +        wheel scrolls.  This also removes some unused arguments from a couple of

>>> "    " This. 

To much space.

[Peter Kasting]

(In reply to comment #48)
> (From update of attachment 95041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95041&action=review
> 
> Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.

Sure, that's easy.  Filed bug 61648.  I'll break the patch into two pieces and post one each on that bug and this one.

[Antonio Gomes]

(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 95041 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95041&action=review
> > 
> > Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.
> 
> Sure, that's easy.  Filed bug 61648.  I'll break the patch into two pieces and post one each on that bug and this one.

Thanks. Shame that I got this:

Access Denied
You are not authorized to access PR #61648.

[Peter Kasting]

(In reply to comment #50)
> Access Denied
> You are not authorized to access PR #61648.

That's weird.  I promise I didn't mark it as a security bug...

[Peter Kasting]

Created attachment 95818 [details]
Followon patch v7

Now without unrelated cleanup.

[WebKit Review Bot]

Comment on attachment 95818 [details]
Followon patch v7

Attachment 95818 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8754871

[Peter Kasting]

Created attachment 95840 [details]
Followon patch v8

[Peter Kasting]

Created attachment 95929 [details]
Followon patch v9

[Kenneth Rohde Christiansen]

Comment on attachment 95929 [details]
Followon patch v9

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

> Source/WebCore/dom/Node.cpp:2808
> +            // Allow pan-scrolling only on areas with scrollbars, to match the
> +            // behavior of the mouse wheel and arrow keys.

I dont like this description because there are platforms not showing scrollbars (and maybe not even scroll indicators). Would it be possible to put it differently?

> Source/WebCore/dom/Node.cpp:2819
> +            while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(true)))

I wonder what that true means, coudl you add /* description */ true ?

> Source/WebCore/page/EventHandler.cpp:536
> +    if (toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(false))

same here

> Source/WebCore/rendering/RenderBox.cpp:642
> +bool RenderBox::canBeScrolledAndHasScrollableArea(bool mustHaveScrollbar) const

What is platforms disable scrollbars? I dont like this depending on actually having scrollbars, but well.

> Source/WebCore/rendering/RenderLayer.cpp:1284
> +    return frameView && frameView->verticalScrollbar();

I expect some platforms to not even create the scrollbars, maybe iOS, Android, others... how will this work?

[Peter Kasting]

This patch is merely supposed to make pan-scrolling act like wheel and arrow scrolling already do.  Platforms with no scrollbars are presumably not using pan scrolling (or wheel or arrow scrolling) at all, but rather gestural scrolling.  I don't have the ability to compile iOS or Android, so I can't tell you for certain.  But I can't see why this would have any effect.

[Eric Seidel (no email)]

Comment on attachment 80522 [details]
Followon patch v2

Cleared Eric Seidel's review+ from obsolete attachment 80522 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 95929 [details]
Followon patch v9

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

Seems like this needs a refresh to be landed.

>> Source/WebCore/dom/Node.cpp:2819
>> +            while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(true)))
> 
> I wonder what that true means, coudl you add /* description */ true ?

In general when that's the case, we should be usign an enum instead of a bool.

Is this the same bug?

http://jsfiddle.net/dNk9v/ 

(Navigating with the TAB key should NOT break the transition properly working when clicking the tabs - legend tags)

In this example, the form inside a container with overflow:hidden is MOVED somehow to display the focused field, but such motion is NOT reflected in ANY CSS value, so it's impossible to control/prevent the transition to break.

More info here  http://stackoverflow.com/questions/20579803/how-do-i-stop-the-browser-from-jumping-to-the-focused-field

Regardless of the solution, whatever scrolls/shifts/moves/pulls the form field into the visible area of the container, should be reflected in the css, so we can control it.

As it is right now, a simple css transition that could be made with one line of JS, requires tens of lines to prevent defaults and manage EVERYTHING (focus, keydown, keyup, submit events… just because the focused field pulled into the visible area of the container BREAKS the css transition (the transition moves the form as I requested, not aware of whatever property the browser changed)

Or should I open another ticket?