url doesn't correctly go to anchor point when collapsible items present on page

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: Spinningspark.wiki, Assigned: neil)

Tracking

(Blocks 1 bug, {regression})

29 Branch
mozilla34
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ wontfix, firefox33+ fixed, firefox34+ fixed, firefox-esr24 unaffected, firefox-esr31 wontfix)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Follow a link with a url containing a #anchor.  Incorrect results if the page has collapsible elements above the anchor point that are set to "collapsed"

An example is at https://en.wikipedia.org/wiki/Wikipedia:NGRIDIRON#American_football.2FCanadian_football where the "Frequently Asked Questions" section is collapsed.



Actual results:

In the example page linked above, the page scrolls down to around the "Baseball" section whereas it should have scrolled to the "American football" section.

Apparently, the page position calculation is being done before the collapsible items are collapsed resulting in the position being too far down the page after the collapses are actioned.


Expected results:

The anchor is on the "American football/Canadian football" heading and that is where the page should have been scrolled to.  This works correctly in IE and Chrome.

The right anchor is found after keying F6-Enter.  Presumably, this is because the page position is now being calculated after the collapsible items have been collapsed.
(Reporter)

Comment 1

5 years ago
Correction, the problem is only apparent in the example I gave when going through Wikipedia's internal redirect.  The correct URL is https://en.wikipedia.org/wiki/Wikipedia:NGRIDIRON

Comment 2

5 years ago
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6e5d4c425fcc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140110003144
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/95f43cea056e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140110014346
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6e5d4c425fcc&tochange=95f43cea056e

Regressed by:
c64703059abd	Neil Rashbrook — Bug 952087 Anchor scroll fails if the anchor is created between DOMContentLoaded and onload r=smaug
Blocks: 952087
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Misc Code
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 31 Branch → 29 Branch
I can reproduce the issue on Nightly on OSX. I'm tracking but, given the schedule, it may not be possible to fix this in 32.


Jet - Can you please have someone take a look at this to see what will be involved in a fix and if we can get a fix in time for 32 (will have to come by next Wed)?
(Assignee)

Comment 4

5 years ago
For the URL provided here the document starts loading without a ref, but then script sets the ref while the document is loading.

Although this scrolled to the wrong place because subsequent script altered the document layout, once the document finished loading it used to directly call ScrollToAnchor which would correct the scrolling.

That doesn't work for the URL from bug 952807 because in that case the anchor wasn't found when the document's content initially loaded, so the patch there solved that case by calling ScrollToRef instead.

But in this case, because the ref was added after the document started loading, the document doesn't realise that there is a ref to scroll to.

I originally thought that I should make ScrollToRef call ScrollToAnchor if we have no ref, but looking at it I think SetDocumentURI should call SetScrollToRef.
(Assignee)

Comment 5

5 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8473898 - Flags: review?(bugs)
Comment on attachment 8473898 [details] [diff] [review]
Proposed patch

testcase, please.
But this could work. Tiny bit ugly
Attachment #8473898 - Flags: review?(bugs) → review+
Flags: needinfo?(bugs)
(I would like to point out that this is a problem at all because of bug 516293, which forces MediaWiki to update location.hash after the page is loaded.)
(Assignee)

Comment 8

