Fix rubber-band effect on non-scrollable pages

[Christopher Cameron]

[chromium] Fix rubber-band effect on non-scrollable pages

[Christopher Cameron]

Created attachment 184098 [details]
Patch

[Christopher Cameron]

This reverts a part of
https://bugs.webkit.org/show_bug.cgi?id=105632
in order to fix rubber-band over-scrolling in Chrome.

[Antonio Gomes]

Comment on attachment 184098 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:-3777
> -    if (!isScrollable())

can not isScrollable get relaxed instead? so it could take overscroll/rubber-band into account.

enum ScrollabilityStrategy { ConsiderRubberBand, IgnoreRubberBand };
bool isScrollable(ScrollabilityStrategy strategy)
{
    // Even non overflown views are scrollable if rubber-band is enforced
    if (strategy == ConsiderRubberBand)
        return true;
    (...)
}

[James Robinson]

Comment on attachment 184098 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [chromium] Fix rubber-band effect on non-scrollable pages

don't mark a patch [chromium] if it touches cross-platform code

[Christopher Cameron]

Created attachment 184267 [details]
Patch

[Christopher Cameron]

(In reply to comment #3)
> (From update of attachment 184098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184098&action=review
> 
> > Source/WebCore/page/FrameView.cpp:-3777
> > -    if (!isScrollable())
> 
> can not isScrollable get relaxed instead? so it could take overscroll/rubber-band into account.

Maybe it would be best to add this comment to the original site of the removed isScrollable.  I've updated the patch to say explicitly that 
    // Note that even non-scrollable views should handle wheel events, to allow for rubber-band
    // over-scroll behavior
This way, it avoids hanging the other call to isScrollable in updateScrollableAreaSet.  Does this look good to you?

(In reply to comment #4)
> don't mark a patch [chromium] if it touches cross-platform code

Sorry, fixed.

[Christopher Cameron]

(In reply to comment #6)
> This way, it avoids hanging the other call to 

This should read "changing the other call", not "hanging the other call"

[Antonio Gomes]

Comment on attachment 184267 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3778
> -    if (!isScrollable())
> -        return false;
> +    // Note that even non-scrollable views should handle wheel events, to allow for rubber-band
> +    // over-scroll behavior.

That would work, but what about wrapping the if statement with  #if !ENABLE(RUBBER_BANDING) ?

[Christopher Cameron]

Created attachment 184284 [details]
Patch

[Christopher Cameron]

(In reply to comment #8)
> (From update of attachment 184267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184267&action=review
> 
> > Source/WebCore/page/FrameView.cpp:3778
> > -    if (!isScrollable())
> > -        return false;
> > +    // Note that even non-scrollable views should handle wheel events, to allow for rubber-band
> > +    // over-scroll behavior.
> 
> That would work, but what about wrapping the if statement with  #if !ENABLE(RUBBER_BANDING) ?

Sure -- updated.

[Antonio Gomes]

Btw, it should be USE instead of ENABLE

[Christopher Cameron]

Thanks!

(In reply to comment #11)
> Btw, it should be USE instead of ENABLE

Good point -- "optional OS service" does match the conditions for its use (particular OS version) more than "turn on a specific feature" (but ENABLE_RUBBER_BANDING is fairly embedded).

[Nico Weber]

I hate to be that guy, but is there some way this could be tested? I remember Alexei added overflow paint tests somewhere. Is that not enough testing infrastructure to cover this?

[Nico Weber]

This added the tests we have: http://trac.webkit.org/changeset/107711

[Christopher Cameron]

(In reply to comment #13)
> is there some way this could be tested?

I should be able to add a WebCore::Internals::wheelEvent (similar to Internals::setScrollViewPosition) to hook into the FrameView at a higher level. Should't be too complicated, unless I run into platform-specific idiosyncrasies along the line.

[James Robinson]

Take a look at the eventSender interface (exposed only to DumpRenderTree) - can it generate the mousewheel events you want?

[Christopher Cameron]

(In reply to comment #16)
> Take a look at the eventSender interface (exposed only to DumpRenderTree) - can it generate the mousewheel events you want?

Thanks -- it looks like they can be added in there.  Do you want me to hold off on committing this until I add the tests?

[Antonio Gomes]

(In reply to comment #17)
> (In reply to comment #16)
> > Take a look at the eventSender interface (exposed only to DumpRenderTree) - can it generate the mousewheel events you want?
> 
> Thanks -- it looks like they can be added in there.  Do you want me to hold off on committing this until I add the tests?

Yes. It should be less than some hours to come up with a test.

[Antonio Gomes]

Comment on attachment 184284 [details]
Patch

Lets add a test. :%s/r+/r-/g

[Christopher Cameron]

Created attachment 185019 [details]
Patch

[Christopher Cameron]

(In reply to comment #20)
> Created an attachment (id=185019) [details]
> Patch

I've added a single test (testing over-scroll in one direction).  I'll add the other directions, but please verify that you like the structure of the test first.

I added a beginMouseWheel function to EventSender (since this needs to bracket the scroll events).  I did not add an endMouseWheel because it won't be used in this test.  LMK if you'd like it for completeness).

Also, LMK if you'd prefer that any part of beginMouseWheel be parameterized (e.g, hasPreciseScrollingDeltas).

[Nico Weber]

Comment on attachment 185019 [details]
Patch

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

Nice! Looks good to me (but I'm not a reviewer); one question about the naming though:

> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:1048
> +void EventSender::mouseWheelBegin(const CppArgumentList& arguments, CppVariant* result)

Should this be called mouseDragBegin() as it's not really a wheel event?

[Build Bot]

Comment on attachment 185019 [details]
Patch

Attachment 185019 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16160773

New failing tests:
fast/workers/worker-close-more.html

[Christopher Cameron]

Created attachment 185116 [details]
Patch

[Christopher Cameron]

(In reply to comment #22)
> (From update of attachment 185019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185019&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:1048
> > +void EventSender::mouseWheelBegin(const CppArgumentList& arguments, CppVariant* result)
> 
> Should this be called mouseDragBegin() as it's not really a wheel event?

I've renamed this (though I'm entirely flexible on the name).  I've also added the remaining cardinal directions to the tests.

[Christopher Cameron]

Review ping.

[Antonio Gomes]

(In reply to comment #26)
> Review ping.

James, could you sign-off the chromium specific bits?

[Christopher Cameron]

(In reply to comment #27)
> (In reply to comment #26)
> > Review ping.
> 
> James, could you sign-off the chromium specific bits?

Ping.

[James Robinson]

Comment on attachment 185116 [details]
Patch

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

chromium parts seem fine

> LayoutTests/platform/chromium/rubberbanding/event-overhang-e.html:1
> +<html>

<!DOCTYPE html> on this and all other new html files

[Christopher Cameron]

Created attachment 185817 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185817 [details]
Patch

Clearing flags on attachment: 185817

Committed r141514: <http://trac.webkit.org/changeset/141514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.