1061679 - [Browser Chrome] Press states of back, refresh, cancel and overflow menu using the wrong icons

Status: RESOLVED FIXED

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

Description

[Eric Pang [:epang] UX]

Hi Kevin,
Again feel free to reassign if this should be you.

The pressed states of the browser chrome icons seems to be incorrect.  When pressed the icons show are larger blue icon, they should be the same size.  The forward icon seems to be working correctly though. 

Let me know if you have any questions of need me to clarify.

Thanks!

Comment 1

[Eric Pang [:epang] UX]

(In reply to Eric Pang [:epang] from comment #0)
> Hi Kevin,
> Again feel free to reassign if this should be you.
> 
> The pressed states of the browser chrome icons seems to be incorrect.  When
> pressed the icons show are larger blue icon, they should be the same size. 
> The forward icon seems to be working correctly though. 
> 
> Let me know if you have any questions of need me to clarify.
> 
> Thanks!

Looks like it's a visual asset problem.  Going to fix the assets now and it should be a quick image swap.  Sorry about not catching this earlier!  I'll attach a link to the new assets soon.

Comment 2

[Kevin Grandon :kgrandon]

No worries, feel free to post a link or attach them here and I'll take a stab. Can we verify that it is only the @1.5x images?

I'll also probably flag you for a ui-review, thanks.

Comment 3

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #2)
> No worries, feel free to post a link or attach them here and I'll take a
> stab. Can we verify that it is only the @1.5x images?
> 
> I'll also probably flag you for a ui-review, thanks.

I was only able to check on 1.5 so I'm not sure.  But a few of the images at different sizes seemed to have problems so it's probably safer to replace all of them.

I've uploaded them to box here:
https://mozilla.box.com/s/xleyd4fnpl5e2gvo2bxr

Yep, if you can flag me as ui-review that would be great :)

Comment 4

[Kevin Grandon :kgrandon]

Hi Eric,

After looking at our assets for rocketbar it appears we have two sets of assets. One for "light" statusbar backgrounds, like the browser, and one for "dark" (mostly apps that request navigation chrome I believe).

Just wanted to check with you if we need to update the "dark" themed images as well? I'm not sure where these assets originally came from, but I think Vivien originally implemented them.

Current 1.5x back active icon: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/images/back_active%401.5x.png
Same asset for "dark" colors: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/images/dark/back_active%401.5x.png

Comment 5

[Kevin Grandon :kgrandon]

I guess the reason I ask is because all I see on box is for the "dark" skin, not the "light" skin.

Comment 6

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #5)
> I guess the reason I ask is because all I see on box is for the "dark" skin,
> not the "light" skin.

Hey Kevin, didn't realize this was how it was done.  I think we're good to update both since the Chrome itself is only one colour.  The dark and light is for the status bar icons.  Thanks!

Comment 7

[Kevin Grandon :kgrandon]

Hey Eric - I think we still need the "dark" themed icons for apps with navigation chrome. In this case the chrome/statusbar is black. If we want, we can still use the same icons in both cases.

Comment 8

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #7)
> Hey Eric - I think we still need the "dark" themed icons for apps with
> navigation chrome. In this case the chrome/statusbar is black. If we want,
> we can still use the same icons in both cases.

Hey Kevin, just did a quick check in photoshop with a black background and it looks like the icons will work for both light and dark themes :). Let's see how this looks when implemented and make tweaks if needed. Thanks!

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Hey Eric - since this is basically only an asset swap I'm wondering if you mind putting an R+ stamp on this after you get a chance to review it.

Additionally I noticed that we had some missing assets in the codebase for stop/reload, so those are now included. I did not replace the default icon states because it seems like we were missing assets for the "dark" skin, and I wasn't sure how or where those originally came from.

Let's try this and see if it fixes the problem. Thanks!

Comment 10

[Eric Pang [:epang] UX]

Attachment: Dark-Skin-screen.png

Hey Kevin, the light screen looks great.  But the dark skin doesn't seem to be implemented correctly according to Francis' spec.  It should have the refresh/favorite icon.  Also a white icon is being used, so it doesn't seem to be using the updated icons.  Can you take a look? Thanks!

Comment 11

[Kevin Grandon :kgrandon]

Hey Eric - just to confirm, do we want both the active and default icons for both skins? So we only have to worry about one set of icons?

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Kevin Grandon :kgrandon from comment #11)
> Hey Eric - just to confirm, do we want both the active and default icons for
> both skins? So we only have to worry about one set of icons?

I think that's what you want, so I'll go ahead and implement this and flag you again. Thanks!

Comment 13

[Ben Francis [:benfrancis]]

