Closed Bug 1245493 Opened 8 years ago Closed 8 years ago

Opening a link from tab queue notification will display Firefox with the header cut off for a few seconds

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

47 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox47 verified, firefox48 verified, firefox49 verified, fennec47+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox47 --- verified
firefox48 --- verified
firefox49 --- verified
fennec 47+ ---

People

(Reporter: TeoVermesan, Assigned: mcomella)

References

Details

Attachments

(5 files)

Tested using:
Device: Nexus 4 (Android 5.1)


Steps to reproduce:
1. Enable "Open multiple links"
2. Go to gmail.com and tap on a link
3. Tap on the "x tab waiting" from the notification bar

Expected results:
- The link opens correctly in Firefox.

Actual results:
- The top of the page is cut off.
- The page is displayed correctly after a few seconds.

Note:
- no reproducible every time
- saw this issue a few times on my personal phone
Is this a regression? This looks like a dynamic toolbar problem.
tracking-fennec: --- → ?
I've seen this a couple of times as well but haven't been able to pin it down. I think it's the toolbar animation getting interrupted somehow but it's not clear to me how that would happen. It's likely a regression from bug 1180295.
Component: Overlays → Graphics, Panning and Zooming
Yeah, I see this every now and then too. It feels like something is blocking the UI thread and therefore interrupts the animation.
Also, opening a link from the tab queue notification will display Firefox with the URL Bar hidden.
We shouldn't even animate the toolbar when we're loading a new tab from another app like this - we should just display it immediately.

Can we get rid of the animation in this flow?
Flags: needinfo?(bugmail.mozilla)
I'm not really sure where the code for that flow is, or how to distinguish it from other flows. I'm guessing that right now it probably goes through the "SELECTED" case statement in BrowserApp.onTabChanged, which has an animation. Is there some way to tell that the tab being selected was opened from another app or from the queue?
Flags: needinfo?(bugmail.mozilla) → needinfo?(margaret.leibovic)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> I'm not really sure where the code for that flow is, or how to distinguish
> it from other flows. I'm guessing that right now it probably goes through
> the "SELECTED" case statement in BrowserApp.onTabChanged, which has an
> animation. Is there some way to tell that the tab being selected was opened
> from another app or from the queue?

I'm not sure off the top of my head. I imagine this would involve taking a look into our startup/intent handling code. I can look more closely later.
Assignee: nobody → rbarker
tracking-fennec: ? → 47+
Sorry I haven't had the chance to get back to this bug.

mcomella, can you help look into this? I definitely notice this often myself, and it definitely makes Firefox look janky.
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
I can confirm that changing the VisibilityTransition from ANIMATE to IMMEDIATE fixes this issue.

Looking for a fix that will figure out if this is a tab from an external app or not...
Assignee: rbarker → michael.l.comella
Flags: needinfo?(michael.l.comella)
One solution to this problem is to add
 mDynamicToolbar.setVisible(true, VisibilityTransition.IMMEDIATE)

to onStart so that we always re-display the toolbar when Firefox was entirely hidden from the users' perspective. This has a user benefit (e.g. the user knows which site they're on) but is annoying with the current implementation because the page doesn't scroll to compensate for the added toolbar. fwiw, Chrome does this, but only shows the toolbar briefly and then scrolls it away.

Without this, we have to keep some one-off state variables that start to feel like code smells to me.

However, given the change in user experience for the first solution, I'll start implementing the second.
After this patch, I still occasionally see the toolbar positioned part
way down from the top of the screen. However, this state looks slightly
less janky without the animation I removed and I can't consistently
reproduce it anymore.  Given the DynamicToolbar.setVisible calls I make,
I'd guess this is likely to be a bug caused by BrowserApp.onTabChanged
(and thus DynamicToolbar.setVisible) not getting called instantly and
so the DynamicToolbar is initialized to a different location on screen.
I'd guess it's a bug in DynamicToolbar as to why it's positioned partially
off-screen.

There is a little bit of code duplication, but that is because the code
to load a url on a new intent is duplicated (i.e. once from GeckoApp.initialize
- the initial load - and once from GeckoApp.onNewIntent). This could
potentially be cleaned up if we moved the app loading code into onResume,
but that may not be possible since we need to wait for Gecko to start
up.

Alternative solution: show the toolbar each time onStart is called (i.e.
FF is unhidden).  This is good for the user - they'll be more aware which
page they're on - but it's janky with the current implementation, where
the page content does not scroll when the toolbar is shown so previously
visible content is hidden. Thus, I went with the other approach.  fwiw,
Chrome does this behavior, but scrolls the toolbar offscreen shortly
after it is shown.

