Make the ReadingList sidebar look like other sidebars

VERIFIED FIXED in Firefox 38

Status

defect
P4
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Unfocused, Assigned: bwinton)

Tracking

(Blocks 1 bug)

Trunk
Firefox 39
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

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
Flags: qe-verify+
Flags: firefox-backlog+
(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?
Flags: needinfo?(clarkbw)
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.
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).
(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.
I think we need to talk with UX but this definitely feels like a lower priority to the other items we have to finish.
Flags: needinfo?(clarkbw)
Priority: -- → P4
(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.
Summary: Update ReadingList sidebar header to new design → Make the ReadingList sidebar look like other sidebars
I think I can take this bug, if that's okay with everyone…
Assignee: nobody → bwinton
Blocks: 1132074
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Posted patch bug1137556.diff (obsolete) — Splinter Review
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
Attachment #8582060 - Flags: ui-review?(mmaslaney)
Attachment #8582060 - Flags: review?(florian)
QA Contact: andrei.vaida
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Duplicate of this bug: 1144373
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).
Attachment #8582060 - Flags: review?(florian) → feedback+
(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!
Attachment #8582060 - Attachment is obsolete: true
Attachment #8582060 - Flags: ui-review?(mmaslaney)
Attachment #8582560 - Flags: ui-review?(mmaslaney)
Attachment #8582560 - Flags: review?(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;
Posted patch The next version. (obsolete) — Splinter Review
(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
Attachment #8582560 - Attachment is obsolete: true
Attachment #8582560 - Flags: ui-review?(mmaslaney)
Attachment #8582560 - Flags: review?(florian)
Attachment #8582605 - Flags: ui-review?(mmaslaney)
Attachment #8582605 - Flags: review?(florian)
Comment on attachment 8582605 [details] [diff] [review]
The next version.

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

Thanks!
Attachment #8582605 - Flags: review?(florian) → review+
Comment on attachment 8582605 [details] [diff] [review]
The next version.

Blake, can we change the orange highlight to white? xoxo
Attachment #8582605 - Flags: ui-review?(mmaslaney) → ui-review+
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+.  ;)
Attachment #8582605 - Attachment is obsolete: true
Attachment #8582646 - Flags: ui-review+
Attachment #8582646 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/538dfc749e73
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.