1414721 - Colors of Onboarding nav icons are inconsistent

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P3)

Description

[Fischer [:Fischer]]

Some icons sush as icons_default.svg didn't assign color in the not hovered state, unlike others with `fill="#3E3D40"`. We should check out the hovered state as well

Comment 1

[Fischer [:Fischer]]

Attachment: inconsistent-nav-icons.png

Comment 2

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

STR:

 1. Launch Firefox with a clean profile.
 2. Open a new tab.
 3. Click on the onboarding tour icon.

Expected:

 All the icons have the same color.

Actual:

 The Default Browser's icon is darker.

Regression Range:
 
 This is not a regression, as it reproduces ever since the icons were changed.

Comment 3

[Fischer [:Fischer]]

Attachment: new_user_tours_nav_icons.png

Comment 4

[Fischer [:Fischer]]

Attachment: new_user_tours_nav_icons_skipped.png

Comment 5

[Fischer [:Fischer]]

Attachment: update_user_tours_nav_icons.png

Comment 6

[Fischer [:Fischer]]

Attachment: update_user_tours_nav_icons_skipped.png

Comment 7

[Fischer [:Fischer]]

Attachment: notification_tour-icons.png

Comment 8

[Fischer [:Fischer]]

Attachment: nav_icons_RTL.png

Comment 9

[Fischer [:Fischer]]

Attachment: notification_RTL.png

Comment 10

[Fischer [:Fischer]]

Attachment: tour_nav_icons_high_contrast_mode.png

Comment 11

[Fischer [:Fischer]]

Attachment: notification_high_contrast_mode.png

Comment 12

[Fischer [:Fischer]]

Attachment: Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Previously we used background images to display tour icons on the nav list and one the tour notification. This caused two pitfalls: First, if fill colors were different inside svg icons, we would see inconsistent tour icon colors. Second, for one tour icon there would need two svg files.

This patch switchs to the mask-image approach, which makes a icon svg as a mask filtering the background color beneath, so we can unify icon colors by the external css and remove extra colored svg files.

Review commit: https://reviewboard.mozilla.org/r/197950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197950/

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

https://reviewboard.mozilla.org/r/197950/#review203146

::: commit-message-dc45e:5
(Diff revision 1)
> +Bug 1414721 - Colors of Onboarding nav icons are inconsistent, r?gasolin
> +
> +Previously we used background images to display tour icons on the nav list and one the tour notification. This caused two pitfalls: First, if fill colors were different inside svg icons, we would see inconsistent tour icon colors. Second, for one tour icon there would need two svg files.
> +
> +This patch switchs to the mask-image approach, which makes a icon svg as a mask filtering the background color beneath, so we can unify icon colors by the external css and remove extra colored svg files.

Hi Fred,

Please see the screenshots attached in the bug.

p.s In the high-contrast mode, we are the same as before, all tour icons are stripped off.

Comment 14

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197950/diff/1-2/

Comment 15

