Closed Bug 1142298 Opened 5 years ago Closed 5 years ago

Fix reader view font/color control glitches

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: pdehaan, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

"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.
Flags: qe-verify+
Expanding scope a little.
Summary: Light/Dark/Sepia controls in reader view aren't aligned properly → Fix reader view font/color glitches
Summary: Fix reader view font/color glitches → Fix reader view font/color control glitches
Attached patch Patch v.1Splinter Review
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.
Assignee: nobody → dolske
Attachment #8579682 - Flags: review?(gijskruitbosch+bugs)
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 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?
Attachment #8579682 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/3ed442e5cbbf
OS: Mac OS X → All
Hardware: x86 → All
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/3ed442e5cbbf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Iteration: --- → 39.2 - 23 Mar
Flags: firefox-backlog+
Blocks: 1132074
Hi Dolske, can you provide a point value.
Flags: needinfo?(dolske)
Points: --- → 2
Flags: needinfo?(dolske)
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
Status: RESOLVED → VERIFIED
QA Contact: andrei.vaida
Duplicate of this bug: 1137217
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
Attachment #8579682 - Flags: approval-mozilla-beta?
Attachment #8579682 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Untested, but this should work, I think? Blake, can you check?
Attachment #8592411 - Flags: review?(bwinton)
Comment on attachment 8592411 [details] [diff] [review]
fix CSS issue in aboutReader.css,

Egh, wrong bug.
Attachment #8592411 - Attachment is obsolete: true
Attachment #8592411 - Flags: review?(bwinton)
You need to log in before you can comment on or make changes to this bug.