1148050 - Type Control Visual Improvements

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

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

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.

Comment 1

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

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

Comment 2

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

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.

Comment 3

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

Attachment: 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 )

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?

Comment 4

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

(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 5

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

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

Comment 6

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

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…  ;)

Comment 7

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

Looks great. ;)

Comment 8

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

Attachment: The second cut at the patch.

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

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 10

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

Attachment: The third cut at the patch.

(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!  :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 12

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

Attachment: Rebased patch.

Here you go!

Comment 13

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

(Did I get the flags right, Ryan?)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/049a2ddcc904

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/049a2ddcc904

Comment 16

[Marco Mucci [:MarcoM]]

Hi Blake, can you provide a point value.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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

Comment 18

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

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 19

[:Margaret Leibovic]

Comment on attachment 8586175 [details] [diff] [review]
Rebased patch.

(I think bwinton meant to flip these flags)

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8586175 [details] [diff] [review]
Rebased patch.

Should be in 38 beta 2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5489efc1204

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b6bd63b68e5