Closed Bug 1414721 Opened 7 years ago Closed 6 years ago

Colors of Onboarding nav icons are inconsistent

Categories

(Firefox :: Tours, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

Details

(Whiteboard: [photon-onboarding][photon-onboarding-newui])

Attachments

(13 files, 1 obsolete file)

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
Priority: -- → P3
Whiteboard: [photon-onboarding]
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
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.
Has Regression Range: --- → no
Has STR: --- → yes
OS: Unspecified → All
QA Contact: cristian.comorasu
Hardware: Unspecified → All
Version: unspecified → Trunk
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Attached image nav_icons_RTL.png (obsolete) —
Attached image notification_RTL.png
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 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;
Attachment #8926711 - Flags: review?(gasolin)
Attached image nav_icons_RTL_2.png
Attachment #8926705 - Attachment is obsolete: true
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 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
Attachment #8926711 - Flags: review?(gasolin) → review+
Keywords: checkin-needed
Flags: qe-verify+
Please land this in v59 so we don't need to deal with the potential regression at the last day of v58
Keywords: checkin-needed
Merge day (2017-11-13) has been branched out yesterday. Now Nightly is on Fx59. So let's land it
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/099f35fb5a7e
Colors of Onboarding nav icons are inconsistent, r=gasolin
(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.
https://hg.mozilla.org/mozilla-central/rev/099f35fb5a7e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Hi Cristian,
Could you help to verify this issue, thanks.
Flags: needinfo?(cristian.comorasu)
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.
Flags: needinfo?(cristian.comorasu)
(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?
Flags: needinfo?(cristian.comorasu)
Because 57 is released and this issue is not causing the onboarding unable to use so set wontfix on 57.
Here is a screenshot with the issue: https://imgur.com/a/p8y4R
Flags: needinfo?(cristian.comorasu)
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
Flags: needinfo?(cristian.comorasu)
The issue is fixed with the nightly from 2017-11-15. 
Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(cristian.comorasu)
(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 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
Attachment #8926711 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(fliu)
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
Flags: needinfo?(fliu) → needinfo?(gchang)
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 on attachment 8926711 [details]
Bug 1414721 - Colors of Onboarding nav icons are inconsistent,

Polish UI issue and was verified. Beta58+.
Flags: needinfo?(gchang)
Attachment #8926711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.