Closed
Bug 1134766
Opened 8 years ago
Closed 8 years ago
Possible to get browser chrome into broken state
Categories
(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | verified |
b2g-master | --- | verified |
People
(Reporter: benfrancis, Assigned: apastor)
Details
(Keywords: dogfood, regression, Whiteboard: [systemsfe])
Attachments
(4 files)
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.
Reporter | ||
Comment 1•8 years ago
|
||
Can QA reproduce this? If so, please nominate for 2.2+
Keywords: qawanted
Updated•8 years ago
|
QA Contact: bzumwalt
Comment 2•8 years ago
|
||
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.
Reporter | ||
Comment 3•8 years ago
|
||
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.
Updated•8 years ago
|
blocking-b2g: 2.2? → 2.2+
Comment 4•8 years ago
|
||
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
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Flags: needinfo?(ktucker)
Keywords: qawanted → regression
Updated•8 years ago
|
Comment 5•8 years ago
|
||
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
Updated•8 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → apastor
Assignee | ||
Comment 6•8 years ago
|
||
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!
Flags: needinfo?(chrislord.net)
Comment 7•8 years ago
|
||
(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?
Flags: needinfo?(chrislord.net)
Assignee | ||
Comment 8•8 years ago
|
||
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!
Flags: needinfo?(chrislord.net)
Comment 9•8 years ago
|
||
(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).
Flags: needinfo?(chrislord.net) → needinfo?(alive)
Comment 10•8 years ago
|
||
(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?
Flags: needinfo?(alive)
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
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.
Attachment #8569136 -
Flags: review?(alive)
Comment 13•8 years ago
|
||
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.
Attachment #8569136 -
Flags: review?(alive) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•8 years ago
|
||
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!
Flags: needinfo?(bfrancis)
Keywords: checkin-needed
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c5b086914cba6eb98961724423a0563d00d1f312
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 16•8 years ago
|
||
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
Flags: needinfo?(bfrancis) → needinfo?(etienne)
Comment 17•8 years ago
|
||
(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.
Flags: needinfo?(etienne)
Comment 18•8 years ago
|
||
Please request Gaia v2.2 approval on this patch when you get a chance.
Assignee | ||
Comment 19•8 years ago
|
||
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]: -
Flags: needinfo?(apastor)
Attachment #8569136 -
Flags: approval-gaia-v2.2?(bbajaj)
Updated•8 years ago
|
Attachment #8569136 -
Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Comment 20•8 years ago
|
||
v2.2: https://github.com/mozilla-b2g/gaia/commit/b575ab915380382eadaac44ac67b5ce59afaf2e9
Comment 21•8 years ago
|
||
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
Flags: needinfo?(bfrancis)
Updated•8 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Comment 23•8 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•