Bug 27860 - Release anchor scroll lock when a scroll is handled by a Node
Summary: Release anchor scroll lock when a scroll is handled by a Node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 15:39 PDT by Nate Chapin
Modified: 2009-10-15 14:09 PDT (History)
1 user (show)

See Also:


Attachments
patch (2.12 KB, patch)
2009-07-30 15:40 PDT, Nate Chapin
eric: review-
Details | Formatted Diff | Diff
patch + layout test (39.36 KB, patch)
2009-08-19 15:46 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
layout test now has dumpAsText() expected results (5.56 KB, patch)
2009-08-21 12:00 PDT, Nate Chapin
eric: review-
Details | Formatted Diff | Diff
Cleaned up layout test, clearer description and PASS/FAIL output (5.96 KB, patch)
2009-09-10 16:52 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Layout tests is less implementation dependent, and modified EventHandler so I'm no longer dispatching new events. (7.33 KB, patch)
2009-09-15 14:17 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Move some calls to setFrameWasScrolledByUser() earlier in their functions (7.57 KB, patch)
2009-09-15 15:02 PDT, Nate Chapin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-07-30 15:39:30 PDT
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.
Comment 1 Nate Chapin 2009-07-30 15:40:55 PDT
Created attachment 33845 [details]
patch
Comment 2 Eric Seidel (no email) 2009-08-07 14:02:30 PDT
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.
Comment 3 Nate Chapin 2009-08-13 13:01:57 PDT
(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?
Comment 4 Nate Chapin 2009-08-13 13:12:04 PDT
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?
Comment 5 Eric Seidel (no email) 2009-08-18 13:59:48 PDT
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. :)
Comment 6 Nate Chapin 2009-08-19 15:46:54 PDT
Created attachment 35153 [details]
patch + layout test
Comment 7 Nate Chapin 2009-08-21 12:00:00 PDT
Created attachment 38384 [details]
layout test now has dumpAsText() expected results
Comment 8 Eric Seidel (no email) 2009-09-10 15:43:10 PDT
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.
Comment 9 Nate Chapin 2009-09-10 16:52:59 PDT
Created attachment 39392 [details]
Cleaned up layout test, clearer description and PASS/FAIL output
Comment 10 Nate Chapin 2009-09-15 14:17:14 PDT
Created attachment 39614 [details]
Layout tests is less implementation dependent, and modified EventHandler so I'm no longer dispatching new events.
Comment 11 Nate Chapin 2009-09-15 15:02:58 PDT
Created attachment 39616 [details]
Move some calls to setFrameWasScrolledByUser() earlier in their functions
Comment 12 Eric Seidel (no email) 2009-09-28 14:36:49 PDT
Comment on attachment 39616 [details]
Move some calls to setFrameWasScrolledByUser() earlier in their functions

Seems OK.