Bug 111413 – Support wheel event even when FrameView does not have scrollbars.

• Component: Layout and Rendering
• Status: NEW

[Dongseong Hwang]

Currently, when FrameView does not have scrollbars, FrameView does not support wheel event. However, our mouse can have wheel button without scrollbars in FrameView. We can emulate wheel event as delegates scrolling does.
So this patch make FrameView support wheel event even when FrameView does not have scrollbars.

In addition, EFL WK2 will turn off delegates scrolling, so this patch will be needed.

[Dongseong Hwang]

This policy has been so long time.
I quote as follows from r37159.
 void ScrollView::wheelEvent(PlatformWheelEvent& e)
 {
-    if (!allowsScrolling() || platformWidget())
+    // We don't allow mouse wheeling to happen in a ScrollView that has had its scrollbars explicitly disabled.
+    if (!canHaveScrollbars() || platformWidget())
         return;

I assume there is no ports that do not have scrollbars without having delegates scrolling, because I think that wheel event is too essential to be disabled.

[Dongseong Hwang]

Created attachment 191423 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 191423 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3894
> -    if (delegatesScrolling()) {
> -        IntSize offset = scrollOffset();
> -        IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY());
> -        if (offset != newOffset) {
> -            ScrollView::scrollTo(newOffset);
> -            scrollPositionChanged();
> -            frame()->loader()->client()->didChangeScrollOffset();
> -        }
> +    if (delegatesScrolling() || !canHaveScrollbars()) {
> +        IntPoint position = scrollPosition();
> +        IntPoint newPosition = position - IntSize(wheelEvent.deltaX(), wheelEvent.deltaY());
> +        newPosition = adjustScrollPositionWithinRange(newPosition);
> +        if (position != newPosition)
> +            scrollTo(IntSize(newPosition.x(), newPosition.y()));

Is scrollPositionChanged() and frame()->loader()->client()->didChangeScrollOffset() no longer needed?

Two, are you sure every port that disables scrollbars wants the wheel-event redirected to scrollTo? The deleted comment below must have been there for a reason.

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 191423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review
> 
> > Source/WebCore/page/FrameView.cpp:3894
> > -    if (delegatesScrolling()) {
> > -        IntSize offset = scrollOffset();
> > -        IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY());
> > -        if (offset != newOffset) {
> > -            ScrollView::scrollTo(newOffset);
> > -            scrollPositionChanged();
> > -            frame()->loader()->client()->didChangeScrollOffset();
> > -        }
> > +    if (delegatesScrolling() || !canHaveScrollbars()) {
> > +        IntPoint position = scrollPosition();
> > +        IntPoint newPosition = position - IntSize(wheelEvent.deltaX(), wheelEvent.deltaY());
> > +        newPosition = adjustScrollPositionWithinRange(newPosition);
> > +        if (position != newPosition)
> > +            scrollTo(IntSize(newPosition.x(), newPosition.y()));
> 

Thanks for review :)

> Is scrollPositionChanged() and frame()->loader()->client()->didChangeScrollOffset() no longer needed?
Good point! FrameView::scrollTo does the same thing.

void FrameView::scrollTo(const IntSize& newOffset)
{
    LayoutSize offset = scrollOffset();
    ScrollView::scrollTo(newOffset);
    if (offset != scrollOffset())
        scrollPositionChanged();
    frame()->loader()->client()->didChangeScrollOffset();
}

> Two, are you sure every port that disables scrollbars wants the wheel-event redirected to scrollTo? The deleted comment below must have been there for a reason.

I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008.

[Allan Sandfeld Jensen]

(In reply to comment #4)
> I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008.

No, it is only a year old see bug 78731 and http://trac.webkit.org/changeset/107838

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (In reply to comment #4)
> > I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008.
> 
> No, it is only a year old see bug 78731 and http://trac.webkit.org/changeset/107838

Nah, sorry. That was only moving code around.

[Build Bot]

Comment on attachment 191423 [details]
Patch

Attachment 191423 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17009118

New failing tests:
editing/selection/selection-invalid-offset.html

[Simon Fraser (smfr)]

Comment on attachment 191423 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:3889
> +    if (delegatesScrolling() || !canHaveScrollbars()) {

This seems like a behavior change, which some platforms don't want.

[Dongseong Hwang]

(In reply to comment #7)
> (From update of attachment 191423 [details])
> Attachment 191423 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17009118
> 
> New failing tests:
> editing/selection/selection-invalid-offset.html

I think it is flaky, because IMO this test is not related to wheel event.

(In reply to comment #8)
> (From update of attachment 191423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review
> 
> > Source/WebCore/page/FrameView.cpp:3889
> > +    if (delegatesScrolling() || !canHaveScrollbars()) {
> 
> This seems like a behavior change, which some platforms don't want.

Thank you feedback. could you let me know which specific platforms would have problem?

And I'm curious if it does not make sense to support wheel event when FrameView can not have scrollbars. otherwise, what is relevant way to support wheel event?

If turning off, EFL WK2 can not deal with wheel event because EFL WK2 does not have scrollbars.

[Dongseong Hwang]

(In reply to comment #9)
> If turning off, EFL WK2 can not deal with wheel event because EFL WK2 does not have scrollbars.

s/If turning off/If turning off delegates scrolling/
I'm trying to turn off delegates scrolling in EFL and Qt WK2 now, because I think works of Mac and Chromium can cover the requirement of delegated scrolling now.