1142298 - Fix reader view font/color control glitches

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Peter deHaan [:pdehaan]]

Attachment: Screen Shot 2015-03-11 at 3.25.05 PM.png

"Properly" is probably a bit subjective, but hey...

Maybe related to Bug 1132659, but I noticed that the "Light" button doesn't have a rounded lower-left corner, so the shadow looks wrong, plus there is a slight gap on the right edge of the Sepia button and the edge of the doorhanger UI.

You can also see a verrrryyy slight 1px gap in the vertical line at the bottom of the Serif vs Sans-serif buttons.

Comment 1

[Peter deHaan [:pdehaan]]

Attachment: Screen_Shot_2015-03-11_at_3_41_53_PM.png

Comment 2

[Justin Dolske [:Dolske]]

Expanding scope a little.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Roughly in order of the patch:

* Remove bottom border, since it looks weird with the color-control buttons. (And the button/underline will always contrast with the page, so the boundary is always clear.)

* Set 3px border-radius on some first/last buttons to avoid stomping on the panel's own rounded corners. (background-clip doesn't work right here)

* The white glitch at the right of the "Sepia" button in attachment 8576301 [details] was because we only set the width to 33%. So the 3 buttons are only 99.0% of the width, not 100%!

* Change the orange underline to a box-shadow instead of bottom-border. The attachment 8576302 [details] glitch is because it's a CSS mitered-corner with the bottom and side borders (the gray line between buttons).

* Added a hover effect to the font selector.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8579682 [details] [diff] [review]
Patch v.1

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +354,2 @@
>    font-size: 14px;
> +  border-bottom: 0px;

Err, that border-bottom isn't needed.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8579682 [details] [diff] [review]
Patch v.1

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +349,5 @@
>    background-color: transparent;
>  }
>  
>  #color-scheme-buttons > button {
> +  width: 33.33%;

Part of me wants to just use flexbox here. Bad idea?

Comment 6

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/3ed442e5cbbf

Comment 7

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

(In reply to :Gijs Kruitbosch from comment #5)
> Part of me wants to just use flexbox here. Bad idea?

Sounds like a good idea to me.

Comment 8

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Part of me wants to just use flexbox here. Bad idea?
> 
> Sounds like a good idea to me.

When working on this originally, I remember that using flex led to some of the vertical borders not aligning properly. But maybe that only matters for the font/size options, not the color options.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Marco Mucci [:MarcoM]]

Hi Dolske, can you provide a point value.

Comment 11

[Andrei Vaida [:avaida]]

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

I didn't notice any issues with the current UI [1] of the type controls panel on this build. There's still a slight vertical alignment difference between "Sans-serif" and "Serif", but I think that's expected since the font types themselves are different. Do let me know if you think otherwise.

[1] http://i.imgur.com/eeguiNf.png

Comment 13

[:Margaret Leibovic]

Comment on attachment 8579682 [details] [diff] [review]
Patch v.1

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: reader view font style popup will be ugly
[Describe test coverage new/current, TreeHerder]: landed on nightly/aurora
[Risks and why]: low-risk, small CSS changes
[String/UUID change made/needed]: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c0496dd61e60

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 16

[:Gijs (he/him)]

Attachment: fix CSS issue in aboutReader.css,

Untested, but this should work, I think? Blake, can you check?

Comment 17

[:Gijs (he/him)]

Comment on attachment 8592411 [details] [diff] [review]
fix CSS issue in aboutReader.css,

Egh, wrong bug.