1158289 - Optimize Reader View's line length to have between 45 and 75 characters

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Line length/font size info:
General wisdom is that 45-75 characters per line is ideal — suggested by Elements of Typographic Style and many other solid resources.

This writing goes into explaining both font size proportionally and line width:
http://typecast.com/blog/a-more-modern-scale-for-web-typography

We utilize this bookmarklet for testing:
https://css-tricks.com/bookmarklet-colorize-text-45-75-characters-line-length-testing/

Comment 1

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

Attachment: WIP, showing proof of concept.

So, this is a terrible patch that shouldn't be checked in, but let me know how you like the behaviour, and I can make a version that doesn't have the terrible bits.  ;)
There's an OS X build up to play with at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Width-v1.dmg

Comment 2

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

Attachment: bug1158289.diff

Okay, since Michael is happy with this, let's send it over to code review.  :)

Comment 3

[:Margaret Leibovic]

Comment on attachment 8600603 [details] [diff] [review]
bug1158289.diff

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

::: toolkit/themes/shared/aboutReader.css
@@ +53,5 @@
>  }
>  
> +#container {
> +  max-width: 30em;
> +  margin: 0 auto;

Note for posterity: these styles were probably placed on the body originally because we used to not have a #container element. Since this really only needs to apply to the content of the article (not the controls), it's fine to put them on #container.

Comment 4

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

Comment on attachment 8600603 [details] [diff] [review]
bug1158289.diff

Approval Request Comment
[Feature/regressing bug #]: reading mode
[User impact if declined]: With large or small font sizes, we'll show too few or too many characters per line.
[Describe test coverage new/current, TreeHerder]: Manual
[Risks and why]: Low, css-only change.
[String/UUID change made/needed]: None.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/51177c126be9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/51177c126be9

Comment 7

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/51177c126be9

Comment 8

[:Gijs (he/him)]

This makes the width of the article really really narrow on desktop - there is almost twice as much as whitespace as there is readable content width: http://imgur.com/p3pDyel . This ends up looking really wrong. Is this really the intent here?

Comment 9

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

I'mma let Michael answer this one, since he's the designer and all…
(But I will note that once we have a reading list sidebar, it won't be as out of proportion.)

Comment 10

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

The width should maintain it's 660px max-height, with a min height of 300px for smaller windows.

Comment 11

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

Regarding our default size, which I'm assuming we may need to enlarge.

Comment 12

[:Gijs (he/him)]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #10)
> The width should maintain it's 660px max-height, with a min height of 300px
> for smaller windows.

660px is about double what 30em is for most default font sizes. Should we reopen this?

Comment 13

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

I'ld be happy with either re-opening or filing a new bug and assigning it to me.
Although I'ld like to hear whether we want 45-75 characters _or_ 660px, because we can't have both at all font sizes…
(I mean, we could with crazy letter-spacing, but I don't think we want that.  ;)

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Let us know when it is ready for uplift (not super clear)

Comment 15

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] -- PTO May 9=>16 from comment #14)
> Let us know when it is ready for uplift (not super clear)

This is ready now. I filed bug 1162873 which got closed as wontfix, so we should just uplift this to 38.0.5 and later.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8600603 [details] [diff] [review]
bug1158289.diff

Aurora is now 40 so approval is not required. I cleared the Aurora request.

This change needs to land on m-r (38.0.5) and m-b (39). Approved for both.

Comment 17

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a65c4683eebc
https://hg.mozilla.org/releases/mozilla-release/rev/5fff1e20ed9c