Unable to show the dynamic address bar on short Twitter pages

VERIFIED FIXED in Firefox 23

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: kbrosnan, Assigned: cwiiis)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox23 verified, firefox24 verified, firefox25 verified, fennec23+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
http://www.youtube.com/watch?v=Ie35sF4Op20

On a Nexus 4 I get into this state somewhat regularly. I don't have solid STR other than to use Twitter often.
(Reporter)

Updated

5 years ago
tracking-fennec: --- → ?
Chris, any ideas here?
Assignee: nobody → chrislord.net
tracking-fennec: ? → 23+

Updated

5 years ago
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 2

5 years ago
So I guess there's some possibly situation where the page changes size or similar and the notification callback doesn't get triggered...

I don't know when this happens, but I think I can probably write some kind of work-around that makes sure the toolbar can always be made visible regardless.
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 3

5 years ago
Created attachment 775667 [details] [diff] [review]
Simple possible fix

Ugh, so I think this is just a silly comparison error... At least it's a small patch :) I have a patch for bug 892246 that may also help in certain situations of this.
Attachment #775667 - Flags: review?(bugmail.mozilla)
Attachment #775667 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

5 years ago
Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbar sometimes doesn't show when it should
User impact if declined: Can end up stuck in a situation where it's impossible to reveal the dynamic toolbar
Testing completed (on m-c, etc.): Tested locally
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None
Attachment #775667 - Flags: approval-mozilla-beta?
Attachment #775667 - Flags: approval-mozilla-aurora?
1. i can reproduce the problem on my nexus 4 on FF23 beta on semi random tweets like this one:
https://twitter.com/_Reinette/status/356913948861267969

2. The apk at 
http://chrislord.net/files/mozilla/fix-inacccessible-toolbar.apk doesn't work on my Nexus 4 nor does it work on :kbrosnan's Nexus4

:cwiis Please compile a new APK that will actually work on a Nexus 7
(Assignee)

Comment 7

5 years ago
Pushed to m-c:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e26030f774d
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
Don't know why my apk's aren't working, maybe just a failed upload - You'll be able to get apks from the build machine here once the build finishes (click on the 'B' next to 'Android 2.2 opt' and the link will be at the bottom-left):

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e7743ae9df87
> Here you go:
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-
> inbound-android/1373964790/fennec-25.0a1.en-US.android-arm.apk

I can't reproduce this problem in the above nightly APK. However the problem is extremely hard (it seems random) to reproduce in 23 so this doesn't mean the problem is fixed (my guess is that Chris Lord's patch helps and should be approved). I guess the next step is that assuming this patch passes review and
 :kbrosnan and :aaronmt don't find any bugs, to uploift to beta.
https://hg.mozilla.org/mozilla-central/rev/9e26030f774d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

:Kbrosnan can you please help with some adhoc testing here to help with verification on nightly or aurora given you were able to reproduce this ?
Attachment #775667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
QA Contact: kbrosnan
Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

Risk is low enough we can take to Beta as well this week.
Attachment #775667 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b487258a6450
https://hg.mozilla.org/releases/mozilla-beta/rev/5fdab6872b49
status-firefox23: --- → fixed
status-firefox24: --- → fixed
status-firefox25: --- → fixed
(Reporter)

Comment 15

5 years ago
Still reproduced this on the July 17th nightly - http://www.youtube.com/watch?v=LjxHMbkIIvo
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

5 years ago
Created attachment 778408 [details] [diff] [review]
Another simple possible fix

Ugh, so this is something we should commit regardless of whether it fixes the situation (but I hope it does) - we were locking the toolbar after starting the show animation instead of before. As the monitor isn't held over both operations,
there's the possibility the animation could be cancelled before the lock takes effect.
Attachment #778408 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 17

5 years ago
Created attachment 778411 [details] [diff] [review]
Mitigate dynamic toolbar not getting locked when it should

I'm not a fan of this patch, but it ought to mitigate situations where the dynamic toolbar is incorrectly not locked into position. I'd prefer we take the other simple fix and see if that sorts the issue before pushing this though, as I think it makes the code a bit uglier.
Attachment #778411 - Flags: review?(bugmail.mozilla)
Comment on attachment 778408 [details] [diff] [review]
Another simple possible fix

Review of attachment 778408 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing as per IRC discussion. This doesn't make much of a difference because both those calls are nonblocking and will run regardless of whatever else happens. The animation can still be cancelled after the two lines of code and it will still be a problem.
Attachment #778408 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 778479 [details] [diff] [review]
Likely, simple, possible fix

kats helped me to notice that the animation-cancelling logic in LayerMarginsAnimator was wrong.

As well as reversing the call order in BrowserApp (which is probably not necessary, but could mitigate future threading issues), this also moves the cancelling inside the check to see if margins are pinned so that margin animations aren't cancelled by scrolling when margins are pinned.
Attachment #775667 - Attachment is obsolete: true
Attachment #778479 - Flags: review?(bugmail.mozilla)
Attachment #778479 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 778411 [details] [diff] [review]
Mitigate dynamic toolbar not getting locked when it should

I also don't really like doing this. I'm going to drop the review flag for now but we can reconsider it later if the other patch doesn't do the job. I'm still leaning towards minusing this and trying to figure out the real fix for the issue.
Attachment #778411 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 22

5 years ago
You seem to be rather adept at hitting this bug Kevin, if you could test out a build once this hits central (or at your nearest convenience) and report back, that'd be much appreciated!

I'll hold off on requesting for beta/aurora for now but let me know if you think different.
Flags: needinfo?(kbrosnan)
Resetting status flags to reflect this new landing - we can adjust once Kevin confirms inability to reproduce.
status-firefox23: fixed → affected
status-firefox24: fixed → affected
status-firefox25: fixed → affected
https://hg.mozilla.org/mozilla-central/rev/c30555419404
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
status-firefox25: affected → fixed
(Reporter)

Comment 25

5 years ago
I have not been able to reproduce this while browsing on Twitter today and yesterday using Nightly. I would like to hold off on marking verified as it can be rather intermittent. Though I would recommend uplifting the patch for the next beta assuming there is still time.
Flags: needinfo?(kbrosnan)
(Assignee)

Comment 26

5 years ago
Comment on attachment 778479 [details] [diff] [review]
Likely, simple, possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbar getting stuck in its hidden state
User impact if declined: Pages that shrink enough to show the toolbar permanently will occasionally hide it permanently instead
Testing completed (on m-c, etc.): Tested on m-c, no reports of regressions so far and one report that it may have fixed the issue
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None
Attachment #778479 - Flags: approval-mozilla-beta?
Attachment #778479 - Flags: approval-mozilla-aurora?
Attachment #778479 - Flags: approval-mozilla-beta?
Attachment #778479 - Flags: approval-mozilla-beta+
Attachment #778479 - Flags: approval-mozilla-aurora?
Attachment #778479 - Flags: approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 25.0a1 (2013-07-28)
Device: LG Nexus 4
OS: Android 4.2.2
status-firefox25: fixed → verified
Unable to reproduce the issue on Firefox Mobile 23 RC build 1 and Aurora 24.0a2 2013-07-31 on the Samsung Galaxy R (Android 2.3.4). Marking the issue as Verified Fix
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
You need to log in before you can comment on or make changes to this bug.