Closed Bug 1180382 Opened 4 years ago Closed 4 years ago

Anchor Links with hashtags fail to open or scroll page

Categories

(Firefox for Android :: General, defect)

39 Branch
Unspecified
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox39 --- wontfix
firefox40 + wontfix
firefox41 + verified
firefox42 + verified
fennec 41+ ---

People

(Reporter: pgorod, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.0; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253
Firefox for Android

Steps to reproduce:

I'm getting this bug only in Firefox Mobile (mine is version 39 on Android Kitkat on a Nexus S).

To reproduce, 

1. go to this link:
https://dl.dropboxusercontent.com/u/8232796/MissalIsilo/TCSemana12.htm#TCSem12

2. Then, a bit below, after the red text "Anos ímpares", click the link "Se"



Actual results:

The link shows some feedback of being clicked (shadow appears and disappears), and turns to the "visited" color.

But the browser doesn't do anything, the page doesn't move.

This ALWAYS fails, every single time I try.


Expected results:

It should have jumped to that link's destination, scrolling the page to the appropriate anchor.

Notes: 
1. This happens in tons of anchored links, sometimes across pages, sometimes inside the same page

2. The link works fine if I long-tap, select open in new tab, and switch to the new tab

3. The link works fine if I copy the URL and paste in the address bar

4. This is a regression, from about one or two months ago. I use this "site" (actually, reference materials) daily and it used to work well before.

5. The link works fine in other browsers, including Firefox on Windows.

6. Many other anchored links work fine; so the question is what makes some links fail?

7. A mere suggestion to investigate the answer to the question above: I suspect (but I didn't really check) it's when I'm on a page that has an anchor (like "TCSem12") and I try to navigate to an anchor that begins with that same string (like "TCSem12SeL1A1"). This is sort of common with anchors that describe chapter-subchapter strucutures.
OS: Unspecified → Android
Okay, I can confirm this on a current Nightly as well.

Nightly builds give me the following regression range: [2015-03-24, 2015-03-25], respectively https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=840cfd5bc971&tochange=db0409de517a

Bisecting Inbound (using mozregression 0.36, because newer versions use taskcluster, which has Inbound builds for Fennec starting only from June) then gives me https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c44d46087f59&tochange=7ee9c3fce092, which points to Bug 1146339.

Using the navigation buttons (back/forward, respectively the history context menu), I encounter a related problem on the example page you've linked to - when navigating backwards/forwards between different anchors on the same page, sometimes the page won't reposition itself. That problem however seems to have a different regression range, because even "good" builds are affected.

Regarding your "Note 1", can you also give an example where this fails across different pages?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pgorod)
Keywords: regression
Product: Firefox → Firefox for Android
Version: 39 Branch → Firefox 39
Looks like security bug 1146339 regressed scrolling to anchors on a page for Firefox for Android.
tracking-fennec: --- → ?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
That's pretty odd.  Someone who has an Android debug setup should step through what happens when we attempt the anchor scroll here...
Flags: needinfo?(bzbarsky)
@JanH: regarding my note 1... sorry, I was under the impression that these cases existed (cross-page links not working) but after some tests I was not able to find any. It's hard to see it because the links are long and the screen space is tiny. Perhaps I was wrong and you should disregard that comment, unless we come across any examples in the mean time.

Regarding what you mention about problems navigating backwards/forwards, yes, I also have the impression that this is buggy, and that it seems to be a different bug (started long before). You'«ll find several unfixed bugs if you search for "scroll position anchor", and I believe that there is a long-standing fundamental problem with scroll position in Firefox Core, the most advanced discussion seems to be here: https://bugzilla.mozilla.org/show_bug.cgi?id=43114 (despite having an owner, that bug is not being worked on).
Flags: needinfo?(pgorod)
Could it be that we use some session history callback or location-change callback on Android to implement hash-scrolling (or at least update the rendering)? We used to do hash-scrolling before location-change callback, but now the scrolling happens afterwards.
Flags: needinfo?(bugs)
And in particular, is this a problem for subframes on Android or only for the toplevel page?  If I recall correctly scrolling of the toplevel page on Android is handled pretty weirdly (as in, outside of Gecko)...
Tracking this user visible regression.
(In reply to Olli Pettay [:smaug] (away-ish until July 26, expect slower than usual reviews) from comment #8)
> If I read the code correctly, we end up executing 
> http://hg.mozilla.org/mozilla-central/annotate/eee2d49d055c/mobile/android/
> chrome/content/browser.js#l4686
> which leads to
> http://hg.mozilla.org/mozilla-central/annotate/eee2d49d055c/mobile/android/
> chrome/content/browser.js#l4026
> which then uses
> http://hg.mozilla.org/mozilla-central/annotate/eee2d49d055c/mobile/android/
> chrome/content/browser.js#l3985
> And I guess
> http://hg.mozilla.org/mozilla-central/annotate/eee2d49d055c/mobile/android/
> chrome/content/browser.js#l4027 then uses wrong scroll information

This flow is the relevant one, yes.

> If that is the case, could we execute
> http://hg.mozilla.org/mozilla-central/annotate/eee2d49d055c/mobile/android/
> chrome/content/browser.js#l4686 asynchronously?

In theory, yes, that should work. This code is fairly brittle though so I wouldn't be surprised if it caused some other regression.

> I don't have any Android devices to test this.

I can try it later today. Or you could push a try build and have somebody else test. Leaving ni on me for now.
Looks like anchor links are working normally again in that try build. I also have the impression that the situation regarding the forward/backward navigation buttons mentioned above seems to have improved as well.
I haven't had a chance to try it yet but the patch itself looks good to me and based on comment 11 I'm happy to r+
Flags: needinfo?(bugmail.mozilla)
I agree with JanH's testing. The try build looks good.
Attached patch patchSplinter Review
Assignee: nobody → bugs
https://hg.mozilla.org/mozilla-central/rev/c189d7442e19
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Verified as fixed on today's Nightly (20150725030209).
Device: Galaxy S3 Mini (Android 4.1.2)

How are the chances for an uplift?
Status: RESOLVED → VERIFIED
This small but user visible fix has been verified. What do you think about uplifting to beta8?
Flags: needinfo?(bugs)
I don't really know how risky the change is. Kats, any comments?
(My gut feeling is that it shouldn't be too risky.)
Flags: needinfo?(bugs) → needinfo?(bugmail.mozilla)
My gut feeling is that it *is* too risky for a late beta. Aurora uplift should be OK though. In general the scroll position code on Fennec is rather racy and finicky and I worry that this change will change the order of things resulting in subtle bugs.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8638468 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1146339
[User impact if declined]:
See the bug title
[Describe test coverage new/current, TreeHerder]: manually verified
[Risks and why]: See comment 20
[String/UUID change made/needed]: NA
Attachment #8638468 - Flags: approval-mozilla-aurora?
Comment on attachment 8638468 [details] [diff] [review]
patch

The fix has been verified on nightly and seems like a small change. Let's uplift to Aurora.
Attachment #8638468 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: ? → 41+
And verified on Aurora (20150730004009) as well.
I just installed the Beta (v 41) on my phone and it seems to be fixed. While taking the opportunity to thank everyone involved, I would like to ask about the rest of the quirks causing Firefox to lose position.

Would any of you be willing to take on the fifteen year old bug 43114, which blocks 11 other bugs, and which has become much more annoying and user-visible with the advent of all the mobiles (as per my comment 27 on that bug)?

I can easily produce more specific bugs with reproducible steps (using those pages I used for our current bug) but I'm not sure if it helps. I don't know if the best strategy is to just try to fix the whole thing once and for all, or to make small incremental improvements that address specific cases that annoy users...

I would appreciate your thoughts and advices, so I know if I can contribute in a useful manner.
Duplicate of this bug: 1166830
Duplicate of this bug: 1206555
You need to log in before you can comment on or make changes to this bug.