Update the tool bar to use the new 1.3 light tool bar design (with gray icons)

Hey Dominic, I just wanted to note that the patch from this bug needs is dependent with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=947093 They will land together along with all other toolbar updates to remain consistent. Thanks!

Comment on attachment 8343601 [details] patch for Gaia/master Stealing review

(In reply to James Burke [:jrburke] from comment #3) > Comment on attachment 8343601 [details] > patch for Gaia/master > > Stealing review Thanks James, you should be more qualified to review this, feel free to steal it.

Comment on attachment 8343601 [details] patch for Gaia/master Review comments are in the pull request. I would like to do another review round once comments are addressed, so please switch review back to me when the next pass is ready.

Comment on attachment 8343601 [details] patch for Gaia/master I think It's better now :)

Comment on attachment 8343601 [details] patch for Gaia/master Getting close now, just two more things, mentioned in the pull request: the spinning icon and the bottom toolbar buttons seem off in some way, see images here: https://github.com/mozilla-b2g/gaia/pull/14445/files#r8255340 Flip back for me for review when ready.

(In reply to James Burke [:jrburke] from comment #7) > Comment on attachment 8343601 [details] > patch for Gaia/master > > Getting close now, just two more things, mentioned in the pull request: the > spinning icon and the bottom toolbar buttons seem off in some way, see > images here: > > https://github.com/mozilla-b2g/gaia/pull/14445/files#0 > > Flip back for me for review when ready. Hi James, is it mandatory that the refresh icon spins? It's the only icon on any toolbar to spin (and as much as I like it maybe it will be more consistent if we left it static. Plus Pavel's been having trouble with the center point :). Let us know what you think. We would like to get this reviewed since the more time that passes the more outdated this PR becomes (and it needs to be landed with a lot of other dependent bugs - so toolbars update across the OS at the same time). Thanks!

There were two issues with the patch: 1) the spinning icon 2) the bottom toolbar on the account_picker looks broken. See pictures in the pull request comment: https://github.com/mozilla-b2g/gaia/pull/14445#discussion_r8255340 So number 2 definitely needs to be addressed. Perhaps it has, because looking at the pull request now there is a commit update after my last comment? If so, an r+ or at least a comment on the github pull request to indicate how it was addressed (or how I can update to see it addressed) would help me see that it was addressed. As for the spinning icon, it is technically a regression: the icon as it is today does not have this problem, and that means we will get a bug on it. For the animation, is it possible to take the existing images/icon/refresh.ping, use that as an underlay while making the new icon, to make sure the symmetry is maintained? The icon is animated just with a CSS rotation, so matching the existing icon (as far as symmetry layout) should be enough to fix it. I appreciate wanting to land and not have it bitrot, but also, in the general interest of the project trying to improve quality, I would like to see if we can use a bit of extra time to land it without regressions. If it is any help, I do not see massive visual changes coming to the email app in the next couple of weeks over the holidays, the email next/prev and possibly some work around email setup screens being the biggest things. I am available this week for reviews, so if it takes another day, then I can provide a quick turnaround on review.

See bug 882994 for some context on why we made the refresh button spin. That bug doesn't seem totally comprehensive, so a quick recap from memory: - We got complaints from the Leo device team that the explicit header progress bar was confusing/misleading. On IMAP we may actually go from 0% to 100% multiple times in succession, so a boolean indicator of synchronization is much better for us. We would need non-trivial back-end changes to make this smoother, or just to lie in general. - CPU issues due to having 2 animations, each of which took up about 30% of the CPU on their own, and 60%+ when both present. -- The 'candybar' striping that happens in the header progress bar when we haven't advanced any status could not be animated off of the main thread. (Because it was and still is animating background-position. Though it's possible that Gecko's performance has improved.) -- The spinner style progress for when we were "loading more messages" could be animated off the main thread because it's just a transform rotation. In general, I think it's effectively mandatory that we: - Have some (preferably boolean) indicator that synchronization is in process - Have the animation, if any animation is used, be animate-able off the main thread and either maintain or decrease our CPU overhead on supported devices (so ideally some type of transform). We are getting complaints from partners again about the e-mail app stuttering during scrolling, so we absolutely can't lose more CPU time to animations. Bug 827277 is potentially relevant, although little work has been done on it.

