1134766 - Possible to get browser chrome into broken state

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

Attachment: screenshot

STR:
* Install Twitter app
* Launch Twitter app

Expected:
* No browser chrome

Actual:
* Browser chrome

Reproduced on 2.2 nightly build.

Does not reproduce 100% of the time, only seen once.

Comment 1

[Ben Francis [:benfrancis]]

Can QA reproduce this? If so, please nominate for 2.2+

Comment 2

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot_PossibleRepro

Ben, are you talking about the Rocketbar being expanded unexpectedly? If so, I can get this 100% of the time when I open Twitter app after restart with no data connection. Haven't gotten this to happen when using the app normally.

If this is what you are talking about I can confirm this happens on 2.2 and get a branch check in for this issue.

Comment 3

[Ben Francis [:benfrancis]]

Yes, that STR works for me and looks like it might be the same bug. If a web app with no browser chrome is launched with no data connection, the Rocketbar will display in the expanded state.

Nominating for 2.2+, but a branch check would be helpful, thank you.

Comment 4

[Brogan Zumwalt [Inactive]]

Issue reproduces in Flame 2.2 and 3.0

When data connection is disabled, the Twitter app is launched with an expanded rocketbar which blocks part of the screen.

Device:  Flame 2.2
BuildID: 20150219002504
Gaia: ce79d35b92261e7cbfeaefebf87859ebeb0979b4
Gecko: 159a3907b959
Version: 37.0a2 (2.2)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0
Build ID: 20150219010228
Gaia: 620aecfde85a8b093247837c55de2708e22be1e1
Gecko: 360b5f211180
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Issue does NOT reproduce on Flame 2.1

Rocketbar cannot be expanded by scrolling or any other means while in Twitter app from marketplace. Rocketbar is not expanded when launching Twitter app without data connection.

Device: Flame 2.1
Build ID: 20150219001626
Gaia: a43e3cdf8783e9d87156d47b8bfff0f5f44f9e2e
Gecko: 5653f229724f
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 5

[Brogan Zumwalt [Inactive]]

Unable to offer regression window due to significant drop in repro rate with earlier builds.

Following STR from Comment 2 the Twitter app launches with Rocketbar expanded while no data connection is present. Repro rate drops in earlier builds to ~1/7 with multiple restarts/reflashing likely required if issue does not appear on first attempt.

Issue last witnessed occurring on:
Device: Flame 2.2 Mozilla-Central-Flame-KK-Eng
Build ID: 20141218142705
Gaia: 2650bae535e15eef5f299cc80d4fab460f78ada7
Gecko: 121e801ae044
Version: 37.0a1 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 6

[Alberto Pastor [:albertopq]]

It seems that is happening in here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L683

That was introduced by :Cwiiis in Bug 1112594. Chris, any thoughts?

Thanks!

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Alberto Pastor [:albertopq] from comment #6)
> It seems that is happening in here:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.
> js#L683
> 
> That was introduced by :Cwiiis in Bug 1112594. Chris, any thoughts?
> 
> Thanks!

useCombinedChrome should only return true if the app is meant to have app chrome - is this definitely where the bar is being expanded, and if so, is useCombinedChrome returning the wrong value in this case? And if so, why?

Comment 8

[Alberto Pastor [:albertopq]]

Thanks for the reply Chris!

I'm not too familiar with this code, so I might me missing something, sorry about that.
What I see is that we are checking in usedCombinedChrome is:

  return this.app.config.chrome && !this.app.config.chrome.bar;

And the value of this.app.config.chrome at that moment is:

{ scrollable: false, maximized: false }

Which will return true, but not sure if that's the expected behavior. Should we add 

'&& this.app.config.chrome.scrollable === true'? Does that make sense? 

Thanks!

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Alberto Pastor [:albertopq] from comment #8)
> Thanks for the reply Chris!
> 
> I'm not too familiar with this code, so I might me missing something, sorry
> about that.
> What I see is that we are checking in usedCombinedChrome is:
> 
>   return this.app.config.chrome && !this.app.config.chrome.bar;
> 
> And the value of this.app.config.chrome at that moment is:
> 
> { scrollable: false, maximized: false }
> 
> Which will return true, but not sure if that's the expected behavior. Should
> we add 
> 
> '&& this.app.config.chrome.scrollable === true'? Does that make sense? 
> 
> Thanks!

It looks like app.config.chrome has changed since useCombinedChrome was written... I'm not very familiar with this code either, going to pass the n? onto Alive who knows it better than I do. Interested to see what's going on here...

