Bug 136917

Summary: [CoordinatedGraphics][EFL] window.scrollTo(x, y) doesn't work when fixed layout is enabled
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, hh.kaka, lucas.de.marchi, ossy, yoon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 136999, 137336    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Gyuyoung Kim 2014-09-18 03:02:15 PDT 
When fixed layout is enabled, scrollTo doesn't work on EFL port with coordinated graphics. Since updateScrollbars() doesn't update scroll position when delegatesScrolling() is enabled. To fix this issue, EFL port should update scroll position based on WK2 port. This patch update scroll position in FrameView::requestScrollPositionUpdate() as well as Mac port.
Comment 1 Gyuyoung Kim 2014-09-18 03:05:51 PDT 
Created attachment 238301 [details]
Patch
Comment 2 Gyuyoung Kim 2014-09-18 03:06:37 PDT 
CC'ing Ossy, Yoon and Benjamin. PTAL !
Comment 3 Grzegorz Czajkowski 2014-09-18 08:00:26 PDT 
Comment on attachment 238301 [details]
Patch

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

Thanks for the patch Gyuyoung. Since I was looking at the issue also, added some ideas that I had in mind.

> Source/WebCore/page/FrameView.cpp:2194
> +#elif USE(TILED_BACKING_STORE)
> +    if (delegatesScrolling()) {
> +        setFixedVisibleContentRect(IntRect(position, visibleContentRect().size()));
> +        hostWindow()->delegatedScrollRequested(position);
> +        return true;
> +    }

Can we override ScrollingCoordinatorCoordinatedGraphics::requestScrollPositionUpdate and reuse code above?

> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/scrollTo.html:2
> +<body style="border:1px solid black;background-color:lightblue;height:2000px;width:2000px;">

Are all those styles necessary? I think height/width are enough to demonstrate this issue.
Comment 4 Gyuyoung Kim 2014-09-18 08:58:34 PDT 
Comment on attachment 238301 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:2194
>> +    }
> 
> Can we override ScrollingCoordinatorCoordinatedGraphics::requestScrollPositionUpdate and reuse code above?

Yes, looks better implementation. Let me adjust it.

>> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/scrollTo.html:2
>> +<body style="border:1px solid black;background-color:lightblue;height:2000px;width:2000px;">
> 
> Are all those styles necessary? I think height/width are enough to demonstrate this issue.

right, those are unnecessary styles.
Comment 5 Gyuyoung Kim 2014-09-18 10:14:16 PDT 
Created attachment 238313 [details]
Patch
Comment 6 Grzegorz Czajkowski 2014-09-18 12:44:19 PDT 
Comment on attachment 238313 [details]
Patch

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

Thanks for update! General concept looks fine for me. I am wondering if this change affects other port especially GTK? Would be nice if someone could have a look at it.

> Source/WebCore/ChangeLog:3
> +        [CoordinatedGraphics][EFL] scrollTo doesn't work when fixed layout is enabled

Nit: I'd explicit write that we're fixing window.scrollTo(x, y)

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:118
> +    if (frameView->delegatesScrolling()) {

Early return ?

> Tools/ChangeLog:11
> +        When fixed layout is enabled, scrollTo doesn't work on EFL port with coordinated graphics.
> +        Since updateScrollbars() doesn't update scroll position when delegatesScrolling() is enabled. 
> +        To fix this issue, EFL port should update scroll position based on WK2 port. This patch
> +        update scroll position in FrameView::requestScrollPositionUpdate() as well as Mac port.

Would be nice to mention that we're adding a test here (not fixing scroll).
Comment 7 Gyuyoung Kim 2014-09-18 18:44:47 PDT 
(In reply to comment #6)
> (From update of attachment 238313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238313&action=review
> 
> Thanks for update! General concept looks fine for me. I am wondering if this change affects other port especially GTK? Would be nice if someone could have a look at it.

No, unfortunately(?) coordinated graphics is only used by EFL port. I think that is a big problem. My long term plan is not to use the coordinated graphics, since there is no expert on this.
Comment 8 Gyuyoung Kim 2014-09-18 18:50:55 PDT 
Created attachment 238346 [details]
Patch
Comment 9 Gyuyoung Kim 2014-09-18 18:51:34 PDT 
Greg, nits you mentioned are all fixed !
Comment 10 Gyuyoung Kim 2014-09-18 20:16:12 PDT 
Darin, Benjamin, I wonder whether you guys can take a look this patch.
Comment 11 Darin Adler 2014-09-19 08:57:42 PDT 
Comment on attachment 238346 [details]
Patch

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

I’m not really familiar with the coordinated graphics code path, but the patch seems OK to me.

> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:28
> +#include "ewk_view_private.h"

This test seems to be specific to EWK, not particularly specific to coordinated graphics. Maybe the bug was with coordinates graphics, but not the test.

> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:38
> +static bool scroll = false;

Strange name for this boolean. It’s not a “scroll”, so I assume it’s real name should be something else. But also, it’s a write-only global variable, and so we should probably just leave it out entirely.

> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:45
> +static void didChangeContentsPosition(WKViewRef, WKPoint p, const void*)

Argument name p here seems unneeded.
Comment 12 Gyuyoung Kim 2014-09-19 18:41:59 PDT 
Created attachment 238401 [details]
Patch for landing
Comment 13 Gyuyoung Kim 2014-09-19 18:42:53 PDT 
Comment on attachment 238346 [details]
Patch

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

Thanks you for review !

>> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:28
>> +#include "ewk_view_private.h"
> 
> This test seems to be specific to EWK, not particularly specific to coordinated graphics. Maybe the bug was with coordinates graphics, but not the test.

Yes, right. Test was moved to EFL dir.

>> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:38
>> +static bool scroll = false;
> 
> Strange name for this boolean. It’s not a “scroll”, so I assume it’s real name should be something else. But also, it’s a write-only global variable, and so we should probably just leave it out entirely.

Removed.

>> Tools/TestWebKitAPI/Tests/WebKit2/CoordinatedGraphics/WKViewScrollTo.cpp:45
>> +static void didChangeContentsPosition(WKViewRef, WKPoint p, const void*)
> 
> Argument name p here seems unneeded.

Removed.
Comment 14 WebKit Commit Bot 2014-09-19 19:33:52 PDT 
Comment on attachment 238401 [details]
Patch for landing

Clearing flags on attachment: 238401

Committed r173785: <http://trac.webkit.org/changeset/173785>
Comment 15 WebKit Commit Bot 2014-09-19 19:33:56 PDT 
All reviewed patches have been landed.  Closing bug.