179735 2017-11-15 10:30:10 -0800 window.scrollTo is initially unclamped on iOS 2020-10-30 14:39:47 -0700 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=184297 InRadar P2 Normal --- 1 ajuma webkit-unassigned cdumez danyao fred.wang rbuis simon.fraser thorton webkit-bug-importer oldest_to_newest 1372011 0 ajuma 2017-11-15 10:30:10 -0800 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). 1372023 1 simon.fraser 2017-11-15 10:48:45 -0800 Can you attach a testcase that shows the incorrect behavior? 1372057 2 327004 ajuma 2017-11-15 11:48:09 -0800 Created attachment 327004 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. 1398980 3 fred.wang 2018-02-13 23:07:26 -0800 (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? 1399045 4 ajuma 2018-02-14 06:28:53 -0800 (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. 1703213 5 webkit-bug-importer 2020-10-30 14:39:47 -0700 <rdar://problem/70897654> 327004 2017-11-15 11:48:09 -0800 2017-11-15 11:48:09 -0800 Test case test-scroll-to.html text/html 337 ajuma PCFET0NUWVBFIGh0bWw+CjxzY3JpcHQ+CiAgICB3aW5kb3cub25sb2FkID0gZnVuY3Rpb24oKSB7 CiAgICAgICAgd2luZG93LnNjcm9sbFRvKC0xMDAsIDk5OTk5OSk7CiAgICAgICAgZG9jdW1lbnQu Z2V0RWxlbWVudEJ5SWQoIm91dHB1dCIpLmlubmVySFRNTCA9CiAgICAgICAgICAgICJ3aW5kb3cu c2Nyb2xsWDogIiArIHdpbmRvdy5zY3JvbGxYICsKICAgICAgICAgICAgIiwgd2luZG93LnNjcm9s bFk6ICIgKyB3aW5kb3cuc2Nyb2xsWTsKICAgIH0KPC9zY3JpcHQ+CkFmdGVyIGNhbGxpbmcgd2lu ZG93LnNjcm9sbFRvKC0xMDAsIDk5OTk5OSksCjxkaXYgaWQ9Im91dHB1dCI+PC9kaXY+Cg==