Bug 138083 – [EFL] Apply adjustScrollPositionWithinRange() to scroll offset for ScrollingCoordinatorCoordinatedGraphics.

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[KwangHyuk]

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.

[KwangHyuk]

Created attachment 240465 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.

[KwangHyuk]

(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.

[Gyuyoung Kim]

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 ?

[KwangHyuk]

Created attachment 250657 [details]
Patch

[KwangHyuk]

As this patch just touched code enclosed by TILED_BACKING_STORE, it will not effect for both iOS and GTK for now.

[KwangHyuk]

Created attachment 254575 [details]
Patch

[KwangHyuk]

* 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.

[Ryuan Choi]

(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.

[KwangHyuk]

(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. :)

[Gyuyoung Kim]

Comment on attachment 254575 [details]
Patch

Clearing r? until new patch is uploaded.

[KwangHyuk]

Confirmed that fixed by 146959.

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