1384504 - [Photon] Update sidebar tree styling on Windows

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

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

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Patch only adds bottom border to sidebar header and adjusts spacing (based on interactions in other sidebar bugs, I haven't included any changes to the highlight style).

Comment 3

[Dão Gottwald [:dao]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #2)
> Patch only adds bottom border to sidebar header and adjusts spacing (based
> on interactions in other sidebar bugs, I haven't included any changes to the
> highlight style).

Are you going to file another bug?

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review166704

::: browser/themes/windows/browser-aero.css:19
(Diff revision 1)
>  @media (-moz-windows-default-theme) {
>    .sidebar-header,
>    #sidebar-header {
>      -moz-appearance: none;
> -    border-bottom: none;
> +    border-bottom: 1px solid hsla(240, 5%, 5%, .1);
>    }

Can we just get rid of this rule if we remove:

http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/toolkit/themes/windows/global/global.css#141-143

?

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review167744

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review174864

::: browser/themes/windows/browser.css:904
(Diff revision 2)
>  #sidebar {
>    background-color: Window;
>  }
>  
>  #sidebar-header {
> -  border-bottom: 1px solid ThreeDLightShadow;
> +  border-bottom: 1px solid hsla(240, 5%, 5%, .1);

I suspect the difference between this custom color and ThreeDLightShadow is rather small. Can you confirm? If so, I think we should just keep ThreeDLightShadow. I expect that the hsla value won't work for high contrast themes.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

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

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8890280 [details]
> Bug 1384504 - [Photon] Update sidebar tree styling on Windows.
> 
> https://reviewboard.mozilla.org/r/161406/#review174864
> 
> ::: browser/themes/windows/browser.css:904
> (Diff revision 2)
> >  #sidebar {
> >    background-color: Window;
> >  }
> >  
> >  #sidebar-header {
> > -  border-bottom: 1px solid ThreeDLightShadow;
> > +  border-bottom: 1px solid hsla(240, 5%, 5%, .1);
> 
> I suspect the difference between this custom color and ThreeDLightShadow is
> rather small. Can you confirm? If so, I think we should just keep
> ThreeDLightShadow. I expect that the hsla value won't work for high contrast
> themes.

Yeah, it's not a huge difference. I reverted it.

I also changed the background color of the sidebar to white in this patch, seems like it fits in the scope of this bug.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review174900

::: browser/themes/windows/browser.css:900
(Diff revision 3)
>  /* ::::: content area ::::: */
>  
>  %include ../shared/sidebar.inc.css
>  
> -#sidebar {
> -  background-color: Window;
> +#sidebar-box {
> +  background-color: -moz-Field;

Should also set -moz-fieldtext as the text color.

And it looks like we'll need this on Linux too...

::: browser/themes/windows/places/places.css:8
(Diff revision 3)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* Sidebars */
> +
> +:root {
> +  background-color: transparent;

I don't think transparency works here, you're probably just getting the same white default background as web content this way.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

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

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review174900

> Should also set -moz-fieldtext as the text color.
> 
> And it looks like we'll need this on Linux too...

I'll file another bug for Linux.

> I don't think transparency works here, you're probably just getting the same white default background as web content this way.

Nah, I tested by changing the background of #sidebar-box to red; it seems to work.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #12)
> I'll file another bug for Linux.

Filed bug 1392222.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8890280 [details]
Bug 1384504 - [Photon] Update sidebar tree styling on Windows.

https://reviewboard.mozilla.org/r/161406/#review175916

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c5b27abffa7
[Photon] Update sidebar tree styling on Windows. r=dao

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/3c5b27abffa7

Comment 17

[ovidiu boca[:Ovidiu]]

Hi Nihanth, what do we need to verify here? Thanks

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to ovidiu boca[:Ovidiu] from comment #17)
> Hi Nihanth, what do we need to verify here? Thanks

This patch updates the styling of the sidebar header, the background color of the sidebar, and the spacing within the bookmarks/history list in the sidebar (all on Windows).

Comment 19

[ovidiu boca[:Ovidiu]]

Hi Nihanth,
We did a comparison between the expected results from the spec: https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252085   and the actual results on the latest Nightly 57.0a1(2017-08-24) and here are the results:

1. The text from "Bookmarks" has a bigger font than the one from spec
2. The colors from the close button "x", "Bookmarks" text the star and the arrow that is near to "Bookmarks" are different from the spec, in spec from what I see the colors are grayer.
3. The distance between search box and "Bookmarks" box is different from the one that is in the spec. 
4. In the History sidebar, an extra button "view" appears. 

These tests were made on Windows 10.

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to ovidiu boca[:Ovidiu] from comment #19)
> Hi Nihanth,
> We did a comparison between the expected results from the spec:
> https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252085   and the
> actual results on the latest Nightly 57.0a1(2017-08-24) and here are the
> results:
> 
> 1. The text from "Bookmarks" has a bigger font than the one from spec

We discussed this and we're leaving it large, see http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html

> 2. The colors from the close button "x", "Bookmarks" text the star and the
> arrow that is near to "Bookmarks" are different from the spec, in spec from
> what I see the colors are grayer.

The close button shares its style with the tab close button, so I think it's correct. As for the star and the arrow, I'll ask shorlander about this.

> 3. The distance between search box and "Bookmarks" box is different from the
> one that is in the spec.

Noted. Not sure if we should add the extra padding. I'll ask shorlander and Dao.

> 4. In the History sidebar, an extra button "view" appears. 

Yes, this is missing from the spec.

> These tests were made on Windows 10.

Thanks!

Comment 21

[ovidiu boca[:Ovidiu]]

Hi Nihanth, any news about this? Thanks.

Comment 22

[ovidiu boca[:Ovidiu]]

The issues presented at 1,2 and 4 are ok. The only question remains at point 3.

Comment 23

[ovidiu boca[:Ovidiu]]

Attachment: buttons from sidebar(Win7).png

We encounter another issue on Windows 7 x32 where the buttons from sidebar are different from the specifications. Please see the attached print screen with the actual result.

Comment 24

[Valentina Claudia Ona]

Attachment: bookmark.bmp

I verify this issue on Windows 8.1 x64 where the buttons from sidebar are different from the specifications. Please see the attachment.

Comment 25

[roxleitan]

Attachment: sidebar tree Ubuntu 16.04.png

Reproducible on Ubuntu 16.04 x64.

Comment 26

[Aaron Benson]

Attachment: sidebar-dropdown-states.png

Other than matching Stephen's html mock, we should update the button style for the dropdown button (History pane) to look like it does on macOS (label + dropdown arrow and a standard Photon hover style).

Comment 27

[Valentina Claudia Ona]

I verify this issue and for issue 3, from comment #19, I logged in bug 1400165.
Considering than issue 3 will be tracked in bug 1400165 and 1,2 and 4 are clarified, I will marked this verified.

Comment 28

[ovidiu boca[:Ovidiu]]

based on the fact that this issue is still reproducible in Ubuntu, I 've logged bug 1400266.