Closed Bug 952087 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: neil, Assigned: neil)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 2 obsolete files)

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.
Severity: normal → minor
Priority: -- → P4
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #8350030 - Flags: review?(bugs)
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?
Attachment #8350030 - Flags: review?(bugs) → review-
(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.
(In reply to neil@parkwaycc.co.uk 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.
Attached patch Addressed review comments (obsolete) — Splinter Review
Assignee: nobody → neil
Attachment #8350030 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8355879 - Flags: review?(bugs)
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();
    }
  }
}
Attachment #8355879 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d1b5408d946f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8355879 - Attachment is obsolete: true
Attachment #8358086 - Flags: review?(bugs)
Comment on attachment 8358086 [details] [diff] [review]
Addressed review comments

Please push to try
Attachment #8358086 - Flags: review?(bugs) → review+
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.)
https://hg.mozilla.org/mozilla-central/rev/c64703059abd
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 1054289
Depends on: 1287514
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.