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.
Created attachment 238301 [details] Patch
CC'ing Ossy, Yoon and Benjamin. PTAL !
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 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.
Created attachment 238313 [details] Patch
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).
(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.
Created attachment 238346 [details] Patch
Greg, nits you mentioned are all fixed !
Darin, Benjamin, I wonder whether you guys can take a look this patch.
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.
Created attachment 238401 [details] Patch for landing
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 on attachment 238401 [details] Patch for landing Clearing flags on attachment: 238401 Committed r173785: <http://trac.webkit.org/changeset/173785>
All reviewed patches have been landed. Closing bug.