(Threaded scrolling) WebKit not scrolling to the correct location upon going back on macsurfer.com In Tiled Drawing (and therefore Threaded Scrolling) mode, *all* scroll position changes are treated as user generated scrolls and generate scroll events. This breaks restoring scroll position when going back to sites that are slow to layout to their full length. In radar as <rdar://problem/12039913>
Created attachment 165703 [details] Patch v1
Comment on attachment 165703 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=165703&action=review > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:171 > +void ScrollingTreeState::setIsProgrammaticScroll(bool isProgrammaticScroll) > +{ I think this should just be an extra parameter to setRequestedScrollPosition to make it harder to call setRequestedScrollPosition without thinking about whether it's a programmatic scroll or not. If you do this you don't need the extra IsProgrammaticScroll flag.
Comment on attachment 165703 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=165703&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:58 > + , m_updatingForProgrammaticScroll(false) I can’t find any code that’s reading this data member. Is this just a leftover from an earlier cut at the patch? > Source/WebCore/page/scrolling/ScrollingCoordinator.h:176 > + bool m_updatingForProgrammaticScroll; As I mentioned above, I think this should be removed. > Source/WebCore/page/scrolling/ScrollingTree.h:124 > + bool m_handlingProgrammaticScroll; I know the other booleans aren’t doing it, but I thought we preferred names that finish the sentence “tree <xxx>”, so it would be “is handling programmatic scroll” rather than “handling programmatic scroll”. > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:46 > + , m_isProgrammaticScroll(false) Is the correct phrase “tree state is programmatic scroll”? Maybe “tree state represents programmatic scroll”? What is the phrase form of what this means here?
(In reply to comment #2) > (From update of attachment 165703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165703&action=review > > > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:171 > > +void ScrollingTreeState::setIsProgrammaticScroll(bool isProgrammaticScroll) > > +{ > > I think this should just be an extra parameter to setRequestedScrollPosition to make it harder to call setRequestedScrollPosition without thinking about whether it's a programmatic scroll or not. > > If you do this you don't need the extra IsProgrammaticScroll flag. Okay, this makes sense to me. Will do.
(In reply to comment #3) > (From update of attachment 165703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165703&action=review > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:58 > > + , m_updatingForProgrammaticScroll(false) > > I can’t find any code that’s reading this data member. Is this just a leftover from an earlier cut at the patch? > > > Source/WebCore/page/scrolling/ScrollingCoordinator.h:176 > > + bool m_updatingForProgrammaticScroll; > > As I mentioned above, I think this should be removed. You're totally right. Earlier cut at the patch, and now removed. > > > Source/WebCore/page/scrolling/ScrollingTree.h:124 > > + bool m_handlingProgrammaticScroll; > > I know the other booleans aren’t doing it, but I thought we preferred names that finish the sentence “tree <xxx>”, so it would be “is handling programmatic scroll” rather than “handling programmatic scroll”. > > > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:46 > > + , m_isProgrammaticScroll(false) > > Is the correct phrase “tree state is programmatic scroll”? Maybe “tree state represents programmatic scroll”? What is the phrase form of what this means here? I'll sort these names out.
Created attachment 165707 [details] Patch v2
Comment on attachment 165707 [details] Patch v2 Attachment 165707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14042002 New failing tests: scrollbars/scrollevent-iframe-no-scrolling.html fast/events/fire-scroll-event.html fast/events/scroll-event-during-modal-dialog.html fast/events/touch/scroll-without-mouse-lacks-mousemove-events.html scrollbars/scrollable-iframe-remove-crash.html fast/events/scroll-event-phase.html
(In reply to comment #7) > (From update of attachment 165707 [details]) > Attachment 165707 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14042002 > > New failing tests: > scrollbars/scrollevent-iframe-no-scrolling.html > fast/events/fire-scroll-event.html > fast/events/scroll-event-during-modal-dialog.html > fast/events/touch/scroll-without-mouse-lacks-mousemove-events.html > scrollbars/scrollable-iframe-remove-crash.html > fast/events/scroll-event-phase.html Seeing *some* of these on Mac, certainly not all. Looking in to them.
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 165707 [details] [details]) > > Attachment 165707 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/14042002 > > > > New failing tests: > > scrollbars/scrollevent-iframe-no-scrolling.html > > fast/events/fire-scroll-event.html > > fast/events/scroll-event-during-modal-dialog.html > > fast/events/touch/scroll-without-mouse-lacks-mousemove-events.html > > scrollbars/scrollable-iframe-remove-crash.html > > fast/events/scroll-event-phase.html > > Seeing *some* of these on Mac, certainly not all. Looking in to them. Complete boneheaded mistake on my part - Turns out that window.scrollTo *is* supposed to generate scroll events. Perhaps we really should be generating scroll events for the "restore scroll position on back" also - if the page needs them to stay in sync, then it needs them. But window.scrollTo and the position restoration should still not be considered user scrolls. This is an easy modification to the current patch.
Created attachment 165718 [details] Patch v3 - Now with more scroll events
Comment on attachment 165718 [details] Patch v3 - Now with more scroll events View in context: https://bugs.webkit.org/attachment.cgi?id=165718&action=review > Source/WebCore/page/scrolling/ScrollingTreeState.h:139 > + bool m_representsProgrammaticScroll; How about m_scrollPositionRequestRepresentsProgrammaticScroll or m_requestedScrollPositionRepresentsProgrammaticScroll to better indicate that it goes with setRequestedScrollPosition? Looks fine otherwise!
Landed in http://trac.webkit.org/changeset/129652