(In reply to Andrew Sutherland (:asuth) from comment #10) > See bug 882994 for some context on why we made the refresh button spin. > That bug doesn't seem totally comprehensive, so a quick recap from memory: > > - We got complaints from the Leo device team that the explicit header > progress bar was confusing/misleading. On IMAP we may actually go from 0% > to 100% multiple times in succession, so a boolean indicator of > synchronization is much better for us. We would need non-trivial back-end > changes to make this smoother, or just to lie in general. > > - CPU issues due to having 2 animations, each of which took up about 30% of > the CPU on their own, and 60%+ when both present. > -- The 'candybar' striping that happens in the header progress bar when we > haven't advanced any status could not be animated off of the main thread. > (Because it was and still is animating background-position. Though it's > possible that Gecko's performance has improved.) > -- The spinner style progress for when we were "loading more messages" could > be animated off the main thread because it's just a transform rotation. > > > In general, I think it's effectively mandatory that we: > - Have some (preferably boolean) indicator that synchronization is in process > - Have the animation, if any animation is used, be animate-able off the main > thread and either maintain or decrease our CPU overhead on supported devices > (so ideally some type of transform). We are getting complaints from > partners again about the e-mail app stuttering during scrolling, so we > absolutely can't lose more CPU time to animations. Bug 827277 is > potentially relevant, although little work has been done on it. Hi Andrew, I wasn't aware of the background on the rotating refresh icon. Thanks for the history behind it. I think we're good to keep the rotating icon, Pavel's just been having a hard time with the center point, so it keeps looking wobbly :).

Comment on attachment 8343601 [details] patch for Gaia/master Hey Guys, I tried a lot of things and I realized that the problem is not only the image size but also the button size (4.5rem) and I think I did the best I can at this moment. James, can you take a look? :) Andrew, if you have time for feedback it will be great :) Thanks in advance :)

Comment on attachment 8343601 [details] patch for Gaia/master (:jrburke's review should cover this, so clearing the feedback request against me. :jrburke can of course loop me in again if desired.)

Comment on attachment 8343601 [details] patch for Gaia/master That last two bits of review feedback, the spinning icon, and the account_picker bottom toolbar look great in this latest rev, good to go!

Landed to https://github.com/mozilla-b2g/gaia/commit/a5288df8de82cd84be14391c96f99a140535a78e

I think this may have wanted us to rev the html cookie cache version. I just got some very screwed-up looking UI on the toolbar.

The HTML cookie cache is calculated during build to be a hash of all the file contents, so hopefully it would have updated on its own. What could have been a problem: this patch depends on bug 947093 also being landed, so if you grabbed in between this one landing and that one, maybe that was a problem? Also, if `make DEBUG=1` was used to test, since that uses the source file version, it may be weird on first reload, since the hash version update is only done in the build_stage area. I have not thought to rev the cache version for that case given that it was just a dev thing. If it was not one of those things, then we might need to explore some more about what happened.

Reference for the digest calculation: https://github.com/mozilla-b2g/gaia/blob/master/apps/email/build/make_gaia_shared.js#L78 It is done in the build area, so should include the shared files too since that step is run after the Makefile copies the shared files to the buildDir.

(In reply to James Burke [:jrburke] from comment #17) > Also, if `make DEBUG=1` was used to test, since that uses the source file > version, it may be weird on first reload, since the hash version update is > only done in the build_stage area. I have not thought to rev the cache > version for that case given that it was just a dev thing. Yeah, this was it. CACHE_VERSION is always '1' in DEBUG builds. I just tried using DESKTOP=1 but not DEBUG=1 for running in Firefox nightly, and that totally does not work because the app protocol gets sad or something like that. Thanks for helping de-confuse my confuddled brain! I think at some point I forgot that we are clever and compute the version. Maybe as a future action item to make things more obvious we can rename VERSION TO HASH and have the string be something like 'hey-jerk-read-the-comment-above'.