Bug 115346 – Do not apply scroll position if scrolling is delegated.

• Status: NEW
• Component: WebCore Misc.

[Eunmi Lee]

If the scrolling is delegated, we don't have to apply scroll position in the ScrollView to convert rootView to contents or contents to rootView.
The scroll position already does not applied to convert window to contents or contents to window,
so I add codes only for rootView to contents and contents to rootView.

[Eunmi Lee]

Created attachment 199988 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 199988 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Do not apply scroll position if scrolling is delegated.

think it's worth saying who should not apply it :)

> Source/WebCore/ChangeLog:9
> +        convert rootView to contents and contents to rootView.

could you please give more details on why it is not needed?

> Source/WebCore/ChangeLog:10
> +

what about test?

[Eunmi Lee]

Comment on attachment 199988 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +
> 
> what about test?

Thanks, I will update changelog and make a test for that :)

[Eunmi Lee]

Comment on attachment 199988 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +
>> 
>> what about test?
> 
> Thanks, I will update changelog and make a test for that :)

Hmm, it is hard to make test.
The rootViewToContents and contentsToRootView is used when we do hit test in the 'sub frame', and the result is wrong without this patch.
But I can not find the case which uses the hit test using sub frame and most hit test is done in the main frame.
How can I make test case in this situation?

[Radu Stavila]

Comment on attachment 199988 [details]
Patch

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

> Source/WebCore/platform/ScrollView.cpp:715
> +        return convertFromRootView(rootViewPoint);

I think it would be better to have a single call to convertFromRootView. So, move the "if (delegatesScrolling())" after IntPoint viewPoint = convertFromRootView(rootViewPoint) and just return viewPoint; Otherwise, return viewPoint + scrollOffset....

> Source/WebCore/platform/ScrollView.cpp:724
> +        return convertToRootView(contentsPoint);

Seems like this should be convertToRootView(contentsPoint + IntSize(0, headerHeight())) ?

> Source/WebCore/platform/ScrollView.cpp:734
> +

Same comment about having a single call to convertFromRootView

> Source/WebCore/platform/ScrollView.cpp:744
> +

Ditto about the single call AND about the headerHeight

[Gyuyoung Kim]

This patch is too old. I'm not sure if this patch is still valid. Eunmi ?

[Simon Fraser (smfr)]

Comment on attachment 199988 [details]
Patch

This would need to be tested on iOS, which uses delegated scrolling.

[Brady Eidson]

Comment on attachment 199988 [details]
Patch

r- to clear stale patches from the review queue