Closed Bug 1038723 Opened 10 years ago Closed 10 years ago

[Utility Tray] Visual Refresh

Categories

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

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.1, b2g-v2.1 fixed, b2g-v2.2 verified, b2g-v2.5 verified, b2g-master verified)

VERIFIED FIXED
2.1 S4 (12sep)
feature-b2g 2.1
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- verified
b2g-v2.5 --- verified
b2g-master --- verified

People

(Reporter: epang, Assigned: gmarty)

References

Details

(Whiteboard: [systemsfe])

User Story

Visual Specs:

https://mozilla.box.com/s/yek8hkezu40yjwix4dca

Attachments

(7 files, 1 obsolete file)

This bug is to track the visual refresh of the utility tray.
feature-b2g: --- → 2.1
For info, I've started working on this feature in this branch: https://github.com/gmarty/gaia/tree/Bug-1038723-Utility-Tray-Visual-Refresh
Target Milestone: --- → 2.1 S2 (15aug)
Depends on: 1041916
Attached file Github PR (obsolete) —
Etienne can you review this PR? It's mostly CSS changes but some JS too.
Attachment #8476677 - Flags: review?(etienne)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Comment on attachment 8476677 [details] [review] Github PR Adding Salvador for the costcontrole part.
Attachment #8476677 - Flags: review?(salva)
Comment on attachment 8476677 [details] [review] Github PR It's a pretty big (and cool!) patch, it'll probably take a few rounds of review. Here are my first comments: * the active state for the quick settings is weird, it's only covering part of the icons * to a lesser extend the active state for the music controls too, since the icons aren't centered in the highlighted zone * the "grippy" zone doesn't look to spec, I like the spec one better :) * the opacity should be 1 for the part that covers the statusbar, otherwise you can see the "grippy" pass below which is weird * the media control style change is having side effects on the lockscreen. is this expected? * don't know if the spec covers this, but the big notification icon should maybe be gray when there's no unread notifications (we already have code to detect this I think) * not sure about the app install progress bar, it's jumping all around for me * the media playback integration tests probably need to be updated since the DOM changed * same for the update/install/media recording unit tests And don't forget to rebase the patch often, since it's big it's easily bitrotten. Cheers!
Attachment #8476677 - Flags: review?(etienne)
Comment on attachment 8476677 [details] [review] Github PR It would be great if I could review the UX spec of the Cost Control widget (not the visual spec but the functionality one) but I've found some issues: * Data Usage and Telephony / Balance panels are interchanged. They should be displayed at the left and right respectively. * The data being displayed as available in the data usage panel seems not consistent to me. Now it's displaying the current data usage when IMHO, it should display how much data is still available (limit - current) and some kind of "exceeded" message when the limit is exceeded. Although the patch seems ok to me, let's see the functionality spec before r+. Thank you a lot gmarty!
Attachment #8476677 - Flags: review?(salva)
Hi Eric! Can I see the functionality spec of the Cost Control widget, please?
Flags: needinfo?(epang)
(In reply to Salvador de la Puente González [:salva] from comment #6) > Hi Eric! > > Can I see the functionality spec of the Cost Control widget, please? Redirecting to Francis, do you know where we can find the ixd for the Cost Control widget in the utility? Gmarty and I tried to stick to the current functionality but it seems like we might be missing some. thx!
Flags: needinfo?(epang) → needinfo?(fdjabri)
Comment on attachment 8476677 [details] [review] Github PR Etienne, I addressed most of the issues you pointed out and need to discuss some with you. Can you please resume your review?
Attachment #8476677 - Flags: review?(etienne)
Stephany, can you help find the old Cost Control widget spec?
Flags: needinfo?(swilkes)
Peter, sure. But by "old" how old do you mean? :) Let me know and thanks.
Flags: needinfo?(swilkes)
(In reply to Stephany Wilkes from comment #10) > Peter, sure. But by "old" how old do you mean? :) Let me know and thanks. I don't think it's changed since early days, so 1.1/1.3 should be fine.
Flags: needinfo?(swilkes)
This is what I found on Box: https://mozilla.box.com/s/uov306al2n8ko5obyh77 It's so old though that I'm not sure how useful it is.
Flags: needinfo?(fdjabri)
Flags: needinfo?(swilkes)
This is not exactly what I want. I want to simply check what is the new functionality for the new "flat" widget. Can someone from user interaction review my comment 5? Rob, i see in [1] you're in charge of Smart Data Consumption (which I suppose is the new Cost Control) app? Could you tell us who is in charge of Cost Control user interaction specifically? Thank you! [1] https://wiki.mozilla.org/FirefoxOS/functionalteams#Functional_Team_with_Breakdown
Flags: needinfo?(rmacdonald)
(In reply to Salvador de la Puente González [:salva] from comment #5) > It would be great if I could review the UX spec of the Cost Control widget > (not the visual spec but the functionality one) but I've found some issues: > > * Data Usage and Telephony / Balance panels are interchanged. They should > be displayed at the left and right respectively. > * The data being displayed as available in the data usage panel seems not > consistent to me. Now it's displaying the current data usage when IMHO, it > should display how much data is still available (limit - current) and some > kind of "exceeded" message when the limit is exceeded. Guillaume just indicated that he'll swap the order or the widgets. The data being displayed should show how much data is still available as the visual spec shows.
Flags: needinfo?(rmacdonald)
Please Peter, could you define data availability in these cases: 1. Limit enabled and current usage below the limit. 3. Limit enabled and current usage above the limit. 2. Limit disabled. I understand the following definitions: 1. limit - current usage 2. does not apply - it should show how much I exceeded the limit (total would be useful as well) 3. does not apply - it should show how much I expended until now What do you think?
Flags: needinfo?(pdolanjski)
Flagging Eric/Francis for UX input.
Flags: needinfo?(pdolanjski)
Flags: needinfo?(fdjabri)
Flags: needinfo?(epang)
(In reply to Salvador de la Puente González [:salva] from comment #5) > * Data Usage and Telephony / Balance panels are interchanged. They should > be displayed at the left and right respectively. I was looking into it and realised that I haven't changed the respective placement of these panels. If you look at the pull request[1], you'll see that only the content of the #datausage-limit-view div changed. If it's an issue or a UX change, then I think it should be tracked by another bug. What do you think? [1] https://github.com/mozilla-b2g/gaia/pull/23153/files#diff-ba40a3d5eb758d6a622fcf1d8e4ea40eL37
I filed a followup to swap the order of the widgets, bug 1059902.
Thanks Salvador, I've created a visual spec that covers the three scenario's you mentioned. Guillaume, is this something we can implement? Also, flagging rob to make sure the ixd looks okay. Thanks!
Flags: needinfo?(rmacdonald)
Flags: needinfo?(gmarty)
Flags: needinfo?(epang)
Guillaume, it looks like bug 1059864 is still happening with your patch. It would be great to fix this before landing, but if it's not easy we can just take care of it in a followup.
(In reply to Eric Pang [:epang] from comment #19) > > Also, flagging rob to make sure the ixd looks okay. Thanks! Looks fine to me. Also, Valentine is the owner of the new cost control app but, as today is his final day, Katie Caldwell will be assuming ownership going forward. Thanks!
Flags: needinfo?(rmacdonald)
Flags: needinfo?(fdjabri)
No longer blocks: 1059864
Blocks: 1060118
(In reply to Peter Dolanjski [:pdol] from comment #18) > I filed a followup to swap the order of the widgets, bug 1059902. Perfects, let's not block on this and track it in the follow up!
(In reply to Rob MacDonald [:robmac] from comment #21) > (In reply to Eric Pang [:epang] from comment #19) > > > > Also, flagging rob to make sure the ixd looks okay. Thanks! > > Looks fine to me. > > Also, Valentine is the owner of the new cost control app but, as today is > his final day, Katie Caldwell will be assuming ownership going forward. > Thanks! Roger. I will be PTO but I'll send an email to :mai to let her know.
(In reply to Eric Pang [:epang] from comment #19) > Created attachment 8480724 [details] > Notifications_CostControl_Spec.jpg > > Thanks Salvador, > > I've created a visual spec that covers the three scenario's you mentioned. > > Guillaume, is this something we can implement? > > Also, flagging rob to make sure the ixd looks okay. Thanks! Thank you very much. It would be great if Katie could provide feedback but I feel we are not loosing functionality now.
Spec for music player to be consistent with music player in the notification tray.
Comment on attachment 8476677 [details] [review] Github PR All good! Thanks again for the quasi-instantaneous turnaround on the comments. We should do a last "rebase+wait for a green try" when the tree reopens before merging, but congrats on making FL!
Attachment #8476677 - Flags: review?(etienne) → review+
Thanks a million Etienne. I know it's quite a big patch. (and clearing the ni? flag)
Flags: needinfo?(gmarty)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
That merge caused a linter test failure. Even the try build on the pull request fails with the same error (the Li bug), and this risked a gaia tree closure on the last day of FL work. I did an emergency fix in this commit: https://github.com/mozilla-b2g/gaia/commit/c1625ae6a088ee362b88a2e857147f956f3baea8 but please in the future make sure any gaia-try errors in the pull request are not related to your work. I know it can be difficult sometimes to sort through some of them, but this one was fairly obvious that it was related to this changeset.
Depends on: 1060541
It looks like there are now two follow-ups for this commit, and the second of which is not even listed here. https://github.com/mozilla-b2g/gaia/commit/40be751fb4e79d49cb4470d104b34a06c4359afa
When you add variables (in this case {{value}}), explain what the variable is in a localization comment. I had to look at the specs to see that is going to be something like 100 MB, 2 GB (I though it was a currency format). > data-available = {{value}} available Not great in terms of localizability. It can be translated as "Available: {{value}}", in the current form the adjective can't be adjusted to the number, because this isn't a plural form. Also, looking at the specs: what happens with long translations? See also bug 863593 for more context. I really wish these things wouldn't land two days from FL.
Just flashed my phone with the last master. Given the font size used for titles (for example updates, screenshots), this is going to be a truncation festival during QA rounds.
Sorry guys but I'm reverting the patch as it causes a regression and drops Cost Control functionality. I agreed on moving the swap of panels in another bug but for the data displayed, let's implement the functionality described in comment 19 or leave it as is (not removing the data usage bar) and making the proper changes into another bug. Revert: a4140a11b05a1d2c70e7ee0abd6d33aa7d54bdf6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As I will at PTO since tomorrow, ask for :mai review. Thank you!
Sorry, the revert hash was invalid (an invalid commit pushed only to my Gaia repo). I'm not able to revert the patch so I'm going to file a bug to fix the Cost Control widget part leaving this as resolved. Please, contact :mai to review the patch. We should not land patches breaking functionality.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 1060928
Depends on: 1060946
No longer blocks: 1060903
Depends on: 1060903
With four regressions with UI breakage and functional breakage, I think it make sense to backout rather than attempt to fix these as blockers. master revert: 3d363af3097dcee95e4dc4ec4d0a4703b47f8e04 master revert2: a8f84e7bebf10d4c59b087484d559641573a95cb master revert3: b220d585b65d62950ced725075d677dfa717acc9 Separate e-mail to come.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think the easiest way is to split the patch for 1038723 in 2 patches: * 1 for the system app * 1 for the costcontrol app This is the patch for the system app that includes: * a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1061088#c2 * patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1060522 * https://github.com/mozilla-b2g/gaia/commit/c1625ae6a088ee362b88a2e857147f956f3baea8 Etienne, can you please review it (it has only a couple of CSS changes + test fix).
Attachment #8476677 - Attachment is obsolete: true
Attachment #8482287 - Flags: review?(etienne)
This is the second patch for the control cost widget only. It implements the specs as defined by :epang in [1]. However, Bug 1059902 will still have to be worked on. Marina, can you review it while :salva is away? [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1038723#c19
Attachment #8482288 - Flags: review?(mri)
Comment on attachment 8482287 [details] [review] Github PR for system app r=me if you fix the notifications-on-lockscreen css to make sure that we're not loosing the divider when a notification is selected.
Attachment #8482287 - Flags: review?(etienne) → review+
Patch 1 (system app) relanded in https://github.com/mozilla-b2g/gaia/commit/4793372476c1dd6c95bc15b1bc111a35080b19e7 Still waiting on review and gaia-try for patch 2 (cost control app).
Attached image widget.png
Hi Eric, I don't find specs for the following scenarios: 1. New sim 2. Airplanemode enabled 3. Locked sim 4. No sim 5. Balance not available I miss all the icons from the widget. Would you mind reviewing the attached image and check if is correct the current behaviour of the widget? Guillaume, if Eric confirm us that the icons must disappear, please, would you mind removing the files and all references to them? Regards
Flags: needinfo?(epang)
(In reply to Marina Rodríguez [:mai] from comment #42) > Created attachment 8482565 [details] > widget.png > > Hi Eric, > I don't find specs for the following scenarios: > 1. New sim > 2. Airplanemode enabled > 3. Locked sim > 4. No sim > 5. Balance not available > > I miss all the icons from the widget. Would you mind reviewing the attached > image and check if is correct the current behaviour of the widget? > > > > Guillaume, if Eric confirm us that the icons must disappear, please, would > you mind removing the files and all references to them? > > Regards Hi Marina, Thanks for the new scenarios, the only one I'm concerned with is airplane mode because it overlaps with the arrow. Can we update to "Turn off airplane mode to enable usage"? I think we might need help from the localization team for shorter strings in general though since in other languages we might run into truncation issues. I removed the icons and graphics from the widget on purpose. The screen is already very busy visually, so we wanted to make it more simple and clean. Functionally the widget should still work the same way - with just a visual refresh Thanks!
Flags: needinfo?(epang)
(In reply to Marina Rodríguez [:mai] from comment #42) > Guillaume, if Eric confirm us that the icons must disappear, please, would > you mind removing the files and all references to them? Hey Marina, I've removed the images from widget/icons/ and widget/ui/. Waiting on TBPL and your r+ :-)
Comment on attachment 8482288 [details] [review] Github PR for cost control Great job! The patch work fine. r+ As agreed offline, please open the follow-ups to adding more unit test and to remove the fte-icon element and all the references to them. Please land once TBPL is Green.
Attachment #8482288 - Flags: review?(mri) → review+
Depends on: 1061636
Depends on: 1061638
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 1063216
Comment on attachment 8482287 [details] [review] Github PR for system app This creates a much nicer visual implementation for the utility tray. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Visual polish. [User impact] if declined: The utility tray will not match the recently overhauled status and rocketbar. [Testing completed]: Manual testing. [Risk to taking this patch] (and alternatives if risky): Has been sitting on mater for a while now, so I think it should be low risk. [String changes made]: Looks like it adds a new string, not sure what this feature was doing before that.
Attachment #8482287 - Flags: approval-gaia-v2.1?(bbajaj)
Comment on attachment 8482288 [details] [review] Github PR for cost control Also needed. See comment 48.
Attachment #8482288 - Flags: approval-gaia-v2.1?(bbajaj)
Attachment #8482287 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Attachment #8482288 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
I know that the system app isn't really my main area (since I'm on the media team), but I was a bit surprised that it seems like no one on the music app was informed about the visual changes to the media playback widget, since its whole purpose is to relay information from the music app. The current UI honestly just looks broken to me (although I'm willing to admit that that's at least 50% personal preference, since I liked seeing the album art), and I had to go investigate what broke, only to find this bug. I'm not sure if I'm just out of the loop, and Dominic was aware of this, but for future changes like this, a quick heads-up to the media team would be nice. Thanks!
(In reply to Jim Porter (:squib) from comment #51) > I know that the system app isn't really my main area (since I'm on the media > team), but I was a bit surprised that it seems like no one on the music app > was informed about the visual changes to the media playback widget, since > its whole purpose is to relay information from the music app. The current UI > honestly just looks broken to me (although I'm willing to admit that that's > at least 50% personal preference, since I liked seeing the album art), and I > had to go investigate what broke, only to find this bug. > > I'm not sure if I'm just out of the loop, and Dominic was aware of this, but > for future changes like this, a quick heads-up to the media team would be > nice. Thanks! Hey Jim, thanks for the feedback! This was purely a visual refresh, Hung who owns visuals for media was aware of the change. I also prefer having the album art be displayed, but because of the business of the utility tray we decided to remove it for some breathing space. I tried a bunch of comps to try and make it work! In the future the music widget will mostly resemble what is implemented in the music app. fyi, We opened a follow up bug to fix highlight states, alignment, icons size, etc. bug 1063026 sorry, no album art tho :(
Depends on: 1067356
Marcia, can this meta bug and its dependencies be verified on 2.1?
Flags: needinfo?(mozillamarcia.knous)
Blocks: 1079394
No longer blocks: 1079394
Depends on: 1079394
(In reply to Tony Chung [:tchung] from comment #53) > Marcia, can this meta bug and its dependencies be verified on 2.1? Kevin, please help with verifyme against 2.1, as marcia is out today. see the list of fixed dependencies.
Keywords: verifyme
Flags: needinfo?(mozillamarcia.knous) → needinfo?(ktucker)
Flags: needinfo?(ktucker) → needinfo?(ychung)
Yeojin, I know this cannot be completely verified because of the issues that are still in "new status" but please verify any dependencies that are currently resolved as fixed.
Depends on: 1081278
(In reply to KTucker [:KTucker] from comment #55) > Yeojin, I know this cannot be completely verified because of the issues that > are still in "new status" but please verify any dependencies that are > currently resolved as fixed. All resolved dependencies are either verified, invalid, or unable to verify with [QAnalyst-verify-] tag. There are currently 4 new dependencies on this bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ychung) → needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Hey Guillaume, I just noticed that the <ul><li> was reversed in [1] (<ul> inside <li> instead of <li> inside <ul>). Maybe you'd want to make it the other way around? [1] https://github.com/mozilla-b2g/gaia/commit/4793372476c1dd6c95bc15b1bc111a35080b19e7#diff-61c16a0a0190e95d51031835f62b30d7R614
Flags: needinfo?(gmarty)
Comment on attachment 8566975 [details] [review] [gaia] gmarty:Bug-1038723-Utility-Tray-Visual-Refresh-fix > mozilla-b2g:master Wow! No idea what happened here. I must have been focusing on something else :-) Anyway, if it looks good to you, can you r+. Thanks! Also, that's probably a bad idea, but I wonder if we need a HTML validator as part of our linting tools.
Flags: needinfo?(gmarty)
Attachment #8566975 - Flags: review?(felash)
Comment on attachment 8566975 [details] [review] [gaia] gmarty:Bug-1038723-Utility-Tray-Visual-Refresh-fix > mozilla-b2g:master Since you're not a peer and I'm not either, I'm asking the original reviewer. But this loooks good to me. I also think this should be in its own separate bug, blocking this one.
Attachment #8566975 - Flags: review?(felash)
Attachment #8566975 - Flags: review?(etienne)
Attachment #8566975 - Flags: feedback+
Comment on attachment 8566975 [details] [review] [gaia] gmarty:Bug-1038723-Utility-Tray-Visual-Refresh-fix > mozilla-b2g:master Can you file a follow up bug? This was fixed and uplifted a long time ago. If we start piling on top of it now without separate bugs it could get messy :)
Attachment #8566975 - Flags: review?(etienne)
Depends on: 1136089
No Repro per following builds: RESULT: When set and reset Useage Data Alert, the Notifications stray immediatley presents changes and also displays alerts as expected. Also, when toggling the various components; Wifi, Airplane Mode, bluetooth, Cell Data, the Notification Bar will display the status appropriatley. Environmental Variables: Device: Aries 2.6 BuildID: 20151218153622 Gaia: d069027f9af6f835ef869f1f01b52339e5a3f423 Gecko: c5cb194cc9cb56d742fb3a7a826f0080b0404edc Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56 Version: 46.0a1 (2.6) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:46.0) Gecko/46.0 Firefox/46.0 Environmental Variables: Device: Flame 2.6 BuildID: 20151218030236 Gaia: 2f093462969d2c5f65dced3908a7abff6b1913e8 Gecko: 66fb852962c0d5f6f5fe0604204da4f5d17763c9 Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a Version: 46.0a1 (2.6) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:46.0) Gecko/46.0 Firefox/46.0 Environmental Variables: Device: Flame 2.5 BuildID: 20151217164854 Gaia: eeed1451e0e48b63abe3199e4d6906adc2a762d2 Gecko: 94905dc59d7286b7fe627afbcddafc495894f08d Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a Version: 44.0 (2.5) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0 Environmental Variables: Device: Flame 2.2 Build ID: 20151217032501 Gaia: 885647d92208fb67574ced44004ab2f29d23cb45 Gecko: 2120b3e6f680 Gonk: bd9cb3af2a0354577a6903917bc826489050b40d Version: 37.0 (2.2) Firmware Version: v18D User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 REPRO per Aries 2.5 RESULT: When toggle Airplane at the Notification UI, the Switch at Settings does not reflect the change. Device: Aries 2.5 BuildID: 20151217165758 Gaia: eeed1451e0e48b63abe3199e4d6906adc2a762d2 Gecko: 94905dc59d7286b7fe627afbcddafc495894f08d Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56 Version: 44.0 (2.5) Firmware Version: D5803_23.1.A.1.28_NCB.ftf User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Keywords: verifyme
I believe that the 2.5 Aries issue listed above was actually: https://bugzil.la/1228513
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: