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).
Can you attach a testcase that shows the incorrect behavior?
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.
(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?
(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.
<rdar://problem/70897654>