Bug 138083

Summary: [EFL] Apply adjustScrollPositionWithinRange() to scroll offset for ScrollingCoordinatorCoordinatedGraphics.
Product: WebKit Reporter: KwangHyuk <hyuki.kim>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: cmarcelo, commit-queue, gyuyoung.kim, jamesr, luiz, ryuan.choi, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description KwangHyuk 2014-10-26 08:00:14 PDT
Apply adjustScrollPositionWithinRange() to the new offset on ScrollView's scrollTo() because it can apply the new offset even though the offset value is not in the scrollable range when delegatesScrolling is used and scroll is activated by wheel event.
Comment 1 KwangHyuk 2014-10-26 08:07:56 PDT
Created attachment 240465 [details]
Patch
Comment 2 Gyuyoung Kim 2014-10-27 17:50:07 PDT
Comment on attachment 240465 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests because of no behavior change.

I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.
Comment 3 Gyuyoung Kim 2014-10-27 17:53:57 PDT
Comment on attachment 240465 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        No new tests because of no behavior change.
> 
> I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.

Can't you add new test in order to test this change ? It looks we need to add a test which can handle when the offset value is not within the scrollable range.
Comment 4 KwangHyuk 2014-10-30 10:22:13 PDT
(In reply to comment #3)
> Comment on attachment 240465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240465&action=review
> 
> >> Source/WebCore/ChangeLog:12
> >> +        No new tests because of no behavior change.
> > 
> > I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.
> 
> Can't you add new test in order to test this change ? It looks we need to
> add a test which can handle when the offset value is not within the
> scrollable range.

Ok, let me check the WKViewScrollTo.cpp.

However, this is a just bug fix for wheel scroll using mouse.
Of course, you can reproduce issue easily using webkit.org.
Just go to www.webkit.org and do the wheel scroll to the upper direction 10 or more times, then do the wheel scroll to the down direction. you can notice that it will not scroll down for 10 times even though there is no webview changed.
Comment 5 Gyuyoung Kim 2015-02-03 07:38:46 PST
Comment on attachment 240465 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:12
>>>> +        No new tests because of no behavior change.
>>> 
>>> I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.
>> 
>> Can't you add new test in order to test this change ? It looks we need to add a test which can handle when the offset value is not within the scrollable range.
> 
> Ok, let me check the WKViewScrollTo.cpp.
> 
> However, this is a just bug fix for wheel scroll using mouse.
> Of course, you can reproduce issue easily using webkit.org.
> Just go to www.webkit.org and do the wheel scroll to the upper direction 10 or more times, then do the wheel scroll to the down direction. you can notice that it will not scroll down for 10 times even though there is no webview changed.

> Ok, let me check the WKViewScrollTo.cpp.

Any update on this check ?
Comment 6 KwangHyuk 2015-04-13 10:49:27 PDT
Created attachment 250657 [details]
Patch
Comment 7 KwangHyuk 2015-04-13 10:54:47 PDT
As this patch just touched code enclosed by TILED_BACKING_STORE, it will not effect for both iOS and GTK for now.
Comment 8 KwangHyuk 2015-06-09 09:52:51 PDT
Created attachment 254575 [details]
Patch
Comment 9 KwangHyuk 2015-06-09 10:09:02 PDT
* Issue description
- go to www.webkit.org by using MiniBrowser
- scroll down and repeat scroll up using mouse wheel.
- scroll up was applied several times although rendered view was not changed.
  . you can check it by trying scroll down as scroll down will not change rendered view either.
Comment 10 Ryuan Choi 2015-06-09 19:24:51 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 240465 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240465&action=review
> > 
> > >> Source/WebCore/ChangeLog:12
> > >> +        No new tests because of no behavior change.
> > > 
> > > I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.
> > 
> > Can't you add new test in order to test this change ? It looks we need to
> > add a test which can handle when the offset value is not within the
> > scrollable range.
> 
> Ok, let me check the WKViewScrollTo.cpp.
> 
> However, this is a just bug fix for wheel scroll using mouse.
> Of course, you can reproduce issue easily using webkit.org.
> Just go to www.webkit.org and do the wheel scroll to the upper direction 10
> or more times, then do the wheel scroll to the down direction. you can
> notice that it will not scroll down for 10 times even though there is no
> webview changed.

Why don't you make the test case like you mentioned?

In my two cents,
we can make the test case that scroll more than two times to invalid position and scroll few pixel (but valid).

I checked some use cases and reproduced this issue with very simple test cases that use windows.scrollTo and/or with fixed elements.
Comment 11 KwangHyuk 2015-06-10 09:03:50 PDT
(In reply to comment #10)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 240465 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=240465&action=review
> > > 
> > > >> Source/WebCore/ChangeLog:12
> > > >> +        No new tests because of no behavior change.
> > > > 
> > > > I'm not sure this change won't change behavior. At least, this patch should pass WKViewScrollTo.cpp test.
> > > 
> > > Can't you add new test in order to test this change ? It looks we need to
> > > add a test which can handle when the offset value is not within the
> > > scrollable range.
> > 
> > Ok, let me check the WKViewScrollTo.cpp.
> > 
> > However, this is a just bug fix for wheel scroll using mouse.
> > Of course, you can reproduce issue easily using webkit.org.
> > Just go to www.webkit.org and do the wheel scroll to the upper direction 10
> > or more times, then do the wheel scroll to the down direction. you can
> > notice that it will not scroll down for 10 times even though there is no
> > webview changed.
> 
> Why don't you make the test case like you mentioned?
> 
> In my two cents,
> we can make the test case that scroll more than two times to invalid
> position and scroll few pixel (but valid).
> 
> I checked some use cases and reproduced this issue with very simple test
> cases that use windows.scrollTo and/or with fixed elements.

Ok, let me try. :)
Comment 12 Gyuyoung Kim 2015-07-16 20:18:20 PDT
Comment on attachment 254575 [details]
Patch

Clearing r? until new patch is uploaded.
Comment 13 KwangHyuk 2015-08-27 04:14:00 PDT
Confirmed that fixed by 146959.

*** This bug has been marked as a duplicate of bug 146959 ***