Review commit: https://reviewboard.mozilla.org/r/42905/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42905/
Comment on attachment 8735668 [details]
MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

https://reviewboard.mozilla.org/r/42903/#review39799
Attachment #8735668 - Flags: review?(margaret.leibovic) → review+
(In reply to Michael Comella (:mcomella) from comment #14)
> Created attachment 8735669 [details]
> MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF
> is first unhidden. r=margaret
> 
> After this patch, I still occasionally see the toolbar positioned part
> way down from the top of the screen. However, this state looks slightly
> less janky without the animation I removed and I can't consistently
> reproduce it anymore.  Given the DynamicToolbar.setVisible calls I make,
> I'd guess this is likely to be a bug caused by BrowserApp.onTabChanged
> (and thus DynamicToolbar.setVisible) not getting called instantly and
> so the DynamicToolbar is initialized to a different location on screen.
> I'd guess it's a bug in DynamicToolbar as to why it's positioned partially
> off-screen.
> 
> There is a little bit of code duplication, but that is because the code
> to load a url on a new intent is duplicated (i.e. once from
> GeckoApp.initialize
> - the initial load - and once from GeckoApp.onNewIntent). This could
> potentially be cleaned up if we moved the app loading code into onResume,
> but that may not be possible since we need to wait for Gecko to start
> up.
> 
> Alternative solution: show the toolbar each time onStart is called (i.e.
> FF is unhidden).  This is good for the user - they'll be more aware which
> page they're on - but it's janky with the current implementation, where
> the page content does not scroll when the toolbar is shown so previously
> visible content is hidden. Thus, I went with the other approach.  fwiw,
> Chrome does this behavior, but scrolls the toolbar offscreen shortly
> after it is shown.

I need to take more time to think through the logic in this patch, but I think we should consider this alternative solution. Partly because it sounds like it would be better UX, and partly because I don't want to add yet another special flag about how a tab was loaded.

I don't think we should scroll the content for the user, since it seems like that would be more unexpected than showing the toolbar on top of the content. If the user switched away from the app, I feel like they've already been taken out of the context they were in, so I don't know if it's that big a deal to show the toolbar on top of the content.

Would it be difficult to follow Chrome's behavior and animate the toolbar offscreen after a certain timeout?

I'd like antlam's input on what we should be doing here.
(In reply to :Margaret Leibovic from comment #16)
> Would it be difficult to follow Chrome's behavior and animate the toolbar
> offscreen after a certain timeout?

I don't think so, we just set a timer in onStart to animate the toolbar and cancel it by overriding @onUserInteraction (or whatever that callback is).

> I'd like antlam's input on what we should be doing here.

NI antlam – what do you think of using Chrome's approach of showing the toolbar when the user returns to the browser and hiding it after a short duration, Anthony?
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > Would it be difficult to follow Chrome's behavior and animate the toolbar
> > offscreen after a certain timeout?
> 
> I don't think so, we just set a timer in onStart to animate the toolbar and
> cancel it by overriding @onUserInteraction (or whatever that callback is).
> 
> > I'd like antlam's input on what we should be doing here.
> 
> NI antlam – what do you think of using Chrome's approach of showing the
> toolbar when the user returns to the browser and hiding it after a short
> duration, Anthony?

Hm, I'm not seeing this on my Nightly, 6P. But I actually think keeping the toolbar visible (no animation in or out) is fine. It's just like opening a new tab. 

When the user starts scrolling, we can scroll the toolbar off the page like we usually do. No need to add a timer.
Flags: needinfo?(alam)
Flags: needinfo?(michael.l.comella)
Spoke on Vidyo.

We're going to try out the, "Always show toolbar when returning to Firefox and pan it off-screen if it was off-screen before" approach.

Anthony also pointed out the toolbar hide/show animation can be slow when you have the toolbar half exposed – we should look into keeping the speed consistent no matter the position – I think this is in DynamicToolbarAnimator (and the ANIMATION_DURATION constant).
Flags: needinfo?(michael.l.comella)
Attachment #8735669 - Flags: review?(margaret.leibovic)
mcomella, any update here? It's tracking 47 which is on beta now.
Flags: needinfo?(michael.l.comella)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> mcomella, any update here? It's tracking 47 which is on beta now.

Thank you for the reminder – I lost track of this bug but will focus on it now.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8735668 [details]
MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42903/diff/1-2/
Attachment #8735669 - Attachment description: MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret → MozReview Request: Bug 1245493 - Temporarily display the dynamic toolbar in onStart if it is not shown. r=ahunt,f=kats
Attachment #8735669 - Flags: review?(ahunt)
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42905/diff/1-2/
Comment on attachment 8735668 [details]
MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

Kats, there is a flicker in the page content when the toolbar animates away – do you know why that is?

Also, it'd be great if you could verify my added DynamicToolbar.isVisible method is correct.
Attachment #8735668 - Flags: feedback?(bugmail.mozilla)
Attachment #8735669 - Flags: review?(ahunt) → review+
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

https://reviewboard.mozilla.org/r/42905/#review46285
Comment on attachment 8735668 [details]
MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

This looks pretty good. My only concern is the scenario where the user puts their finger down and starts scrolling during the 3 second timer,  such that the code to hide the toolbar runs while the user's finger is still down and/or the toolbar is partly offscreen. Please make sure the behaviour in that case isn't buggy.
Attachment #8735668 - Flags: feedback?(bugmail.mozilla) → feedback+
Kats, can you comment on the flicker issue (comment 24)?
Flags: needinfo?(bugmail.mozilla)
That's probably the same as bug 1245523. I had to do some hacks to try to sync up the gecko scroll position change with the visual change on the Java side, and in some cases it doesn't work perfectly. I think long-term this will go away if we adopt the proposal at https://github.com/bokand/URLBarSizing/
Flags: needinfo?(bugmail.mozilla)
(In reply to Michael Comella (:mcomella) from comment #19)
> Anthony also pointed out the toolbar hide/show animation can be slow when
> you have the toolbar half exposed – we should look into keeping the speed
> consistent no matter the position – I think this is in
> DynamicToolbarAnimator (and the ANIMATION_DURATION constant).

Filed bug 1268608 as a mentored bug.
I recorded the page bump when the toolbar scrolls: https://youtu.be/Hp2SRwhoxDc
Did you manually scroll the page in that video? Or was just the result of "toolbar.setVisible(false, VisibilityTransition.ANIMATE);" with no fingers on the screen? It's surprising to me that it scrolls the page so much. The black thing looks like the LayerView is lagging behind the toolbar by a lot. Do you see that normally when scrolling?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Did you manually scroll the page in that video?

I manually scrolled the page at the start of the video to hide the toolbar.

> Do you see that normally when scrolling?

When scrolling away the toolbar, I do see the black bar but it's normally slightly less apparent (it looks like the animation started to hang in the video).

fwiw, if we can't fix the issue directly, if we made the black space the same color as the toolbar, it might look a little better.
I spoke to Anthony. He doesn't want to ship the implementation I just made without fixing bug 1245523 first.

For fixing this bug, that leaves us with my initial approach (adding boolean flags about what state we're in when onTabChanged SELECTED gets called).
(In reply to Michael Comella (:mcomella) from comment #33)
> I spoke to Anthony. He doesn't want to ship the implementation I just made
> without fixing bug 1245523 first.

Filed bug 1269041 to implement this when bug 1245523 is fixed.

> For fixing this bug, that leaves us with my initial approach (adding boolean
> flags about what state we're in when onTabChanged SELECTED gets called).

Margaret gave me an r+ in comment 15 so I'm just going to land this.
Comment on attachment 8735668 [details]
MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42903/diff/2-3/
Attachment #8735669 - Attachment description: MozReview Request: Bug 1245493 - Temporarily display the dynamic toolbar in onStart if it is not shown. r=ahunt,f=kats → MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret
Attachment #8735668 - Flags: feedback+
Attachment #8735669 - Flags: review?(margaret.leibovic)
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42905/diff/2-3/
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

This was already r+'d by Margaret.
Attachment #8735669 - Flags: review?(margaret.leibovic)
Attachment #8735669 - Flags: review+
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

This request applies only to this patch.

Approval Request Comment
[Feature/regressing bug #]: Likely from bug 1180295.
[User impact if declined]: Users may see the toolbar half cut off when switching to Firefox from an external app.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: The code is tricky, but low risk – we add some boolean flags, and in the end, all we change is a flag that determines if we animate or change the view immediately. Worst case, we lose some toolbar animations or animate when we didn't mean to.
[String/UUID change made/needed]: None
Attachment #8735669 - Flags: approval-mozilla-beta?
Attachment #8735669 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/94af575a59a5
https://hg.mozilla.org/mozilla-central/rev/5459831e5a27
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Hi Teodora, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(teodora.vermesan)
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

UI regression, let's uplift to Aurora48 and wait for verification before uplifting to Beta (perhaps in time for 47.0b3)
Attachment #8735669 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested on several devices on latest Nightly 49.0a1 (2016-05-04) I cannot reproduce the issue anymore.
Flags: needinfo?(teodora.vermesan)
Comment on attachment 8735669 [details]
MozReview Request: Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

The fix was verified on Nightly, Beta47+
Attachment #8735669 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I cannot reproduce the issue on Firefox 47 Beta 4 and latest Aurora
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: