Bug 139757 – Delegates scrolling by wheel event should be work within contents area.

• Component: UI Events
• Status: NEW

[Sanghyup Lee]

Currently scroll position which changed by mouse wheel can be negative or exceed maximum scroll position when fixed layout is enabled.

[Sanghyup Lee]

Created attachment 243488 [details]
Patch

[Darin Adler]

Comment on attachment 243488 [details]
Patch

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

A bug fix like this needs a regression test. Could you please write a regression test that demonstrates the bug is fixed and add that to this patch? Or if that’s not possible, you could document why a regression test was impossible or impractical to make in the change log.

> Source/WebCore/ChangeLog:3
> +        [EFL] Delegates scrolling by wheel event should be work within contents area.

This patch should affect any platform that uses delegatesScrolling, not just EFL, so it’s not appropriate for the bug title to say [EFL].

> Source/WebCore/page/FrameView.cpp:4351
> +            ScrollView::setScrollOffset(IntPoint(newOffset));

Should call setScrollOffset, not ScrollView::setScrollOffset.

[Sanghyup Lee]

(In reply to comment #2)
> Comment on attachment 243488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243488&action=review
> 
> A bug fix like this needs a regression test. Could you please write a
> regression test that demonstrates the bug is fixed and add that to this
> patch? Or if that’s not possible, you could document why a regression test
> was impossible or impractical to make in the change log.
> 
> > Source/WebCore/ChangeLog:3
> > +        [EFL] Delegates scrolling by wheel event should be work within contents area.
> 
> This patch should affect any platform that uses delegatesScrolling, not just
> EFL, so it’s not appropriate for the bug title to say [EFL].
> 
> > Source/WebCore/page/FrameView.cpp:4351
> > +            ScrollView::setScrollOffset(IntPoint(newOffset));
> 
> Should call setScrollOffset, not ScrollView::setScrollOffset.

Thank you for your review. I'll fix it.

[Sanghyup Lee]

Created attachment 246427 [details]
Patch

[Sanghyup Lee]

Created attachment 246429 [details]
Patch

[Build Bot]

Comment on attachment 246429 [details]
Patch

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

New failing tests:
fast/events/scroll-delegates-wheelevent.html

[Build Bot]

Created attachment 246434 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 246429 [details]
Patch

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

New failing tests:
fast/events/scroll-delegates-wheelevent.html

[Build Bot]

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

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

[Sanghyup Lee]

Created attachment 246588 [details]
Patch

[Build Bot]

Comment on attachment 246588 [details]
Patch

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

New failing tests:
fast/events/scroll-delegates-wheelevent.html

[Build Bot]

Created attachment 246589 [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

[Csaba Osztrogonác]

Comment on attachment 246588 [details]
Patch

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

Is the Mac failure real or false positive?

> Source/WebCore/ChangeLog:4
> +        [WK2] Delegates scrolling by wheel event should be work within contents area.
> +        https://bugs.webkit.org/show_bug.cgi?id=139757

Why WK2, when you touch only one source which isn't in WebKit2 - Source/WebCore/page/FrameView.cpp ?

[Sanghyup Lee]

(In reply to comment #13)
> Comment on attachment 246588 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246588&action=review
> 
> Is the Mac failure real or false positive?
After check it, I'll leave comment.

> 
> > Source/WebCore/ChangeLog:4
> > +        [WK2] Delegates scrolling by wheel event should be work within contents area.
> > +        https://bugs.webkit.org/show_bug.cgi?id=139757
> 
> Why WK2, when you touch only one source which isn't in WebKit2 -
> Source/WebCore/page/FrameView.cpp ?
I Fixed it.

[Sanghyup Lee]

Created attachment 252790 [details]
Patch

[Build Bot]

Comment on attachment 252790 [details]
Patch

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

New failing tests:
fast/events/scroll-delegates-wheelevent.html

[Build Bot]

Created attachment 252793 [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

[Darin Adler]

Comment on attachment 252790 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:4460
>          IntPoint oldPosition = scrollPosition();
>          IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY());
>          if (offset != newOffset) {
> -            ScrollView::scrollTo(newOffset);
> +            setScrollOffset(IntPoint(newOffset));
>              scrollPositionChanged(oldPosition, scrollPosition());
>              didChangeScrollOffset();
>          }

With this patch as posted for review, scrollPositionChanged and didChangeScrollOffset will be called twice. Instead of these 8 lines of code, I suggest this one line of code:

    setScrollOffset(scrollPosition() + IntSize(-wheelEvent.deltaX(), -wheelEvent.deltaY()));

I didn’t look at the failure on Mac; the one to that is running tests in this mode returned a failure.

[Sanghyup Lee]

(In reply to comment #18)
> Comment on attachment 252790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252790&action=review
> 
> > Source/WebCore/page/FrameView.cpp:4460
> >          IntPoint oldPosition = scrollPosition();
> >          IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY());
> >          if (offset != newOffset) {
> > -            ScrollView::scrollTo(newOffset);
> > +            setScrollOffset(IntPoint(newOffset));
> >              scrollPositionChanged(oldPosition, scrollPosition());
> >              didChangeScrollOffset();
> >          }
> 
> With this patch as posted for review, scrollPositionChanged and
> didChangeScrollOffset will be called twice. Instead of these 8 lines of
> code, I suggest this one line of code:
> 
>     setScrollOffset(scrollPosition() + IntSize(-wheelEvent.deltaX(),
> -wheelEvent.deltaY()));
> 
> I didn’t look at the failure on Mac; the one to that is running tests in
> this mode returned a failure.

Thank you for your kind review and I'll fix it.

As far as I know, delegate scrolling is for WK2. 
So Can I skip this test on Mac?

[Sanghyup Lee]

Created attachment 252854 [details]
Patch

[Darin Adler]

Comment on attachment 252854 [details]
Patch

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

review- because skipping this test on Mac only isn’t right. But we can’t see that problem with EWS because currently only Mac EWS runs tests, so skipping on Mac is enough to make EWS happy.

Someone needs to test this on iOS to see that it works as expected there. Could cause problems there because the scrolling UI involves rubber banding; this might interfere with that, maybe not.

> LayoutTests/platform/mac/TestExpectations:287
> +# This port doesn't support delegate scrolling
> +fast/events/scroll-delegates-wheelevent.html [ WontFix ]

This doesn’t seem right. Mac is not the only port that has this issue. It’s just that the EWS bot only runs the test on Mac. So skipping this only for Mac is incorrect. I bet this also doesn’t work on Windows, and perhaps some other platforms as well.

As far as I can tell, the “delegates scrolling” code path is used on iOS, and on platforms that use WebKit2 with COORDINATED_GRAPHICS. We shouldn’t be testing it on any other platforms unless we expect it to work.

So just skipping this on Mac is not right.

[Darin Adler]

Maybe there’s no wheel event on iOS?

[Darin Adler]

“delegates scrolling” is used in both WebKit 1 and WebKit 2 on iOS.