5 years ago
(In reply to Bartosz Dziewoński from comment #7)
> (I would like to point out that this is a problem at all because of bug
> 516293, which forces MediaWiki to update location.hash after the page is
> loaded.)

This patch appears to fix that bug too, which means I can steal its test case.
(Assignee)

Comment 9

5 years ago
(In reply to Olli Pettay from comment #6)
> testcase, please.

This turned out to be harder than expected. The test requires that the page's location changes during the test. The reftest harness doesn't appreciate this.
(Assignee)

Comment 10

5 years ago
Posted patch TestSplinter Review
The test page is the same as the reference except the #d is set in script. Unfortunately due to limitations of the reftest harness I had to load the test inside a frame. The test fails without the patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2930c93728ee
And passes with it:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60a77286bd7e
Attachment #8474254 - Flags: review?(bugs)
(Assignee)

Comment 11

5 years ago
Hmm, will have to see why the patch causes those debug assertions...
Comment on attachment 8474254 [details] [diff] [review]
Test

So I assume I shouldn't review this yet?
Attachment #8474254 - Flags: review?(bugs)
(Assignee)

Comment 13

5 years ago
Posted patch Revised patch (obsolete) — Splinter Review
The string that was storing the ref was also doing double duty as a flag to indicate that it was safe to scroll. By getting the ref directly from the document's URI I can replace the string with a true flag. Passing try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e82939ac4ca6
Attachment #8473898 - Attachment is obsolete: true
Attachment #8474278 - Flags: review?(bugs)
(Assignee)

Comment 14

5 years ago
Comment on attachment 8474254 [details] [diff] [review]
Test

Test was fine, assertion was in an unrelated test.
Attachment #8474254 - Flags: review?(bugs)
Comment on attachment 8474278 [details] [diff] [review]
Revised patch

>-nsDocument::SetScrollToRef(nsIURI *aDocumentURI)
>-{
>-  if (!aDocumentURI) {
>-    return;
>-  }
>-
>-  nsAutoCString ref;
>-
>-  // Since all URI's that pass through here aren't URL's we can't
>-  // rely on the nsIURI implementation for providing a way for
>-  // finding the 'ref' part of the URI, we'll haveto revert to
>-  // string routines for finding the data past '#'
>-
>-  aDocumentURI->GetSpec(ref);
>-
>-  nsReadingIterator<char> start, end;
>-
>-  ref.BeginReading(start);
>-  ref.EndReading(end);
>-
>-  if (FindCharInReadable('#', start, end)) {
>-    ++start; // Skip over the '#'
>-
>-    mScrollToRef = Substring(start, end);
Am I missing something here - why can you get rid of this?


>+
>+  nsCString unescapedRef;
>+  mDocumentURI->GetRef(unescapedRef);
mDocumentURI can be null
Attachment #8474278 - Flags: review?(bugs) → review-
Attachment #8474254 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Olli Pettay from comment #15)
> (From update of attachment 8474278 [details] [diff] [review])
> >-  // Since all URI's that pass through here aren't URL's we can't
> >-  // rely on the nsIURI implementation for providing a way for
> >-  // finding the 'ref' part of the URI, we'll haveto revert to
> >-  // string routines for finding the data past '#'
> >-
> >-  aDocumentURI->GetSpec(ref);
> >-
> >-  nsReadingIterator<char> start, end;
> >-
> >-  ref.BeginReading(start);
> >-  ref.EndReading(end);
> >-
> >-  if (FindCharInReadable('#', start, end)) {
> >-    ++start; // Skip over the '#'
> >-
> >-    mScrollToRef = Substring(start, end);
> Am I missing something here - why can you get rid of this?

Because these days GetRef lives on nsIURI rather than nsIURL.

> >+  nsCString unescapedRef;
> >+  mDocumentURI->GetRef(unescapedRef);
> mDocumentURI can be null

Not while we're loading a document, surely? If you like I can throw in a null-check of mDocumentURI with the mScrollToRef check.
(Assignee)

Comment 17

5 years ago
(In reply to Olli Pettay from comment #15)
> (From update of attachment 8474278 [details] [diff] [review])
> >+  mDocumentURI->GetRef(unescapedRef);
> mDocumentURI can be null
Note that the entire test suite ran without crashing.

The bad news is that some mochitests failed, sigh.
(Assignee)

Comment 18

5 years ago
Posted patch Fixed patch (obsolete) — Splinter Review
Turns out my previous patch was causing some mochitests to fail. This resolves that by having the docshell tell the document that it's already scrolled. (ScrollToAnchor doesn't actually return rv.)
Attachment #8474278 - Attachment is obsolete: true
Attachment #8475023 - Flags: review?(bugs)
Final beta for 32 goes to build today. I think that we can ship with this issue as I don't think it is a show stopper (already shipped in 29, 30, and 31.) As such, I'm marking this as won't fix in 32. I would like to see this fix land on 33 before the next uplift, which will take place on Sep 2, 2014.
Comment on attachment 8475023 [details] [diff] [review]
Fixed patch

I don't understand the Docshell bits.
Why do we want to call SetScrolledToRefAlready there and not in
presshell::GoToAnchor.

It is also not clear why we need both mScrolledToRefAlready and mScrollToRef.

Maybe I'm just missing something, but the member variables sure need some documentation.


For branches we should probably backout Bug 952087.
Attachment #8475023 - Flags: review?(bugs) → review-
If we need both scrolltoref and scrolledtorefalready, perhaps create an
enum type member variable, which holds values
eNoRefScrollingRequested, eScrollToRef or eScrolledToRefAlready
(but it isn't still still why we need both two latter ones)
(Assignee)

Comment 22

5 years ago
(In reply to Olli Pettay from comment #20)
> It is also not clear why we need both mScrolledToRefAlready and mScrollToRef.

FYI mScrolledToRefAlready is needed because we try to scroll twice, once on DOMContentLoaded and once onload; often the DOMContentLoaded scroll will be near the correct scroll position, but it happens earlier than onload so it makes it look as if the page is loading more quickly.

mScrollToRef is needed so that we don't try to scroll to the ref before the initial reflow (GoToAnchor asserts in that case). In the current code, mScrollToRef does double duty because it's also the ref to scroll to, but this fails in the example because the ref is no longer up-to-date.
Why isn't it enough to set mScrollToRef to false, and not mScrolledToRefAlready to true?
(Assignee)

Comment 24

5 years ago
(In reply to Olli Pettay from comment #23)
> Why isn't it enough to set mScrollToRef to false, and not
> mScrolledToRefAlready to true?

Because if the user has scrolled after the first scroll to ref then we don't want the second scroll to ref to happen, so we need to track the three states a) not ready to scroll to ref b) not scrolled to ref yet c) scrolled to ref but might need to readjust if the user hasn't already scrolled.
Then please use some enum for member variable type to indicated that we have those 3 states.
Well, isn't there 4th "have scrolled and no need to readjust"
(Assignee)

Comment 26

5 years ago
Posted patch Simpler patchSplinter Review
OK, so rather than playing whack-a-mole, this patch seems to pass all tests.
Attachment #8475023 - Attachment is obsolete: true
Attachment #8480689 - Flags: review?(bugs)
Attachment #8480689 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/bdcf3a2da55c
https://hg.mozilla.org/mozilla-central/rev/f4b35e87230b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Thanks for the fix.

Neil, re comment 8 – does the final version of the patch also fix bug 516293? I see that it has been rewritten a few times.
(Assignee)

Comment 30

5 years ago
(In reply to Bartosz Dziewoński from comment #29)
> Thanks for the fix.
> 
> Neil, re comment 8 – does the final version of the patch also fix bug
> 516293? I see that it has been rewritten a few times.

Yes, the test is actually based on the test case from that bug.
Niel, could you fill an uplift request for 33? Thanks
Flags: needinfo?(neil)
(Assignee)

Comment 32

5 years ago
Comment on attachment 8480689 [details] [diff] [review]
Simpler patch

Approval Request Comment
[Feature/regressing bug #]: 952087
[User impact if declined]: Scrolling to an anchor while the page is loading won't readjust correctly once the page has loaded.
[Describe test coverage new/current, TBPL]: attachment 8474254 [details] [diff] [review]
[Risks and why]: Low risk (just avoids using a stale anchor), also happens to fix bug 516293.
[String/UUID change made/needed]: None
Attachment #8480689 - Flags: approval-mozilla-beta?
Flags: needinfo?(neil)
Attachment #8480689 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This does not meet ESR landing criteria.
Duplicate of this bug: 516293

Updated

8 months ago
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.