Here's a mockup of what the Type Controls should look like, versus what they currently look like: http://invis.io/3D2JR8ZJ5 We should try and clean this up a bit.

Can you talk a little bit more about what you want to happen in #2? (I think I understand #1, #3, and #4…)

Sure, the current implementation has the orange rule right above the gray outline, which is technically not wrong, but creates a bit of a blur effect. I'm wondering if we could extend the orange rule's height just one pixel, so that it covers the gray line.

Okay, let me know what you'ld like cleaned up in this version… :) (Screenshot at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV1.png ) One thing I noticed was that the spec had the + on the left, but the implementation has the + on the right. Is that something we should switch, or was that an intentional change?

(In reply to Blake Winton (:bwinton) from comment #3) > Created attachment 8584614 [details] [diff] [review] > The first cut at the patch. > > Okay, let me know what you'ld like cleaned up in this version… :) > (Screenshot at > https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV1.png ) • Type samples should roughly be the same size (use San Serif as the size benchmark) • Move Type labels slightly closure to the samples > One thing I noticed was that the spec had the + on the left, but the > implementation has the + on the right. Is that something we should switch, > or was that an intentional change? • Yes, thank you for bringing that up, ignore that part of the spec. We decided to move forward with the - | + format across platforms.

Comment on attachment 8584614 [details] [diff] [review] The first cut at the patch. • Type samples should roughly be the same size (use San Serif as the size benchmark) • Move Type labels slightly closure to the samples

How do you feel about https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/PanelV2.png instead? (It's subtle, but I swear I did what you asked for… ;)

Looks great. ;)

Okay, here's the patch with the updated styles.

Comment on attachment 8584867 [details] [diff] [review] The second cut at the patch. Review of attachment 8584867 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/windows/global/aboutReader.css @@ +22,5 @@ > .dark-button { > color: #eeeeee; > background-color: #333333; > + margin-top: -1px; > + height: 61px !important; This affects both the button and the page content. And !important is codesmell. I'd rather see these additions split out to their own selector and moved to be beside the "#color-scheme-buttons > button" selectors. Having a selector like #color-scheme-buttons > .dark-button" would avoid the need for !important, and make it more obvious what it's doing. @@ +352,5 @@ > } > > #color-scheme-buttons > button { > width: 33.33%; > + border: 0; There's a selector at line 344 that sets top/right/bottom border to 0 for these, which is now redundant. @@ +367,5 @@ > + font-size: 58px; > + height: 100px; > +} > + > +#font-type-buttons > .serif-button { This would benefit from a comment.

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #9) > ::: toolkit/themes/windows/global/aboutReader.css > @@ +22,5 @@ > > .dark-button { > > color: #eeeeee; > > background-color: #333333; > > + margin-top: -1px; > > + height: 61px !important; > > This affects both the button and the page content. And !important is > codesmell. > > I'd rather see these additions split out to their own selector and moved to > be beside the "#color-scheme-buttons > button" selectors. Having a selector > like #color-scheme-buttons > .dark-button" would avoid the need for > !important, and make it more obvious what it's doing. Good call. Changed! > @@ +352,5 @@ > > #color-scheme-buttons > button { > > width: 33.33%; > > + border: 0; > There's a selector at line 344 that sets top/right/bottom border to 0 for > these, which is now redundant. That selector also affected the other buttons, so I've moved this line up to that block, since the border-left gets overridden in the other buttons. > @@ +367,5 @@ > > +#font-type-buttons > .serif-button { > This would benefit from a comment. Added! Thanks for the review! :)

Needs rebasing.

Here you go!

(Did I get the flags right, Ryan?)

Hi Blake, can you provide a point value.

Blake, I guess you want this to be uplifted to 38, right? if it is the case, could you fill the uplift request, thanks.

Hi Sylvestre! I normally leave these to Jaws, who has been doing a lot of work on uplifts for reader-mode/reading-list, but here's the request details… [User impact if declined]: Users will see an uglier font-panel. [Describe test coverage new/current, TBPL]: Manual testing, and has been on mozilla-central for a day. [Risks and why]: Low risk because its a small CSS change impacting only the reader-mode settings popup. [String/UUID change made/needed]: none Do I also need to ask for uplift to 39, or is that implied by tracking-38?

Comment on attachment 8586175 [details] [diff] [review] Rebased patch. (I think bwinton meant to flip these flags)

Comment on attachment 8586175 [details] [diff] [review] Rebased patch. Should be in 38 beta 2