[[e-mail]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

https://reviewboard.mozilla.org/r/197950/#review204000

Looks good with 1 nit. Since this PR has the potential visual impact, let's wait a few days to land until the next branch (v59) begin.

::: browser/extensions/onboarding/content/onboarding.css:245
(Diff revision 2)
> +  position: absolute;
> +  offset-inline-start: 0px;
> +  top: 0px;
> +  background-color: #3E3D40;
> +  mask-repeat: no-repeat;
> +  mask-position: left 17px top 14px;

the icon position shows incorrectly in rtl mode, please add extra style for :dir(rtl) 

mask-position: right 17px top 14px;

Comment 16

[Fischer [:Fischer]]

Attachment: nav_icons_RTL_2.png

Comment 17

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

https://reviewboard.mozilla.org/r/197950/#review204000

> the icon position shows incorrectly in rtl mode, please add extra style for :dir(rtl) 
> 
> mask-position: right 17px top 14px;

Thanks for spotting this, updated: https://bugzilla.mozilla.org/attachment.cgi?id=8927716

Comment 18

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197950/diff/2-3/

Comment 19

[[e-mail]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

https://reviewboard.mozilla.org/r/197950/#review204352

looks good, please land this after the branch

Comment 20

[[e-mail]]

Please land this in v59 so we don't need to deal with the potential regression at the last day of v58

Comment 21

[[e-mail]]

Merge day (2017-11-13) has been branched out yesterday. Now Nightly is on Fx59. So let's land it

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/099f35fb5a7e
Colors of Onboarding nav icons are inconsistent, r=gasolin

Comment 23

[Fischer [:Fischer]]

(In reply to Fred Lin [:gasolin] from comment #21)
> Merge day (2017-11-13) has been branched out yesterday. Now Nightly is on Fx59. So let's land it
Don't worry about this. Sheriffs will take care about this so just setting "checkin-needed" should be enough.

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/099f35fb5a7e

Comment 25

[Fischer [:Fischer]]

Hi Cristian,
Could you help to verify this issue, thanks.

Comment 26

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I verified using Fx 59.0a1 (build ID: 20171114220116 ) on Windows 10 x64, Ubuntu 14.04 LTS and mac OS X 10.13.2. 
The issue is fixed on Windows 10 x64 and Ubuntu 14.04 LTS, however it is still reproducible on mac OS X 10.13.2.

Comment 27

[Fischer [:Fischer]]

(In reply to Cristian Comorasu [:CristiComo] from comment #26)
> I verified using Fx 59.0a1 (build ID: 20171114220116 ) on Windows 10 x64,
> Ubuntu 14.04 LTS and mac OS X 10.13.2. 
> The issue is fixed on Windows 10 x64 and Ubuntu 14.04 LTS, however it is
> still reproducible on mac OS X 10.13.2.
Hi Cristian,
Could you provide the failure screenshots on OS X 10.13.2, thanks?

Comment 28

[Fischer [:Fischer]]

Because 57 is released and this issue is not causing the onboarding unable to use so set wontfix on 57.

Comment 29

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Here is a screenshot with the issue: https://imgur.com/a/p8y4R

Comment 30

[Fischer [:Fischer]]

Attachment: 59.0a1_default_icon_screenshot.png

Comment 31

[Fischer [:Fischer]]

Attachment: 59.0a1_default_icon_inspection_steps.gif

Comment 32

[Fischer [:Fischer]]

Hi Cristian,

I tried 59.0a1 (build id: 20171115220414) on OSX and the result looks fine, please see [1].
Would you help us to download the latest nightly and see the icons again?

If you still found inconsistency, please follow the the inspection steps [2] to capture the inspection results like [1] for the defualt browse tour, the customize tour and the add-ons tour.

[1] attachment 8928807 [details]: 59.0a1_default_icon_screenshot.png
[2] attachment 8928808 [details]: 59.0a1_default_icon_inspection_steps.gif

Thank you

Comment 33

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

The issue is fixed with the nightly from 2017-11-15. 
Cheers!

Comment 34

[Fischer [:Fischer]]

(In reply to Cristian Comorasu [:CristiComo] from comment #33)
> The issue is fixed with the nightly from 2017-11-15. 
> Cheers!
Thanks Cristian for verifying this.

Comment 35

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1414721

[User impact if declined]: User would see inconsistent onboarding icon colors, which is bad for our onboarding UX.

[Is this code covered by automated tests?]: No because just the image and css updates.

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]:
Yes,
1. Open FF beta, go to about:config
2. Make sure browser.onboarding.tour-type is "new"
3. Go to about:newtab and click the onboarding fox icon on the top-left corner
4. Make sure the tour icons on the left navigation list have consistent colors, then close about:newtab
5. Go to about:config again
6. Make sure browser.onboarding.tour-type is "update"
7. Go to about:newtab again and click the onboarding fox icon on the top-left corner
8. Make sure the tour icons on the left navigation list have consistent colors

[List of other uplifts needed for the feature/fix]: No

[Is the change risky?]: No

[Why is the change risky/not risky?]: Only the image and css updates

[String changes made/needed]: No

Comment 36

[Gerry Chang [:gchang]]

Hi Fischer,
The [Feature/Bug causing the regression] in the template should not be this bug itself. Please put the which bug cause this issue or which feature bug we want to ship in 58.

Comment 37

[Fischer [:Fischer]]

Hi Gerry,
Thanks updated the uplift request:
(In reply to Fischer [:Fischer] from comment #35)
> Comment on attachment 8926711 [details]
> Bug 1414721 - Colors of Onboarding nav icons are inconsistent,
> 
> Approval Request Comment
  [Feature/Bug causing the regression]: bug 1382520
>
> [User impact if declined]: User would see inconsistent onboarding icon
> colors, which is bad for our onboarding UX.
> 
> [Is this code covered by automated tests?]: No because just the image and
> css updates.
> 
> [Has the fix been verified in Nightly?]: Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> Yes,
> 1. Open FF beta, go to about:config
> 2. Make sure browser.onboarding.tour-type is "new"
> 3. Go to about:newtab and click the onboarding fox icon on the top-left
> corner
> 4. Make sure the tour icons on the left navigation list have consistent
> colors, then close about:newtab
> 5. Go to about:config again
> 6. Make sure browser.onboarding.tour-type is "update"
> 7. Go to about:newtab again and click the onboarding fox icon on the
> top-left corner
> 8. Make sure the tour icons on the left navigation list have consistent
> colors
> 
> [List of other uplifts needed for the feature/fix]: No
> 
> [Is the change risky?]: No
> 
> [Why is the change risky/not risky?]: Only the image and css updates
> 
> [String changes made/needed]: No

Comment 38

[Fischer [:Fischer]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1382520

[User impact if declined]: User would see inconsistent onboarding icon colors, which is bad for our onboarding UX.

[Is this code covered by automated tests?]: No because just the image and css updates.

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]:
Yes,
1. Open FF beta, go to about:config
2. Make sure browser.onboarding.tour-type is "new"
3. Go to about:newtab and click the onboarding fox icon on the top-left corner
4. Make sure the tour icons on the left navigation list have consistent colors, then close about:newtab
5. Go to about:config again
6. Make sure browser.onboarding.tour-type is "update"
7. Go to about:newtab again and click the onboarding fox icon on the top-left corner
8. Make sure the tour icons on the left navigation list have consistent colors

[List of other uplifts needed for the feature/fix]: No

[Is the change risky?]: No

[Why is the change risky/not risky?]: Only the image and css updates

[String changes made/needed]: No

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Polish UI issue and was verified. Beta58+.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/baabad5cb968

Comment 41

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I verified using Fx 58.0b5, on Windows 10 x64, mac OS X 10.13.2 and Ubuntu 16.04 LTS x64.
Cheers!