Bug 179735

Summary: window.scrollTo is initially unclamped on iOS
Product: WebKit Reporter: Ali Juma <ajuma>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: cdumez, danyao, fred.wang, rbuis, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184297
Attachments:
Description Flags
Test case none

Ali Juma
Reported 2017-11-15 10:30:10 PST
The inputs to window.scrollTo aren't clamped to the min/max scroll positions by WebCore when ScrollView::delegatesScrolling is true, so window.scrollX and window.scrollY return possibly out-of-bounds values until WebCore receives adjusted values from the UI process. For example: window.scrollTo(0, 1000000); var scrollPos = window.scrollY; // scrollPos is 1000000 even if the page is much smaller than that window.scrollTo(-100, 0); scrollPos = window.scrollX; // scrollPos is -100 even though that's not a valid scroll offset This behavior comes from the patch for bug 132879, which stopped clamping in ScrollView::setScrollPosition with the justification that the Web process shouldn't be clamping since it might have stale information. I wonder if a better approach might be to continue sending an unclamped scroll offset to the UI process, but in the meanwhile use a clamped scroll offset to update FrameView's scroll position. That would mean changing AsyncScrollingCoordinator::requestScrollPositionUpdate to take both a clamped and an unclamped offset, so that it can use the clamped version for the call to updateScrollPositionAfterAsyncScroll (which is what updates FrameView's scroll offset).
Attachments
Test case (337 bytes, text/html)
2017-11-15 11:48 PST, Ali Juma 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2017-11-15 10:48:45 PST
Can you attach a testcase that shows the incorrect behavior?
Ali Juma
Comment 2 2017-11-15 11:48:09 PST
Created attachment 327004 [details] Test case Here's a test page that tries to scroll to an out-of-bounds offset and then outputs the values of scrollX and scrollY.
Frédéric Wang (:fredw)
Comment 3 2018-02-13 23:07:26 PST
(In reply to Ali Juma from comment #0) > I wonder if a better approach might be to continue sending an unclamped > scroll offset to the UI process, but in the meanwhile use a clamped scroll > offset to update FrameView's scroll position. That would mean changing > AsyncScrollingCoordinator::requestScrollPositionUpdate to take both a > clamped and an unclamped offset, so that it can use the clamped version for > the call to updateScrollPositionAfterAsyncScroll (which is what updates > FrameView's scroll offset). @Ali: How about just putting back the clamping for programmatic scroll? i.e. - ScrollPosition newScrollPosition = !delegatesScrolling() ? adjustScrollPositionWithinRange(scrollPosition) : scrollPosition; + ScrollPosition newScrollPosition = (!delegatesScrolling() || inProgrammaticScroll()) ? adjustScrollPositionWithinRange(scrollPosition) : scrollPosition; What would be the problem if programmatically-set position overrides the one in the UI process?
Ali Juma
Comment 4 2018-02-14 06:28:53 PST
(In reply to Frédéric Wang (:fredw) from comment #3) > (In reply to Ali Juma from comment #0) > > I wonder if a better approach might be to continue sending an unclamped > > scroll offset to the UI process, but in the meanwhile use a clamped scroll > > offset to update FrameView's scroll position. That would mean changing > > AsyncScrollingCoordinator::requestScrollPositionUpdate to take both a > > clamped and an unclamped offset, so that it can use the clamped version for > > the call to updateScrollPositionAfterAsyncScroll (which is what updates > > FrameView's scroll offset). > > @Ali: How about just putting back the clamping for programmatic scroll? i.e. > > - ScrollPosition newScrollPosition = !delegatesScrolling() ? > adjustScrollPositionWithinRange(scrollPosition) : scrollPosition; > + ScrollPosition newScrollPosition = (!delegatesScrolling() || > inProgrammaticScroll()) ? adjustScrollPositionWithinRange(scrollPosition) : > scrollPosition; > > What would be the problem if programmatically-set position overrides the one > in the UI process? The problem would be that the WebProcess may not have up-to-date information about the size of the view, so we might clamp incorrectly. For example, the view might have shrunk, increasing the maximum scroll offset. I think you're right that for JS-driven scroll, we're fine to clamp since JS can't make assumptions about when resize happens in the UI, but I'm not sure that extends to all programmatic scrolls, since inProgrammticScroll() seems to be broader than just JS-driven scroll. The use case in the bug that removed the clamping (bug 132879) sounds like a programmatic scroll.
Radar WebKit Bug Importer
Comment 5 2020-10-30 14:39:47 PDT
<rdar://problem/70897654>
Note You need to log in before you can comment on or make changes to this bug.