317189 - Scrolling to anchor after object tags doesn't scroll to the correct position anymore

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
When clicking on the button in the testcase, you should scroll to the correct anchor position, but that doesn't happen anymore in current trunk builds.

This regressed between 2005-09-21 and 2005-09-22, so my guess, a regression from fixing bug 1156.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

You have to download and test the testcase locally, or visit the testcase with this url:
https://bugzilla.mozilla.org/attachment.cgi?id=203719&action=view#anchor
You should directly scroll to the anchor with that url.

Comment 3

[Jay Moran (jaym/lazvon)]

This seems to be broken again from my checkout and build at: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011109 Firefox/3.0b3pre ... I first noticed this with "New Message" arrow links in vBulletin, but the https://bugzilla.mozilla.org/attachment.cgi?id=203719&action=view#anchor does it as well for me. Clicking that link in Safari takes me right to the Anchor Line text.

Comment 4

[Jay Moran (jaym/lazvon)]

Just verified the link works fine in Firefox 2.0.0.12 (as well as Safari 3) which it does. In FFx3 though, it is busted. :( There are plenty of websites, forums, blogs out there that this impacts the rendering of.

Comment 5

[Jay Moran (jaym/lazvon)]

I'm not sure this helps narrow things down at all... but after you click the above link and end up in the middle of the yellow box, instead of the #anchor where you should be... if you click in the location bar and press enter, it snaps right to the #anchor then.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

David, how do you feel about this?

The basic idea is to move the final "scroll to anchor" from the end of document loading (where the content sinks currently call ScrollToRef unconditionally) to when onload fires, at which time all layout-affecting subresources have loaded, thus fixing this bug. I do this by ensuring that GoToAnchor is not called again after it finds the anchor element. The final "scroll to anchor" is performed by the presshell, which remembers the anchor content for that purpose. The presshell refuses to scroll if the scroll position is not the same as when we first scrolled to the anchor, largely fixing the problem of users scrolling around and then being bounced back to the anchor.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you tested how it interacts with session history restoration of scroll position?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In my tests session history restoration always wins. But I guess there is a scenario where it wouldn't:

Here's the order of events:
-- during load, GoToAnchor() succeeds and scrolls to the anchor, say at position y1
-- PresShell::EndLoad calls RestoreRootScrollPosition, which scrolls to the restored position, say y2
-- ScrollToAnchor is fired by LoadComplete, which scrolls to a new anchor position y3, but only if y2==y1

So the only way we can end up not at y2 is if y2==y1, i.e. the first scroll to the anchor puts us exactly at the restored scroll position, and then some late-loading subresource or DOM change moves the anchor to another location.

I guess I should fix this...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, after much effort trying to create a testcase to show that problem, I failed. The reason is this code:
http://mxr.mozilla.org/seamonkey/source/content/base/src/nsContentSink.cpp#207
means anchor scrolling is completely disabled for loads from history! So there really is no interaction between session history restoration and anchor scrolling. So the original patch should be just fine, maybe with an extra comment to explain why there is no such interaction...

Comment 10

[Jay Moran (jaym/lazvon)]

Fix applied. Works for the test case. Thanks! I guess that is all that can be asked for since I can't produce another test case that doesn't work.

The fix doesn't work for older versions of vBulletin forums; even though Safari and Firefox 2.0.0.12 work just fine with the older versions of vBulletin. It does work for new version of vBulletin though. So, perhaps it can be dismissed as problems with their style sheet... even if older Firefox works.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304927 [details] [diff] [review]
fix

I'm hoping that the nsXMLContentSink::OnTransformDone call to
ScrollToRef can't happen when we've already scrolled to a ref in a
different content tree for the same content sink, although I'm not sure
if that's the case.  You may want to ask sicking or peterv.

Other than that, though, this looks fine, and I wouldn't wait even on
that.  (Probably the best fix if that is a problem would be to clear
mScrolledToRefAlready.)

r+sr=dbaron

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 13

[Jay Moran (jaym/lazvon)]

Works great for the test case. Still doesn't work as well as FF2 and Safari though for other websites. I'll shut up about those until I can figure out a testcase though. :)

Comment 14

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050810 Minefield/3.0pre

Verified.