952087 - Anchor scroll fails if the anchor gets created between DOMContentLoaded and onload

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[[e-mail]]

The MSDN blog site loads the comments after DOMContentLoaded (this ends up delaying the load event of course). In other browsers anchor scrolls are done around onload which does require waiting for the comments to load but will at least scroll to the correct spot first time. In Gecko the anchor scroll is attempted immediately after DOMContentLoaded as a usability optimisation however in this case it means that it fails to find the anchor. If the initial anchor scroll fails then Gecko should attempt a full anchor scroll around onload instead of just the corrective anchor scroll it currently does.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8350030 [details] [diff] [review]
Proposed patch

This needs some tests
>+++ b/layout/base/nsDocumentViewer.cpp
>@@ -949,16 +949,17 @@ nsDocumentViewer::LoadComplete(nsresult 
>   if (mPresShell && !mStopped) {
>     // Hold strong ref because this could conceivably run script
>     nsCOMPtr<nsIPresShell> shell = mPresShell;
>     shell->FlushPendingNotifications(Flush_Layout);
>   }
> 
>   nsresult rv = NS_OK;
>   NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_AVAILABLE);
>+  mDocument->ScrollToRef();
and isn't this a bit early. Could we scroll after load event? What do other browsers do if new elements 
are added to dom during load event dispatching?

Comment 3

[[e-mail]]

(In reply to Olli Pettay from comment #2)
> This needs some tests
Could you be a bit less vague? Reftests? Some other sort of tests?

> >+  mDocument->ScrollToRef();
> and isn't this a bit early. Could we scroll after load event? What do other
> browsers do if new elements are added to dom during load event dispatching?
Sorry, that was just a convenient place to put it; I could put it nearer where the old code had it (after page show) with a null check of course.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to [e-mail] from comment #3)
> (In reply to Olli Pettay from comment #2)
> > This needs some tests
> Could you be a bit less vague? Reftests? Some other sort of tests?
I was thinking mochitests, where you open a new window with some anchor in the url and check
how the page is scrolled.



> > >+  mDocument->ScrollToRef();
> > and isn't this a bit early. Could we scroll after load event? What do other
> > browsers do if new elements are added to dom during load event dispatching?
> Sorry, that was just a convenient place to put it; I could put it nearer
> where the old code had it (after page show) with a null check of course.
Yeah, that sounds better.

Comment 5

[[e-mail]]

Attachment: Addressed review comments

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8355879 [details] [diff] [review]
Addressed review comments

> nsDocument::ScrollToRef()
> {
>   if (mScrolledToRefAlready) {
>+    nsCOMPtr<nsIPresShell> shell = GetShell();
>+    if (shell)
>+      shell->ScrollToAnchor();
{} with if


>@@ -1020,24 +1020,26 @@ nsDocumentViewer::LoadComplete(nsresult 
>       bool isInUnload;
>       if (docShell && NS_SUCCEEDED(docShell->GetIsInUnload(&isInUnload)) &&
>           !isInUnload) {
>         mDocument->OnPageShow(restoring, nullptr);
>       }
>     }
>   }
> 
>+  if (mDocument)
>+    mDocument->ScrollToRef();
ditto, and I think it should be
if (mDocument && !mStopped), so perhaps combine that with the other if()


if (!mStopped) {
  if (mDocument) {
    mDocument->ScrollToRef();
  }
  if (mPresShell) {
    nsCOMPtr<nsIPresShell> shellDeathGrip(mPresShell);
    mPresShell->UnsuppressPainting();
    // mPresShell could have been removed now, see bug 378682/421432
    if (mPresShell) {
      mPresShell->LoadComplete();
    }
  }
}

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4dabccb06b8

https://tbpl.mozilla.org/php/getParsedLog.php?id=32652141&tree=Mozilla-Inbound

Comment 8

[Ryan VanderMeulen [:RyanVM]]

B2G as well.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32657529&tree=Mozilla-Inbound

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1b5408d946f

Comment 10

[Phil Ringnalda (:philor)]

Cherrypicked the backout in https://hg.mozilla.org/mozilla-central/rev/cf2d1bd796ea, since yeah, we did merge permaorange to m-c.

Very intermittent in this case, but, OS X 10.6 too, https://tbpl.mozilla.org/php/getParsedLog.php?id=32674762&tree=Mozilla-Central

Comment 11

[[e-mail]]

Attachment: Addressed review comments

Test changes because XHR only blocks onload when fetching remote XML with progress. The fact that the original test usually worked on desktop was just a fluke, partly because XHR tries to load .html files as XML.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8358086 [details] [diff] [review]
Addressed review comments

Please push to try

Comment 13

[[e-mail]]

Comment on attachment 8358086 [details] [diff] [review]
Addressed review comments

Hmm, I guess that should be <!DOCTYPE html>, I suppose expat doesn't really care.

(In reply to Olli Pettay from comment #12)
> Please push to try

I pushed a slightly different version to try, does that suffice?
http://hg.mozilla.org/try/rev/2bce16e50787

(Sorry about the junk in my tree.)

Comment 14

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c64703059abd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c64703059abd