1180382 - Anchor Links with hashtags fail to open or scroll page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Pgr]

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.

Comment 1

[Jan Henning [:JanH]]

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?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Looks like security bug 1146339 regressed scrolling to anchors on a page for Firefox for Android.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's pretty odd.  Someone who has an Android debug setup should step through what happens when we attempt the anchor scroll here...

Comment 4

[Pgr]

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

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

Tracking this user visible regression.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

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

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Something like 
https://hg.mozilla.org/try/rev/c338d37dea9e

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c338d37dea9e

Comment 11

[Jan Henning [:JanH]]

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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+

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

I agree with JanH's testing. The try build looks good.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c189d7442e19

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c189d7442e19

Comment 17

[Jan Henning [:JanH]]

Verified as fixed on today's Nightly (20150725030209).
Device: Galaxy S3 Mini (Android 4.1.2)

How are the chances for an uplift?

Comment 18

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

This small but user visible fix has been verified. What do you think about uplifting to beta8?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I don't really know how risky the change is. Kats, any comments?
(My gut feeling is that it shouldn't be too risky.)

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b0693dc0e29

Comment 24

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

Marking 40 as wontfix based on comment 20.

Comment 25

[Jan Henning [:JanH]]

And verified on Aurora (20150730004009) as well.

Comment 26

[Pgr]

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.