58661 - Anchor scrolling does not happen until page has loaded

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[andrew]

When linking to a large page with many anchors the page will take up to 30 - 40
seconds to go the correct anchour.  Accessing the file via http or on the local
file system shows no difference in speed.  On tests with the above documents on
my local file system IE is 5-6 times faster (~5 secs on IE 5.5 vs ~30 secs on
Mozilla).

Comment 1

[Doron Rosenberg (IBM)]

layout, seeing on new builds, probably a dupe though.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think this is because we don't actually scroll to the anchor location until
the whole page has loaded. We won't be fixing this for RTM, but we should
revisit this soon. One solution suggested by jst was to have a timer that runs
maybe one second intervals when we are loading a page and see if we could scroll
already.

Reassigning to Adam Lock for now.

Comment 3

[Adam Lock]

Yes, the scrolling only happens once the page has entirely loaded. It would be
possible to scroll in the next reflow after the referenced element appeared, but
such an approach would not be infallible. For example, if the element was below
a large image which hadn't loaded yet, or in the middle of some deeply nested
tables then the scrolling would have to be updated as the element was moved around.

Also we would have to be careful with this approach that we didn't annoy the
user by continuously scrolling back to the anchor when they were trying to
scroll the page themselves.

Comment 4

[Adam Lock]

Changing summary, platforms

Comment 5

[mozbug1]

What do you mean until after the page is loaded?  I can't get them to work at
all!  Specifically when I click on a link that is supposed to take me to an
anchor on the same page I don't go to the anchor.  I'm running M18 on Linux.
I've seen this problem since M16 and I can't believe it hasn't been identified
as a bug yet.

Comment 6

[Adam Lock]

jks, does this problem occur on a specific page or any page with anchors? I took 
the opportunity when I was last at the code to fix a number of non-compliances 
with the HTML spec that may cause a few bad pages to break now. Otherwise, 
anchor scrolling seems to work as designed (except for waiting for the document 
complete to occur).

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nominating for nsbeta1 and nsdogfood. Even with the fast connections inside NS I
find this a real pain in the butt, especially when trying to look something up
with LXR (LXR pages are large, and it takes *forever* for the page load to
finish and scrolling to happen).

Comment 8

[Adam Lock]

Heikki, can you suggest a strategy for implementing this?

I looked at putting something into the HTML content sink so that when the named 
element appeared it could do a flush, reflow and scroll to it but it seemed 
nasty for the sink to have scrolling code, especially when there is stuff in the 
presshell for scrolling to anchors already.

Comment 9

[Adam Lock]

Changing target milestone.

Comment 10

[Adam Lock]

Heikki, would you be interested in taking this bug?

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am swamped for 0.9.1 :(

Nisheeth, interested?

Comment 12

[Judson Valeski]

kin, any thoughts here?

Comment 13

[Nisheeth Ranjan]

I'm swamped for 0.9.1, too.  Sorry!

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

If you move a nsbeta1 nominated bug beyond 0.9.1, you HAVE to do something about
the nomination.

Comment 15

[Janos Kramar]

I have had the same problem as jks. It has happened on all anchoring-pages I've visited so far; for example, <a href="http://www.w3.org/TR/REC-html40">http://www.w3.org/TR/REC-html40</a> and <a href="http://www.debian.org/doc/FAQ/">http://www.debian.org/doc/FAQ/</a>. I am using M18.

Comment 16

[Judson Valeski]

-> 0.9.5 sorry, we can't be dealing w/ existing regressions at the moment.

Comment 17

[Adam Lock]

Attachment: Patch to make scrolling happen sooner

Comment 18

[Adam Lock]

I will just describe the change so everyone's clear what it does:

1. I have added code to HTMLContentSink::Notify that attempts to scroll to the
named element the first time it is there to be scrolled to.
2. I have left the old anchor scrolling behaviour intact, so in addition to
scrolling when the element is first found it also happens after page loading
completes.
3. I have fixed some bustage in PresShell::GoToAnchor to stop it always
returning a failure code because some pref called "layout.selectanchor" cannot
be found.

The net result of the change is that scrolling happens a lot sooner than it used
to. Scrolling happens twice which might be slightly annoying if you're scrolling
around in the meantime but it catches situations where the named element has
shifted because of further page layout. Should I take out the second call? Is
there a non-convoluted way to test if the user has scrolled the page in the
meantime?

Reviews and comments please

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

layout.selectanchor pref is my stuff, if that pref is set and you follow an
anchor the link target will be selected. This is pretty much a must if you want
to know where the link pointed to once I check in FIXptr which can select
arbitrary ranges. But it probably makes sense to do the change you did.

I would hate to see the page jump to the anchor a second time if I had started
scrolling, but I think this would not be too common. And scrolling the first
time as soon as we find the anchor is a GREAT improvement, so I could live with
the second scroll to ref for a while.

Best thing, of course, would be to detect if the user scrolled after the first
ScrollToRef() and not jump in that case, but we would also need to detect if the
content has shifted. I don't know how to detect the scroll, but I believe you
can see if the content shifted by looking at the frame and view coordinates
(that could have been affected by scroll, of course).

Finally, a comment about the patch: it seems like the content sink would have
several boolean member variables, please move them next to each other and make
them PRPackedBool to save space.

Comment 20

[Adam Lock]

I changed the "layout.selectanchor" pref stuff because that pref doesn't exist
at all so GetBoolPref is returning an error that makes it look like the whole
scroll operation has failed.

I will submit an updated patch that bunches all the bools in HTMLContentSink
together and turns them into PRPackedBools. I don't see any easy way to current
scroll position from the presshell's associated with the document so I recommend
we go with the two refresh thing for the moment.

Comment 21

[Adam Lock]

Attachment: Second version with packed bools

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 52064 [details] [diff] [review]
Second version with packed bools

r=heikki

I am ok with marking this bug as fixed with this, but
I would like you to open a new bug to fix ScrollToRef() 
after user scrolled...

Comment 23

[Judson Valeski]

sounds good to me too. this will be a big improvement, but the double scroll
seems nasty.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 52064 [details] [diff] [review]
Second version with packed bools

sr=jst

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 52064 [details] [diff] [review]
Second version with packed bools

a=asa (on behalf of drivers) for checkin to 0.9.5.

Comment 26

[Adam Lock]

Fix is in. Bug 103279 opened up to deal with the second scroll op issue.

Comment 27

[Boris Zbarsky [:bzbarsky]]

heikki, may I make a suggestion?  Add the "layout.selectanchor" to all.js with a
default value and a comment explaining what it does.  This will:

1)  Make that call not return an error value all the time (moot now)
2)  Make it possible for people to actually figure out what the deal with that
    pref is without asking you.

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Boris: will do, but it isn't really that important until FIXptr (or something
similar) gets checked in (FIXptr maybe next week).