Closed
Bug 1389002
Opened 7 years ago
Closed 7 years ago
Update background color of preferences page to match updated Photon visual spec.
Categories
(Firefox :: Settings UI, enhancement, P1)
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+
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-preference][triage]
Assignee | ||
Comment 1•7 years ago
|
||
Hi Helen,
Could you help update background color on the spec when you have time?
Thank you very much.
Flags: needinfo?(hhuang)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895697 -
Flags: review?(mconley)
Assignee | ||
Comment 4•7 years ago
|
||
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.
Updated•7 years ago
|
Whiteboard: [photon-preference][triage] → [photon-preference]
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
mozreview-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/#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-
Comment 8•7 years ago
|
||
common.inc.css is the place to do this but you'll need rebased on current mozilla-central for bug 1388761.
Comment 9•7 years ago
|
||
mozreview-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/#review172450
Attachment #8895697 -
Flags: review- → review?(mconley)
Comment 10•7 years ago
|
||
Ah, thanks - didn't notice the dependency. Will re-review now.
Comment 11•7 years ago
|
||
(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 12•7 years ago
|
||
mozreview-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/#review172452
Right, yeah, agreed - this fails to apply on m-c now.
Attachment #8895697 -
Flags: review?(mconley) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
Hi Mike,
I rebased.
Could you review it again?
And this is the screenshot[1] for the update.
[1]: http://imgur.com/a/w066T
Comment 15•7 years ago
|
||
Hey evanxd,
I'm on PTO now, but back next Thursday. Perhaps you could get timdream to review this in the meantime?
Updated•7 years ago
|
Attachment #8895697 -
Flags: review?(mconley) → review?(timdream)
Comment 16•7 years ago
|
||
mozreview-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/#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®exp=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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
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
Comment 20•7 years ago
|
||
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
Assignee | ||
Comment 21•7 years ago
|
||
According to my understand, we should have the preferences reviewer's r+ here. That is Jared or Mike.
Assignee | ||
Updated•7 years ago
|
Attachment #8895697 -
Flags: review?(jaws)
Assignee | ||
Comment 22•7 years ago
|
||
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 23•7 years ago
|
||
mozreview-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/#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 24•7 years ago
|
||
mozreview-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/#review173456
r=me with the background-color removed from the categories selector on common.inc.css
Attachment #8895697 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-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
Comment 27•7 years ago
|
||
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
Comment 28•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
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
Comment 32•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
Rebased and fixed the conflict issue caused by Bug 1377174.
Comment 35•7 years ago
|
||
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
Comment 36•7 years ago
|
||
Backed out so bug 1377174 could be backed out:
https://hg.mozilla.org/integration/autoland/rev/4ba3d5ff24ec1229508e6ad839a95d797b7001d4
Flags: needinfo?(rchien)
Comment 37•7 years ago
|
||
@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)
Assignee | ||
Comment 38•7 years ago
|
||
Sure, let's update it until we have the new color code.
Flags: needinfo?(evan)
Comment 39•7 years ago
|
||
I've confirmed with the team that to use #F9F9FA as background color in Preferences.
Flags: needinfo?(hhuang)
Updated•7 years ago
|
Keywords: checkin-needed
Comment 40•7 years ago
|
||
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
Comment 41•7 years ago
|
||
bugherder |
Comment 42•7 years ago
|
||
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
Comment 43•7 years ago
|
||
Could you close the reviewboard submission? The patch was pushed from inbound so reviewboard didn't close automatically. Thanks.
Flags: needinfo?(evan)
You need to log in
before you can comment on or make changes to this bug.
Description
•