Bug 139757 - Delegates scrolling by wheel event should be work within contents area.
Summary: Delegates scrolling by wheel event should be work within contents area.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-17 16:35 PST by Sanghyup Lee
Modified: 2015-05-11 09:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2014-12-18 03:34 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2015-02-11 22:48 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2015-02-11 22:56 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (523.39 KB, application/zip)
2015-02-11 23:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (667.48 KB, application/zip)
2015-02-11 23:49 PST, Build Bot
no flags Details
Patch (5.07 KB, patch)
2015-02-14 05:07 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (551.56 KB, application/zip)
2015-02-14 05:52 PST, Build Bot
no flags Details
Patch (5.13 KB, patch)
2015-05-09 17:48 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (535.34 KB, application/zip)
2015-05-09 18:36 PDT, Build Bot
no flags Details
Patch (5.95 KB, patch)
2015-05-11 03:50 PDT, Sanghyup Lee
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sanghyup Lee 2014-12-17 16:35:08 PST
Currently scroll position which changed by mouse wheel can be negative or exceed maximum scroll position when fixed layout is enabled.
Comment 1 Sanghyup Lee 2014-12-18 03:34:42 PST
Created attachment 243488 [details]
Patch
Comment 2 Darin Adler 2014-12-18 09:22:36 PST
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.
Comment 3 Sanghyup Lee 2014-12-18 16:46:58 PST
(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.
Comment 4 Sanghyup Lee 2015-02-11 22:48:32 PST
Created attachment 246427 [details]
Patch
Comment 5 Sanghyup Lee 2015-02-11 22:56:40 PST
Created attachment 246429 [details]
Patch
Comment 6 Build Bot 2015-02-11 23:43:20 PST
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
Comment 7 Build Bot 2015-02-11 23:43:23 PST
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
Comment 8 Build Bot 2015-02-11 23:49:36 PST
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
Comment 9 Build Bot 2015-02-11 23:49:39 PST
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
Comment 10 Sanghyup Lee 2015-02-14 05:07:58 PST
Created attachment 246588 [details]
Patch
Comment 11 Build Bot 2015-02-14 05:52:54 PST
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
Comment 12 Build Bot 2015-02-14 05:52:58 PST
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
Comment 13 Csaba Osztrogonác 2015-02-16 07:30:04 PST
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 ?
Comment 14 Sanghyup Lee 2015-02-16 17:10:22 PST
(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.
Comment 15 Sanghyup Lee 2015-05-09 17:48:00 PDT
Created attachment 252790 [details]
Patch
Comment 16 Build Bot 2015-05-09 18:36:39 PDT
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
Comment 17 Build Bot 2015-05-09 18:36:43 PDT
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
Comment 18 Darin Adler 2015-05-10 13:53:08 PDT
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.
Comment 19 Sanghyup Lee 2015-05-10 20:43:48 PDT
(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?
Comment 20 Sanghyup Lee 2015-05-11 03:50:17 PDT
Created attachment 252854 [details]
Patch
Comment 21 Darin Adler 2015-05-11 09:54:01 PDT
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.
Comment 22 Darin Adler 2015-05-11 09:54:19 PDT
Maybe there’s no wheel event on iOS?
Comment 23 Darin Adler 2015-05-11 09:54:50 PDT
“delegates scrolling” is used in both WebKit 1 and WebKit 2 on iOS.