As far as I knew only the icons in the "dark" folder are ever used because both browser windows and app windows with browser chrome use the light background with dark icons as Eric requested.

Also, we agreed with Francis to not show the overflow menu or bookmark icon for app windows with browser chrome.

Kevin, how did you create the screenshot with the dark backround on the Rocketbar? I didn't know that was possible.

Comment 14

[Kevin Grandon :kgrandon]

(In reply to Ben Francis [:benfrancis] from comment #13)
> Kevin, how did you create the screenshot with the dark backround on the
> Rocketbar? I didn't know that was possible.

I think this is for apps with navigation chrome? I suppose Eric has an app with this installed on his device?

Comment 15

[Ben Francis [:benfrancis]]

Facebook requests browser chrome and gets the light background with dark icons, what is different about that app?

Comment 16

[Ben Francis [:benfrancis]]

Eric, if I remember correctly you asked for the light theme for apps with navigation chrome. It seems that for fullscreen apps with navigation chrome we currently show the dark theme. Is that wrong?

Comment 17

[Kevin Grandon :kgrandon]

This could be a bug with full-screen apps /w navigation. We have a test app here you can install via app manager or webIDE to test: https://github.com/mozilla-b2g/gaia/tree/master/apps/system/test/marionette/fullscreennavapp

Comment 18

[Kevin Grandon :kgrandon]

Eric - I've also updated the pull request. Please take another look when you get a chance.

Comment 19

[Kevin Grandon :kgrandon]

Eric - I've discussed with Ben about this over IRC and we have some concern that a single icon set may not be enough for all possible uses of theme-color. Are we sure that we only want a single set?

I'm still curious - where did the original "light" icons come from. Looping in Vivien to find out as he originally implemented them.

Comment 20

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #19)
> Eric - I've discussed with Ben about this over IRC and we have some concern
> that a single icon set may not be enough for all possible uses of
> theme-color. Are we sure that we only want a single set?
> 
> I'm still curious - where did the original "light" icons come from. Looping
> in Vivien to find out as he originally implemented them.

Hey Kevin,

At first I wasn't aware we had dark chrome for fullscreen apps.. so I installed a few apps until I found one. I didn't want to create more work for everyone so I thought it would be easier to keep the dark chrome even though not the original idea.

Would it be possible to make fullscreen apps use light chrome (same as the browser and apps that request navigational chrome?).  We should go light in all cases that request chrome, no need for the dark theme (that was my original intent).

Let me know if changes need to be made before I review, thanks!

Comment 21

[Kevin Grandon :kgrandon]

Well apps can request *any* color for the chrome, so we need to support dark and light really. I'm fine with landing with only a single color and seeing if it has any issues though, as our current color set doesn't seem complete or pixel perfect.

I'd say go ahead and review now and we can follow-up with a set of icons for a dark set if needed.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #19)
> Eric - I've discussed with Ben about this over IRC and we have some concern
> that a single icon set may not be enough for all possible uses of
> theme-color. Are we sure that we only want a single set?
> 
> I'm still curious - where did the original "light" icons come from. Looping
> in Vivien to find out as he originally implemented them.

I really don't remember. I think I found them on box at https://mozilla.app.box.com/s/xleyd4fnpl5e2gvo2bxr originally.

Comment 23

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon from comment #21)
> Well apps can request *any* color for the chrome, so we need to support dark
> and light really. I'm fine with landing with only a single color and seeing
> if it has any issues though, as our current color set doesn't seem complete
> or pixel perfect.
> 
> I'd say go ahead and review now and we can follow-up with a set of icons for
> a dark set if needed.

Hey Kevin, you're right we'll have problems using one set of icons.  For some reason I was thinking of the chrome as Light (light grey) or black.  I've created a new set of icons for use on dark chrome.

I've also updated the link for icons for light chrome.  

Dark Chrome: https://mozilla.box.com/s/phrotvr4tute6jurx2su
Light Chrome: https://mozilla.box.com/s/yv3zxakgw98uslnlrwns

Can you flag me again when it's ready for review? Thanks!

Comment 24

[Eric Pang [:epang] UX]

Comment on attachment 8484594 [details] [review]
Github pull request

looks good, thanks Kevin!  R+

Comment 25

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/0699f90f5dee055fba14fe617da0dcc9419253fc

Comment 26

[Kevin Grandon :kgrandon]

Comment on attachment 8484594 [details] [review]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation, initial assets were not provided in the correct sizes.
[User impact] if declined: Low quality UX with browser chrome assets.
[Testing completed]: Manual testing and UX verification.
[Risk to taking this patch] (and alternatives if risky): Should be low risk, we do replace a lot of images, but it's mainly only an asset swap, and some of the images have moved.
[String changes made]: None.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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