136917 – [CoordinatedGraphics][EFL] window.scrollTo(x, y) doesn't work when fixed layout is enabled

RESOLVED FIXED 136917

[CoordinatedGraphics][EFL] window.scrollTo(x, y) doesn't work when fixed layout is enabled

https://bugs.webkit.org/show_bug.cgi?id=136917

Summary [CoordinatedGraphics][EFL] window.scrollTo(x, y) doesn't work when fixed lay...

Gyuyoung Kim

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

Attachments
Patch (7.96 KB, patch) 2014-09-18 03:05 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (9.94 KB, patch) 2014-09-18 10:14 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (9.64 KB, patch) 2014-09-18 18:50 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch for landing (9.05 KB, patch) 2014-09-19 18:41 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Gyuyoung Kim

Comment 1 2014-09-18 03:05:51 PDT

Created attachment 238301 [details] Patch

Gyuyoung Kim

Comment 2 2014-09-18 03:06:37 PDT

CC'ing Ossy, Yoon and Benjamin. PTAL !

Grzegorz Czajkowski

Comment 3 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.

Gyuyoung Kim

Comment 4 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.

Gyuyoung Kim

Comment 5 2014-09-18 10:14:16 PDT

Created attachment 238313 [details] Patch

Grzegorz Czajkowski

Comment 6 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).

Gyuyoung Kim

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

Gyuyoung Kim

Comment 8 2014-09-18 18:50:55 PDT

Created attachment 238346 [details] Patch

Gyuyoung Kim

Comment 9 2014-09-18 18:51:34 PDT

Greg, nits you mentioned are all fixed !

Gyuyoung Kim

Comment 10 2014-09-18 20:16:12 PDT

Darin, Benjamin, I wonder whether you guys can take a look this patch.

Darin Adler

Comment 11 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.

Gyuyoung Kim

Comment 12 2014-09-19 18:41:59 PDT

Created attachment 238401 [details] Patch for landing

Gyuyoung Kim

Comment 13 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.

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 2014-09-19 19:33:56 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebKit EFL

Assignee

Gyuyoung Kim

Reported

2014-09-18 03:02 PDT

Modified

2014-10-07 01:01 PDT History

CC List

7 users Show

URL

Keywords

Depends on

Blocks

136999 137336

Dependencies

tree graph