Summary: | window.scrollTo is initially unclamped on iOS | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ali Juma <ajuma> | ||||
Component: | DOM | Assignee: | 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
Ali Juma
2017-11-15 10:30:10 PST
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. |