Closed Bug 1158289 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

38 Branch
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: mmaslaney, Assigned: bwinton)

References

Details

Attachments

(1 file, 1 obsolete file)

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/
Flags: qe-verify?
Flags: firefox-backlog+
Target Milestone: Firefox 38 → Firefox 40
Attached patch WIP, showing proof of concept. (obsolete) — Splinter Review
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
Attachment #8598880 - Flags: ui-review?(mmaslaney)
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 2
Flags: qe-verify? → qe-verify-
Attachment #8598880 - Flags: ui-review?(mmaslaney) → ui-review+
Attached patch bug1158289.diffSplinter Review
Okay, since Michael is happy with this, let's send it over to code review.  :)
Attachment #8598880 - Attachment is obsolete: true
Attachment #8600603 - Flags: ui-review+
Attachment #8600603 - Flags: review?(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.
Attachment #8600603 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
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.
Attachment #8600603 - Flags: approval-mozilla-beta?
Attachment #8600603 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/51177c126be9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(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.)
Flags: needinfo?(bwinton)
The width should maintain it's 660px max-height, with a min height of 300px for smaller windows.
Flags: needinfo?(mmaslaney)
Regarding our default size, which I'm assuming we may need to enlarge.
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 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?
Flags: needinfo?(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.  ;)
Flags: needinfo?(bwinton)
Blocks: 1162873
Let us know when it is ready for uplift (not super clear)
(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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(lmandel)
Flags: needinfo?(ryanvm)
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.
Attachment #8600603 - Flags: approval-mozilla-release+
Attachment #8600603 - Flags: approval-mozilla-beta?
Attachment #8600603 - Flags: approval-mozilla-beta+
Attachment #8600603 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.