Closed
Bug 1054289
Opened 10 years ago
Closed 10 years ago
url doesn't correctly go to anchor point when collapsible items present on page
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: Spinningspark.wiki, Assigned: neil)
References
()
Details
(Keywords: regression)
Attachments
(2 files, 3 obsolete files)
2.25 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
811 bytes,
patch
|
smaug
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 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•10 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
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox-esr24:
--- → unaffected
status-firefox-esr31:
--- → affected
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
tracking-firefox34:
--- → ?
tracking-firefox-esr31:
--- → ?
Component: Untriaged → Layout: Misc Code
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 31 Branch → 29 Branch
![]() |
||
Updated•10 years ago
|
Comment 3•10 years ago
|
||
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)?
Flags: needinfo?(bugs)
Assignee | ||
Comment 4•10 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•10 years ago
|
||
Comment 6•10 years ago
|
||
Comment on attachment 8473898 [details] [diff] [review] Proposed patch testcase, please. But this could work. Tiny bit ugly
Attachment #8473898 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Flags: needinfo?(bugs)
Comment 7•10 years ago
|
||
(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•10 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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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-
Updated•10 years ago
|
Attachment #8474254 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 16•10 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•10 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•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8480689 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdcf3a2da55c https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b35e87230b
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bdcf3a2da55c https://hg.mozilla.org/mozilla-central/rev/f4b35e87230b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 29•10 years ago
|
||
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•10 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.
Comment 31•10 years ago
|
||
Niel, could you fill an uplift request for 33? Thanks
Flags: needinfo?(neil)
Assignee | ||
Comment 32•10 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)
Updated•10 years ago
|
Attachment #8480689 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 33•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/8865201cd18e https://hg.mozilla.org/releases/mozilla-beta/rev/e47ff024eec1
Comment 34•10 years ago
|
||
This does not meet ESR landing criteria.
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•