Release anchor scroll lock when a scroll is handled by a Node

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Currently, if a scroll event is handled entirely by a Node (i.e., doesn't bubble up to the Frame), it doesn't reset FrameView::m_maintainScrollPositionAnchor to 0.

This occasionally leads to what looks like a non-functional scrollbar in Chromium when an html element has a scrollbar.  Not sure if any other ports have run into similar problems.

[Nate Chapin]

Created attachment 33845 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 33845 [details]
patch

Needs explanation of what can lead to this.  Needs explaination of why this can't be tested, or it needs tests.

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 33845 [details])
> Needs explanation of what can lead to this.  Needs explaination of why this
> can't be tested, or it needs tests.

So the reduction I've been using for my testing purposes is: www.pmarks.net/posted_links/chromium_anchor_scroll_bug.html#somewhere

I'm not sure if this can be tested by a layout test, so if I could get a second opinion, that'd be great.

Here's what happens:
* Navigation to an anchor link begins
* We set FrameView::m_maintainScrollPositionAnchor to point to the anchor node
* Every time we do a new layout(), we recalculate how scrollbars should be set so that we are displaying m_maintainScrollPositionAnchor
* The first time the user performs a scroll event that is handled by FrameView, m_maintainScrollPositionAnchor is set to 0 and we no longer force focus back to the anchor node when we do a layout.  However, if our scrolling event is being handled entirely by a Node (as is the case in the link above), none of the FrameView functions that reset m_maintainScrollPositionAnchor to 0 ever get called.
* After trying to scroll the scrollbar, force a layout (the layout happens automatically in Chromium, in Safari I just triggered it by resizing the window slightly).  The scrollbar will reset to focus on the anchor node even though the manual scroll should reset m_maintainScrollPositionAnchor to 0.

Does that explain the problem more clearly?

[Nate Chapin]

Note that I've only been able to reproduce this in Safari using my debug build with tip of tree WebKit.  Safari 4.0.2 apparently doesn't include r45710 and therefore can't trigger this issue.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 33845 [details] [details])
> > Needs explanation of what can lead to this.  Needs explaination of why this
> > can't be tested, or it needs tests.
> 
> So the reduction I've been using for my testing purposes is:
> www.pmarks.net/posted_links/chromium_anchor_scroll_bug.html#somewhere
> 
> I'm not sure if this can be tested by a layout test, so if I could get a second
> opinion, that'd be great.
> 
> Here's what happens:
> * Navigation to an anchor link begins
> * We set FrameView::m_maintainScrollPositionAnchor to point to the anchor node
> * Every time we do a new layout(), we recalculate how scrollbars should be set
> so that we are displaying m_maintainScrollPositionAnchor
> * The first time the user performs a scroll event that is handled by FrameView,
> m_maintainScrollPositionAnchor is set to 0 and we no longer force focus back to
> the anchor node when we do a layout.  However, if our scrolling event is being
> handled entirely by a Node (as is the case in the link above), none of the
> FrameView functions that reset m_maintainScrollPositionAnchor to 0 ever get
> called.
> * After trying to scroll the scrollbar, force a layout (the layout happens
> automatically in Chromium, in Safari I just triggered it by resizing the window
> slightly).  The scrollbar will reset to focus on the anchor node even though
> the manual scroll should reset m_maintainScrollPositionAnchor to 0.
> 
> Does that explain the problem more clearly?

[Eric Seidel (no email)]

It's possible to cause a window resize in DumpRenderTree (at least I think so).

It sounds like this could be complicated to test, but I'm not sure.  You'd just have to try.  You can ask folks on IRC, but I think you mostly need to come to a decision of testability and convince the reviewers to agree with whatever you decide. :)

[Nate Chapin]

Created attachment 35153 [details]
patch + layout test

[Nate Chapin]

Created attachment 38384 [details]
layout test now has dumpAsText() expected results

[Eric Seidel (no email)]

Comment on attachment 38384 [details]
layout test now has dumpAsText() expected results

The change looks sane enough.  The test needs work, as described over IM.

[Nate Chapin]

Created attachment 39392 [details]
Cleaned up layout test, clearer description and PASS/FAIL output

[Nate Chapin]

Created attachment 39614 [details]
Layout tests is less implementation dependent, and modified EventHandler so I'm no longer dispatching new events.

[Nate Chapin]

Created attachment 39616 [details]
Move some calls to setFrameWasScrolledByUser() earlier in their functions

[Eric Seidel (no email)]

Comment on attachment 39616 [details]
Move some calls to setFrameWasScrolledByUser() earlier in their functions

Seems OK.