Bug 146693 – Rubber banding is broken when using a Mighty Mouse

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

Scrolling against the edge of a container with a mighty mouse causes the offset to move out of the container.

[Wenson Hsieh]

<rdar://problem/20824782>

[Wenson Hsieh]

Created attachment 256323 [details]
Patch

[Wenson Hsieh]

Created attachment 256325 [details]
Patch

[Brent Fulgham]

Comment on attachment 256325 [details]
Patch

This looks good to me, but I think Beth should double-check in case I'm missing something.

[Beth Dakin]

Comment on attachment 256325 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Wenson Hsieh  <[e-mail]>

You should be using your apple email address here.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1110
> +static bool isStatelessWheelEvent(const PlatformWheelEvent& wheelEvent)

I'm concerned about this phrase "stateless." This does not seem like a good/descriptive-enough name for this to me.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1112
> +    return wheelEvent.phase() == PlatformWheelEventPhaseNone && wheelEvent.momentumPhase() == PlatformWheelEventPhaseNone;

Why is it okay to definitely return early here? Are you certain there is nothing more to do? Has the event already had a chance to be propagated to the DOM? Do we ever get events with this phase configuration for a non-Mighty-mouse? 

This bug, I believe, it unique to overflow scrolling. In other words, it does not happen to the main document. How do we get it right for the main document?

[Wenson Hsieh]

Ok -- I've traced the bug to what I believe is the source. Both mainframe and overflow scrolling go through ScrollController::handleWheelEvent. This, in turn, invokes the scroll client (m_client)'s immediateScrollBy method. This is where things start to diverge.

Mainframe scrolling uses ScrollingTreeFrameScrollingNodeMac::immediateScrollBy, which calls a few methods which end up clamping the destination scroll offset to a min and max. When scrolling against the edge of the mainframe a Mighty Mouse, this clamping operation doesn't cause us to go over the edge of the container, so mainframe Mighty Mouse scrolling behaves as expected.

For overflow scrolling, ScrollAnimatorMac::immediateScrollBy does something similar: before performing the scroll, it consults adjustScrollPositionIfNecessary for the scroll offsets it should land on. However, this *only* clamps the scroll offsets to the min/max if !m_scrollableArea.constrainsScrollingToContentEdge(). This is not the case initially, as the constructor for RenderLayer does not constrain scrolling to the content edge by default! I was able to "fix" this issue by turning this edge constraint on by default, but it's not clear whether this will have unintended consequences for other scrolling behaviors.

It was also reported in the original bug that scrolling was no longer broken once you scrolled within the container for a bit (i.e. if you scrolled down away from the edge and then scrolled back up against the edge, the problem would not occur). I found out that this is due to the current implementation of ScrollAnimatorMac::immediateScrollByWithoutContentEdgeConstraints for overflow scrolling, which calls m_scrollableArea.setConstrainsScrollingToContentEdge(true) regardless of whether or not the ScrollableArea previously constrained scrolling to content edges. I'm not sure whether this is intended behavior.

TL;DR The bug seems to be caused by the value of constrainsScrollingToContentEdge() in ScrollAnimatorMac::adjustScrollPositionIfNecessary.

[Wenson Hsieh]

Created attachment 256494 [details]
Patch

[Wenson Hsieh]

Created attachment 256503 [details]
Patch

[Beth Dakin]

Comment on attachment 256503 [details]
Patch

Looks good!

[Build Bot]

Comment on attachment 256503 [details]
Patch

Attachment 256503 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5744092755525632

New failing tests:
fast/html/marquee-scroll.html
fast/block/float/marquee-shrink-to-avoid-floats.html
fast/html/marquee-scrollamount.html
fast/inline-block/003.html

[Build Bot]

Created attachment 256510 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256503 [details]
Patch

Attachment 256503 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5514735059468288

New failing tests:
fast/html/marquee-scroll.html
fast/block/float/marquee-shrink-to-avoid-floats.html
fast/html/marquee-scrollamount.html
fast/inline-block/003.html

[Build Bot]

Created attachment 256511 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Wenson Hsieh]

Created attachment 256519 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256519 [details]
Patch

Clearing flags on attachment: 256519

Committed r186644: <http://trac.webkit.org/changeset/186644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.