Closed Bug 1389002 Opened 2 years ago Closed 2 years ago

Update background color of preferences page to match updated Photon visual spec.

Categories

(Firefox :: Preferences, enhancement, P1)

55 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: evanxd, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Update background color of preferences page to match updated Photon visual spec[1].

Currently, the background color mentioned in the spec is #FAFAFC. But after discussed with Helen in person, we should use the same color as the background color of the toolbar, which is #F9F9FA.

[1]: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683005
Flags: qe-verify+
Whiteboard: [photon-preference][triage]
Hi Helen,

Could you help update background color on the spec when you have time?

Thank you very much.
Flags: needinfo?(hhuang)
Attachment #8895697 - Flags: review?(mconley)
Hi Mike,

Could you help review the patch of updating background color of preferences page since we will have a updated visual spec to update background color as #F9F9FA to align toolbar's background color.

Thank you.
Whiteboard: [photon-preference][triage] → [photon-preference]
Depends on: 1388761
Yes, the updated background color for Preferences is #F9F9FA, please refer to the spec here https://mozilla.invisionapp.com/share/X8BGCX9PD#/244683005_Navigation
Flags: needinfo?(hhuang)
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review172440

The code looks okay, but this completely breaks the styles in about:addons:

https://screenshots.firefox.com/ATI7BHqRMDPUoZdl/i.imgur.com

I don't think we should land this unless we fix about:addons at the same time. Shipping broken-looking UI like this in Nightly should be avoided. Alternatively, we could shift the colour in about:preferences to #F9F9FA, and then move the style into common.inc.css once we move about:addons over to the new theme as well.
Attachment #8895697 - Flags: review?(mconley) → review-
common.inc.css is the place to do this but you'll need rebased on current mozilla-central for bug 1388761.
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review172450
Attachment #8895697 - Flags: review- → review?(mconley)
Ah, thanks - didn't notice the dependency. Will re-review now.
(In reply to Dão Gottwald [::dao] from comment #8)
> common.inc.css is the place to do this but you'll need rebased on current
> mozilla-central for bug 1388761.

And by "you" I mean Evan. The patch can't work in its current form.
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review172452

Right, yeah, agreed - this fails to apply on m-c now.
Attachment #8895697 - Flags: review?(mconley) → review-
Hi Mike,

I rebased.
Could you review it again?
And this is the screenshot[1] for the update.

[1]: http://imgur.com/a/w066T
Hey evanxd,

I'm on PTO now, but back next Thursday. Perhaps you could get timdream to review this in the meantime?
Attachment #8895697 - Flags: review?(mconley) → review?(timdream)
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review172608

::: toolkit/themes/shared/in-content/common.inc.css:636
(Diff revision 4)
>  
>  /* Category List */
>  
>  *|*#categories {
>    -moz-appearance: none;
> -  background-color: var(--in-content-category-background);
> +  background-color: var(--in-content-page-background);

Thanks for fixing this. There is another place to fix. The buttom-left "Raw JSON" label in about:telemetey

http://searchfox.org/mozilla-central/search?q=category-background&case=false&regexp=false&path=

Please also file a bug to Add-on Manager, so they can change the background color of "Get Add-ons" pane (hosted at discovery.addons.mozilla.org).
Attachment #8895697 - Flags: review?(timdream) → review+
See Also: → 1389426
Thank you for reviewing, Tim and Mike.

Let's land the patch once the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f240260965a2
The try is good, let's land it.
Keywords: checkin-needed
Autoland can't push this because the patch doesn't meet the review requirements needed to do so.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
According to my understand, we should have the preferences reviewer's r+ here. That is Jared or Mike.
Attachment #8895697 - Flags: review?(jaws)
Hi Jared,

Since Mike is on PTO and preferences's patches should get approvals from Preferences module owners/peers to be landed, could you help review it?

Thank you.
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review173400

::: toolkit/themes/shared/in-content/common.inc.css:636
(Diff revision 5)
>  
>  /* Category List */
>  
>  *|*#categories {
>    -moz-appearance: none;
> -  background-color: var(--in-content-category-background);
> +  background-color: var(--in-content-page-background);

Does this still need to be specified? It is now the same as the background color of the page so the initial color, `transparent`, should be acceptable.
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review173456

r=me with the background-color removed from the categories selector on common.inc.css
Attachment #8895697 - Flags: review?(jaws) → review+
Comment on attachment 8895697 [details]
Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar.

https://reviewboard.mozilla.org/r/166986/#review173792

::: toolkit/themes/shared/in-content/common.inc.css:636
(Diff revision 5)
>  
>  /* Category List */
>  
>  *|*#categories {
>    -moz-appearance: none;
> -  background-color: var(--in-content-category-background);
> +  background-color: var(--in-content-page-background);

We could set the background color as `transparent` here but cannot not remove the `background-color` style because we have another style for `richlistbox` elelment[1] will be applied after it's removed.

So my suggestion here is using `transparent` to instead of `var(--in-content-page-background)` here? What do you think, Jared?

[1]: http://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#749
Hi Jared,

What do you think of Comment 25?
Flags: needinfo?(jaws)
Very small nit: you may also consider the keyword `initial` which resolves to `transparent` for background-color.

https://developer.mozilla.org/en-US/docs/Web/CSS/initial
Yeah, you can switch to `initial`, but please add a comment saying "Override the background-color set on all richlistboxes in common.inc.css"
Flags: needinfo?(jaws)
Updated patch for the review comments. Let's land it once the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3a72891143
The try is good, let's land it.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 646ee77ccdc4 -d f5eed79a2b8c: rebasing 413889:646ee77ccdc4 "Bug 1389002 - Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar. r=jaws,timdream" (tip)
merging browser/themes/shared/incontentprefs/preferences.inc.css
merging toolkit/themes/shared/in-content/common.inc.css
warning: conflicts while merging toolkit/themes/shared/in-content/common.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased and fixed the conflict issue caused by Bug 1377174.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b420cda531b
Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar. r=jaws,timdream
Keywords: checkin-needed
@Evan, 

per discussion with Jared at https://bugzilla.mozilla.org/show_bug.cgi?id=1388984#c2 and offline discussion with Helen, we're very likely to follow Jared's opinion and do not modify the toolbar border color. Once bug 1388984 resolved invalid, Helen suggests that in order to make color of preferences distinct from toolbar, we might need to change preferences background color again.

In order to avoid double effort to change background color twice. I'd recommend that this issue should be postponed until Helen provides latest background color.



@Helen,

Can you help update the visual spec and provide the latest preferences background color here since as our consensus the bug 1388984 will be resolved invalid soon.

Thanks
Flags: needinfo?(rchien)
Flags: needinfo?(hhuang)
Flags: needinfo?(evan)
Sure, let's update it until we have the new color code.
Flags: needinfo?(evan)
I've confirmed with the team that to use #F9F9FA as background color in Preferences.
Flags: needinfo?(hhuang)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b57f7b4b3d9
Update background color as #F9F9FA to match updated Photon visual spec and the background color of toolbar. r=jaws,timdream
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2b57f7b4b3d9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Build ID: 20170820100343
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Could you close the reviewboard submission? The patch was pushed from inbound so reviewboard didn't close automatically. Thanks.
Flags: needinfo?(evan)
Sure, done.
Flags: needinfo?(evan)
You need to log in before you can comment on or make changes to this bug.