My best guess is that you're correct, but I'd look at some other uses of config.chrome to verify (and verify that it takes the values you expect on a few stock/marketplace apps).

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

(I had to say appChrome had changed a lot since I wrote it.)

The content of this.app.chrome is changed by https://github.com/mozilla-b2g/gaia/commit/e5928de70f22a45f5c62d3e1022d6f452e0ae2ba which was Ben who reported this bug :)

Could you guys coordinate to figure out what should be done to fix? Thanks.
* Are we still configuring chrome.bar? Or are we supporting chrome.bar in manifest?

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1134766-chrome-twitter > mozilla-b2g:master

Comment 12

[Alberto Pastor [:albertopq]]

Comment on attachment 8569136 [details] [review]
[gaia] albertopq:1134766-chrome-twitter > mozilla-b2g:master

I'm going to play safe here and keep the code as compatible as possible with the current manifest. I'll make sure Ben reviews this, in order to create a new bug for deprecating manifest chrome values if needed.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8569136 [details] [review]
[gaia] albertopq:1134766-chrome-twitter > mozilla-b2g:master

r=me, hope that we have change to clean this up.

Comment 14

[Alberto Pastor [:albertopq]]

ni? Ben, just to keep this bug in his radar. Ben, is there any documentation about the allowed parameters in the manifest for the app chrome? Can we deprecate chrome.bar? Thanks!

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c5b086914cba6eb98961724423a0563d00d1f312

Comment 16

[Ben Francis [:benfrancis]]

chrome.bar is not a valid property of any app manifest as far as I know, app.config.chrome.bar must have been used for something else at some point.

Etienne, was this used for something during your and Vivien's implementation of the titlebar? Is it still needed?

Note: The documentation for the mozApps manifest is here https://developer.mozilla.org/en-US/Apps/Build/Manifest#chrome - this has been basically the same since August 2013. The documentation for the W3C manifest is here http://w3c.github.io/manifest/#display-modes

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Ben Francis [:benfrancis] from comment #16)
> chrome.bar is not a valid property of any app manifest as far as I know,
> app.config.chrome.bar must have been used for something else at some point.
> 
> Etienne, was this used for something during your and Vivien's implementation
> of the titlebar? Is it still needed?

It's older than that I think (vaguely remember porting it as part of the first titlebar implementation).
I think it's used by PopupWindow but we should be able to remove it and replace it with a popup special case.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.2 approval on this patch when you get a chance.

Comment 19

[Alberto Pastor [:albertopq]]

Comment on attachment 8569136 [details] [review]
[gaia] albertopq:1134766-chrome-twitter > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):-
[User impact] if declined: Rocketbar will be shown expanded when it shouldn't when there is not internet connection
[Testing completed]: Added unit tests
[Risk to taking this patch] (and alternatives if risky): One liner with tests. Low risk
[String changes made]: -

Comment 20

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/b575ab915380382eadaac44ac67b5ce59afaf2e9

Comment 21

[Shine(Leave from Mozilla)]

Attachment: Verify_video.MP4

Hi Ben,
I have verified this problem on latest build of Flame 2.0/3.0 and cannot repro this problem. Could you please confirm whether my STR is correct? If so, the verify result is Pass.
According comment 2, the STR is as follow:
1. Launch Marketplace to download and install Twitter app.
2. Launch Twitter app and log in Twitter account.
3. Slide down Notification and turn of wifi.
4. Restart device.
5. Launch Twitter app.
** The Rocketbar is displayed correctly, it will not being expanded unexpectedly.
See attachement: Verify_video.MP4
Rate:0/20

Flame 2.2 build: (Pass)
Build ID               20150319002500
Gaia Revision          9043c11f699c15bb6072422d1dad6518d1b5ddda
Gaia Date              2015-03-19 01:40:44
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0442d170bec
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150319.042028
Firmware Date          Thu Mar 19 04:20:38 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build: (Pass)
Build ID               20150319160212
Gaia Revision          c39e15f631de80c69467fda0d4ea0bcda9e194ca
Gaia Date              2015-03-18 19:30:04
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/cbd0efcd976c
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150319.193329
Firmware Date          Thu Mar 19 19:33:42 EDT 2015
Bootloader             L1TC000118D0

Comment 22

[Ben Francis [:benfrancis]]

That looks fine, Shine.

Comment 23

[Shine(Leave from Mozilla)]

(In reply to Ben Francis [:benfrancis] from comment #22)
> That looks fine, Shine.

Thanks Ben.
Per comment 21, The verify result on Flame 2.0/3.0 is Pass.