1385518 - [Photon] Update sidebar styling for dark theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Background should be dark.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1385518 - (WIP) Update sidebar styling for dark theme.

This patch doesn't correctly handle changing themes during runtime or maintaining the colors while switching through the sidebars (history -> bookmarks for example). Also the hover states for the treechildren are not correct.

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8919591 [details]
Bug 1385518 - (WIP) Update sidebar styling for dark theme.

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8919591 [details]
Bug 1385518 - (WIP) Update sidebar styling for dark theme.

https://reviewboard.mozilla.org/r/190456/#review228422

::: browser/components/places/content/sidebarUtils.js:20
(Diff revision 2)
> +      document.documentElement.style.setProperty("--chrome-secondary-background-color", background);
> +      document.documentElement.style.setProperty("--chrome-color", color);
> +      document.documentElement.setAttribute("compacttheme", "true");

Note that you're definitely not supposed to be dirtying the DOM inside a promiseLayoutFlushed callback.

Comment 9

[Tim Nguyen :ntim]

This will be much easier to do with bug 1418602 fixed, leaving this bug on hold for now.

Comment 11

[Tim Nguyen :ntim]

Now that bug 1418602 is fixed, we can simply add the sidebar/sidebar_text properties to nsBrowserGlue.js.

Although, I'm thinking it's better to get bug 1484891 and bug 1485017 fixed first, to avoid shipping a half-baked dark sidebar.

Comment 12

[Tim Nguyen :ntim]

Attachment: Bug 1385518 - Update sidebar styling for dark theme. r=jaws

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 9005869 [details]
Bug 1385518 - Update sidebar styling for dark theme. r=jaws

Jared Wein [:jaws] (please needinfo? me) has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af2c47f0c8d7
Update sidebar styling for dark theme. r=jaws

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/af2c47f0c8d7

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 56.0a1 (2017-07-28) on Windows 10, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID   - 20180910220142
User Agent - Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64

QA Whiteboard: [bugday-20180905]

Comment 17

[Tim Nguyen :ntim]

Hi Amy, would it be possible to get a quick UI-review of the dark theme sidebar (bookmarks/history/synced tabs) during the 64 cycle ? This is already in Nightly.

Thank you.

Comment 18

[Amy Lee [:amylee] UX]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #17)
> Hi Amy, would it be possible to get a quick UI-review of the dark theme
> sidebar (bookmarks/history/synced tabs) during the 64 cycle ? This is
> already in Nightly.
> 
> Thank you.

Hi, 

Took a quick look and there are some edits I'd like to make. Will post them once I make my notes

Comment 19

[Amy Lee [:amylee] UX]

Hi, 

Please see below for the CSS for the dark them side bar. Thanks!

https://firefoxux.github.io/people/amlee/Dark-Theme-Sidebar

Comment 20

[Tim Nguyen :ntim]

(In reply to Amy Lee [:amylee] UX from comment #19)
> Hi, 
> 
> Please see below for the CSS for the dark them side bar. Thanks!
> 
> https://firefoxux.github.io/people/amlee/Dark-Theme-Sidebar

Thanks! I'll file bugs for individual items.

Comment 21

[Brindusa Tot, Desktop QA]

Attachment: 2018-10-01_17h20_03.png

Build ID 	20181001100147
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Verified this bug on latest Nightly 64.0a1 on Windows 10 x64 and Mac OS X 10.13 and the background color for the Dark theme is grey, not black. Please see the attachment. Is this the expected color for background on the sidebars?

Comment 22

[Amy Lee [:amylee] UX]

Yes this is correct. Thanks!

Comment 23

[Tim Nguyen :ntim]

Yes, that's correct!

Comment 24

[Brindusa Tot, Desktop QA]

Verified on Ubuntu 16.04 with latest Nightly 64.01 (10.02.2018) and the bug is fixed, the background color for sidebars is dark grey when using the Dark theme.