1137556 - Make the ReadingList sidebar look like other sidebars

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect, P4)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

As part of shipping ReadingList, we're starting with a modified design of the sidebar. This will be limited to just the ReadingList sidebar, as doing the other sidebars is reasonable amount of work.

The one thing that stands out is the sidebar header. We have the old design of the header bleeding through when showing the ReadingList. So I think we should update the style of the header *only* for the ReadingList, until the other sidebars are updated.

Mockup:
https://projects.invisionapp.com/share/NK1ZBQ6SY#/screens/60126996

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #0)
> As part of shipping ReadingList, we're starting with a modified design of
> the sidebar. This will be limited to just the ReadingList sidebar, as doing
> the other sidebars is reasonable amount of work.

Are we sure this is really necessary for v1?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Well, it's only a background color change of the header. The actual ReadingList sidebar was implemented with the newer style. The header looks obviously out of place otherwise, especially on windows where the header is the only thing that's blue.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Alternative is change the ReadingList sidebar so it uses the old styling of existing sidebars. mmaslaney requested the new styling for the ReadingList though (bug 1123517 comment 2).

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #3)
> Alternative is change the ReadingList sidebar so it uses the old styling of
> existing sidebars. mmaslaney requested the new styling for the ReadingList
> though (bug 1123517 comment 2).

Yes, this is what I was suggesting. I don't actually know what the difference is in detail, but if we can reduce scope by not having a custom style for Reading List only, that seems worth considering. We need to balance UX requirements with engineering complexities. I just want to make sure Bryan considered it.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

I think we need to talk with UX but this definitely feels like a lower priority to the other items we have to finish.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Screenshot:  http://d.pr/i/12b4w

Comment 7

[Florian Quèze [:florian]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #3)
> Alternative is change the ReadingList sidebar so it uses the old styling of
> existing sidebars. mmaslaney requested the new styling for the ReadingList
> though (bug 1123517 comment 2).

I talked with mmaslaney last week, and he specifically requested that we make the background of our readinglist sidebar look more like the bookmark and history sidebars on Windows/Linux. On Mac the background should be semitransparent, also like the other sidebars.

On Windows/Linux we may need to add a border at the top of our sidebar to separate the sidebar content from the sidebar header. The bookmarks sidebar already has such a border after its search field.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

I think I can take this bug, if that's okay with everyone…

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1137556.diff

Well, let's see how we like this…
Screenshots on each platform, with the bookmarks sidebar for comparison, are at:
https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/SidebarV1.png

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8582060 [details] [diff] [review]
bug1137556.diff

Review of attachment 8582060 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/readinglist/sidebar.css
@@ +5,5 @@
>  %include ../../shared/readinglist/sidebar.inc.css
>  
> +html {
> +  border: 1px solid -moz-use-text-color;
> +  -moz-border-top-colors: #B2AEAA #000;

I'm confused by these rules. Why do you need "-moz-use-text-color" if you are going to specify colors anyway?

And why are you specifying 2 different colors if it's a 1px border?

@@ +18,5 @@
>    -moz-padding-end: 0;
>  }
>  
> +.item.active {
> +  background-color: -moz-cellhighlight;

Should we change the text color at the same time as the background, like http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/listbox.css#44 does? (Note: this is not a rhetorical question, I haven't checked how things look on Linux with various GTK themes).

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch, with Florian's comments implemented.

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> > +html {
> > +  border: 1px solid -moz-use-text-color;
> > +  -moz-border-top-colors: #B2AEAA #000;
> 
> I'm confused by these rules. Why do you need "-moz-use-text-color" if you
> are going to specify colors anyway?
> 
> And why are you specifying 2 different colors if it's a 1px border?

It was a copy of the styles from the bookmark/history.  I cut it down to:
  border: 1px solid #B2AEAA;
  border-bottom-color: #FFF;
  border-left-color: #FFF;
but on closer inspection, it looks like there _is_ a border on the bookmarks, so now it's just:
  border: 1px solid #B2AEAA;


> @@ +18,5 @@
> > +.item.active {
> > +  background-color: -moz-cellhighlight;
> 
> Should we change the text color at the same time as the background, like
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> listbox.css#44 does? (Note: this is not a rhetorical question, I haven't
> checked how things look on Linux with various GTK themes).

I tried it with various themes in Ubuntu, and didn't see a difference, but it's probably better to include it.  Changed!

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8582560 [details] [diff] [review]
The next version of the patch, with Florian's comments implemented.

Review of attachment 8582560 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/readinglist/sidebar.css
@@ +4,5 @@
>  
>  %include ../../shared/readinglist/sidebar.inc.css
>  
> +html {
> +  border: 1px solid #B2AEAA;

Where is this #B2AEAA value coming from? mxr doesn't give any result for it.

@@ +5,5 @@
>  %include ../../shared/readinglist/sidebar.inc.css
>  
> +html {
> +  border: 1px solid #B2AEAA;
> +  background-color: #FFF;

I wonder if there are theme dependent values that we could use instead of these 2 hardcoded values. How will this #FFF look for the developer edition theme?

@@ +19,5 @@
> +  color: -moz-cellhighlighttext;
> +}
> +
> +.item.selected {
> +  background-color: Highlight;

color: HighlightText;

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version.

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> ::: browser/themes/linux/readinglist/sidebar.css
> @@ +4,5 @@
> > +  border: 1px solid #B2AEAA;
> Where is this #B2AEAA value coming from? mxr doesn't give any result for it.

I think I got it from the Inspector.  More notes below.

> @@ +5,5 @@
> > +  border: 1px solid #B2AEAA;
> > +  background-color: #FFF;
> I wonder if there are theme dependent values that we could use instead of
> these 2 hardcoded values. How will this #FFF look for the developer edition
> theme?

Changed to
  border: 1px solid ThreeDShadow;
  background-color: -moz-Field;
  color: -moz-FieldText;

(I threw the color in there, too, because why not, right? ;)

> @@ +19,5 @@
> > +  background-color: Highlight;
> color: HighlightText;

Added.

Now it looks like this https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/LinuxSidebar.png

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8582605 [details] [diff] [review]
The next version.

Review of attachment 8582605 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 16

[Michael Maslaney [:mmaslaney] ([e-mail])]

Comment on attachment 8582605 [details] [diff] [review]
The next version.

Blake, can we change the orange highlight to white? xoxo

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1137556.diff

Orange removed, to be like Windows and Mac.
I believe this is good enough for us to check in, and so have taken the liberty of carrying forward the r+ and ui-r+.  ;)

Comment 18

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/538dfc749e73

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/538dfc749e73

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bc473feb50a

Comment 21

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-29) and Aurora 38.0a2 (2015-03-29), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

The Reading List sidebar now looks the same as the other sidebars, but please note that 38.0a2 is currently affected by Bug 1137089.