Bug 97617 – (Threaded scrolling) WebKit not scrolling to the correct location upon going back on macsurfer.com

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brady Eidson]

(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>

[Brady Eidson]

Created attachment 165703 [details]
Patch v1

[Anders Carlsson]

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.

[Darin Adler]

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?

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 165707 [details]
Patch v2

[WebKit Review Bot]

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

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 165718 [details]
Patch v3 - Now with more scroll events

[Anders Carlsson]

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!

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/129652