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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Spinningspark.wiki]

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.

Comment 1

[Spinningspark.wiki]

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

[Alice0775 White]

[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

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

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)?

Comment 4

[[e-mail]]

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.

Comment 5

[[e-mail]]

Attachment: Proposed patch

Comment 6

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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

testcase, please.
But this could work. Tiny bit ugly

Comment 7

[Bartosz Dziewoński]

(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.)

Comment 8

[[e-mail]]

(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.

Comment 9

[[e-mail]]

(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.

Comment 10

[[e-mail]]

Attachment: Test

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

Comment 11

[[e-mail]]

Hmm, will have to see why the patch causes those debug assertions...

Comment 12

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 8474254 [details] [diff] [review]
Test

So I assume I shouldn't review this yet?

Comment 13

[[e-mail]]

Attachment: Revised patch

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

Comment 14

[[e-mail]]

Comment on attachment 8474254 [details] [diff] [review]
Test

Test was fine, assertion was in an unrelated test.

Comment 15

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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

Comment 16

[[e-mail]]

(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.

Comment 17

[[e-mail]]

(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.

Comment 18

[[e-mail]]

Attachment: Fixed patch

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.)

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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 20

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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.

Comment 21

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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)

Comment 22

[[e-mail]]

(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.

Comment 23

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Why isn't it enough to set mScrollToRef to false, and not mScrolledToRefAlready to true?

Comment 24

[[e-mail]]

(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.

Comment 25

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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"

Comment 26

[[e-mail]]

Attachment: Simpler patch

OK, so rather than playing whack-a-mole, this patch seems to pass all tests.

Comment 27

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdcf3a2da55c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b35e87230b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdcf3a2da55c
https://hg.mozilla.org/mozilla-central/rev/f4b35e87230b

Comment 29

[Bartosz Dziewoński]

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.

Comment 30

[[e-mail]]

(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.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Niel, could you fill an uplift request for 33? Thanks

Comment 32

[[e-mail]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8865201cd18e
https://hg.mozilla.org/releases/mozilla-beta/rev/e47ff024eec1

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

This does not meet ESR landing criteria.