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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2016-01-31-11-25-49.png

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

Comment 1

[:Margaret Leibovic]

Is this a regression? This looks like a dynamic toolbar problem.

Comment 2

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

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.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, I see this every now and then too. It feels like something is blocking the UI thread and therefore interrupts the animation.

Comment 4

[Teodora Vermesan (:TeoVermesan)]

Attachment: 12659667_10153325930406720_1136268680_n.jpg

Also, opening a link from the tab queue notification will display Firefox with the URL Bar hidden.

Comment 5

[:Margaret Leibovic]

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?

Comment 6

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

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?

Comment 7

[:Margaret Leibovic]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 9

[:Margaret Leibovic]

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.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Existing code architecture

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1245493 - Add comments to existing code to clarify them. r=margaret

Review commit: https://reviewboard.mozilla.org/r/42903/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42903/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/42905/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42905/

Comment 15

[:Margaret Leibovic]

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

Comment 16

[:Margaret Leibovic]

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

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 20

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

mcomella, any update here? It's tracking 47 which is on beta now.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 25

[Andrzej Hunt :ahunt]

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 26

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

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.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Kats, can you comment on the flicker issue (comment 24)?

Comment 28

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

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/

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-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.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I recorded the page bump when the toolbar scrolls: https://youtu.be/Hp2SRwhoxDc

Comment 31

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

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?

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/94af575a59a5e5ef4ae2ce072c874e4727884325
Bug 1245493 - Add comments to existing code to clarify them. r=margaret

https://hg.mozilla.org/integration/fx-team/rev/5459831e5a2746943a29a4ea74a5593d93d22526
Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94af575a59a5
https://hg.mozilla.org/mozilla-central/rev/5459831e5a27

Comment 41

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

Hi Teodora, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 42

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

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)

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99e5f18db628

Comment 44

[Teodora Vermesan (:TeoVermesan)]

Tested on several devices on latest Nightly 49.0a1 (2016-05-04) I cannot reproduce the issue anymore.

Comment 45

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

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+

Comment 46

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

https://hg.mozilla.org/releases/mozilla-beta/rev/eac275b1daa5

Comment 47

[u421692]

I cannot reproduce the issue on Firefox 47 Beta 4 and latest Aurora