Last Comment Bug 871524 - Fine tune Reader Mode margins for tablet landscape mode
: Fine tune Reader Mode margins for tablet landscape mode
Status: RESOLVED FIXED
[mentor=margaret][lang=css]
:
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 24
Assigned To: Marcos A. Di Pietro
:
: Anthony Lam (:antlam)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-13 06:29 PDT by Ian Barlow (:ibarlow)
Modified: 2013-06-07 12:31 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reference diagram (23.89 KB, image/png)
2013-05-13 06:29 PDT, Ian Barlow (:ibarlow)
no flags Details
Screenshots for suggested patch (483.21 KB, application/octet-stream)
2013-06-05 13:16 PDT, Marcos A. Di Pietro
no flags Details
patch v1 (793 bytes, patch)
2013-06-05 13:20 PDT, Marcos A. Di Pietro
no flags Details | Diff | Splinter Review
Screenshots more margins/small devices (147.39 KB, application/octet-stream)
2013-06-05 16:30 PDT, Marcos A. Di Pietro
no flags Details
Patch v2 (792 bytes, patch)
2013-06-05 16:32 PDT, Marcos A. Di Pietro
margaret.leibovic: review-
Details | Diff | Splinter Review
Screenshots patch v3 (510.35 KB, application/octet-stream)
2013-06-06 11:26 PDT, Marcos A. Di Pietro
no flags Details
Patch v3 (936 bytes, patch)
2013-06-06 11:27 PDT, Marcos A. Di Pietro
margaret.leibovic: review+
Details | Diff | Splinter Review

Description Ian Barlow (:ibarlow) 2013-05-13 06:29:09 PDT
Created attachment 748792 [details]
reference diagram

In landscape Reader mode, the text block tends to run wider than is comfortable for reading. 

Let's adjust it so that on tablets, the landscape text block is 15-20% wider than it is in portrait mode. This will leave larger margins, but should make for a more comfortable measure for reading.
Comment 1 Ian Barlow (:ibarlow) 2013-05-27 07:57:02 PDT
Would be great to get started on this -- we seem to have lost margins altogether in our latest Nightly builds http://cl.ly/image/3M1X171X2t2g
Comment 2 :Margaret Leibovic 2013-05-29 14:52:28 PDT
This should be doable with CSS and media queries. There are already some examples in aboutReader.css:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#583
Comment 3 Marcos A. Di Pietro 2013-05-31 19:22:40 PDT
I'd like to work on this one. However, I'm kind of confused on how to approach this using CSS. According to Ian, the width should be set based on the value of height (=width in portrait). I've looked into -moz-calc and it seems it does not support computing values on other properties. If there's a way of doing this through CSS that I'm unaware of, I'd gladly work on this if you pointed me on the right direction.

Another solution I can think of is to make the assumption, that in landscape mode, screens have a width that's normally around 1.3-1.6 times its height. So, we could generalize this assumption and say, on average the height (or width in portrait) is say 55% the width. So, it should be possible to acquire similar results by assuming the width should be 75% (55% of shrinking landscape width to match portrait width + 20% to make text block wider). Based on that there should be margins of 12.5% on each side. This approach wouldn't yield exact transformations, but I believe could be a viable solution. What do you think?
Comment 4 Wesley Johnston (:wesj) 2013-05-31 19:28:14 PDT
Can you use vh vw (viewport-height and viewport-width) units?

That said, I would be fine with just setting a max-width as well. If you are on a gigantic screen, you would likely still not want content stretching across the entire viewport.
Comment 5 Marcos A. Di Pietro 2013-05-31 20:04:56 PDT
That's really cool! I didn't know about this.
Since the height in landscape is the width in portrait and we want to make landscape width 20% bigger, I would have to set the text block width to 120vh when in landscape.
Now that I got a grasp on this bug's solution, could someone assign it to me?
Comment 6 Marcos A. Di Pietro 2013-06-02 12:32:25 PDT
Is 640px min-device-width a good media query to heuristically determine if the device is a tablet?
Comment 7 Lucas Rocha (:lucasr) 2013-06-03 04:06:57 PDT
(In reply to Marcos A. Di Pietro from comment #6)
> Is 640px min-device-width a good media query to heuristically determine if
> the device is a tablet?

Ian can probably help you here.
Comment 8 Ian Barlow (:ibarlow) 2013-06-03 07:11:40 PDT
I actually don't know the answer to this. Especially given the varying pixel densities of Android tablets, and the fact that Android generally encourages against using "real" pixels to define sizes.
Comment 9 Marcos A. Di Pietro 2013-06-03 08:13:16 PDT
Ian, correct me if I'm wrong, but I believe there isn't a way to deterministically telling a tablet from a phone apart in CSS. Do you think such a rule, or something along that line, could work?

@media screen and 
(orientation: landscape) and (min-device-width: 640px) and (max-device-width: 800px) and (min--moz-device-pixel-ratio: 0.75),
(orientation: landscape) and (min-device-width: 801px) and (max-device-width: 1024px) and (min--moz-device-pixel-ratio: 1.0),
(orientation: landscape) and (min-device-width: 1025px) and (max-device-width: 1280px) and (min--moz-device-pixel-ratio: 1.5),
(orientation: landscape) and (min-device-width: 1281px) and (min--moz-device-pixel-ratio: 2.0) {
    /* CSS Rules here */
}
Comment 10 Ian Barlow (:ibarlow) 2013-06-03 08:16:35 PDT
Marcos, media queries aren't really my area of expertise. It's worth trying, but probably a better question for Lucas or one of the other front end developers.
Comment 11 Marcos A. Di Pietro 2013-06-03 08:21:11 PDT
OK. I'll wait for someone to weigh in on this.
Comment 12 Lucas Rocha (:lucasr) 2013-06-03 08:33:02 PDT
(In reply to Marcos A. Di Pietro from comment #9)
> Ian, correct me if I'm wrong, but I believe there isn't a way to
> deterministically telling a tablet from a phone apart in CSS. Do you think
> such a rule, or something along that line, could work?
> 
> @media screen and 
> (orientation: landscape) and (min-device-width: 640px) and
> (max-device-width: 800px) and (min--moz-device-pixel-ratio: 0.75),
> (orientation: landscape) and (min-device-width: 801px) and
> (max-device-width: 1024px) and (min--moz-device-pixel-ratio: 1.0),
> (orientation: landscape) and (min-device-width: 1025px) and
> (max-device-width: 1280px) and (min--moz-device-pixel-ratio: 1.5),
> (orientation: landscape) and (min-device-width: 1281px) and
> (min--moz-device-pixel-ratio: 2.0) {
>     /* CSS Rules here */
> }

I suggest not thinking in terms of tablets or orientations specifically but more in terms of screen sizes in general. Ian, I suppose that, in practice, what we want here is to set a max text width when screen is large? Or is it more like you want to always have margins with relative size with the screen?
Comment 13 Ian Barlow (:ibarlow) 2013-06-03 08:52:18 PDT
Yeah, this bug was originally really about just making our margins suck less in whatever simple way we can. 

But this media query discussion is getting complicated enough where I am beginning to wonder if we should be thinking ahead a bit. The longer term goal that I'd like to see is for us to adopt smarter rules around making our typography more responsive to whatever screen size it is displayed on. Which then becomes more about defining an appropriate line length for text to make it the most legible (usually around 65 characters or 11-12 English words), and also means that margins should be changing depending on the text size.

I wonder if this is the real problem we should be trying to solve now rather than waiting...
Comment 14 Marcos A. Di Pietro 2013-06-03 12:38:01 PDT
Ian, what about defining small, medium, and large fonts based on viewport size when in landscape mode?
Take a look at http://jsfiddle.net/ehFX4/7/show/
If you resize the viewport, the font will resize accordingly, and each line will keep showing the same content.

Here are some examples how small, medium, and large fonts could look like.
Small: http://jsfiddle.net/ehFX4/6/show/
Medium: http://jsfiddle.net/ehFX4/7/show/
Large: http://jsfiddle.net/ehFX4/8/show/
Comment 15 Marcos A. Di Pietro 2013-06-04 11:49:56 PDT
Any news on how to proceed with this bug?
Comment 16 :Margaret Leibovic 2013-06-04 12:44:22 PDT
I don't think we should be changing the font size based on the viewport size, since the user already has the option to change font sizes in the text style menu.

I think what Ian is suggesting in comment 13 is that we should change the margins based on the font size, such that lines of text are always about 65 characters long. To do this, maybe we can forget about margins and viewport size, and instead put the text in a container with a max-width set in em units:
https://developer.mozilla.org/en-US/docs/Web/CSS/length#Relative_length_units

What do you think of that idea?
Comment 17 Ian Barlow (:ibarlow) 2013-06-04 12:52:16 PDT
Thanks Margaret, yes that was more or less what I was getting at.
Comment 18 Marcos A. Di Pietro 2013-06-05 13:16:12 PDT
Created attachment 758765 [details]
Screenshots for suggested patch

The attachment contains screenshots for Nexus One (portrait and landscape) and Nexus 10 (portrait and landscape) for the suggested patch I'll post next.
Comment 19 Marcos A. Di Pietro 2013-06-05 13:20:17 PDT
Created attachment 758767 [details] [diff] [review]
patch v1

The patch sets text block width to 35em. If there's space for margins, the text block is center-aligned. If the text block overflows the screen, max-width is set to 98vw.
Comment 20 Ian Barlow (:ibarlow) 2013-06-05 13:37:17 PDT
Nice, this is coming along. Marcos, one of the things we'll want to add is a minimum margin -- on phones, this is looking pretty tight to the edge.
Comment 21 Marcos A. Di Pietro 2013-06-05 13:40:24 PDT
Ok, I'll change the max-width to 90vw instead of 98vw. I'm not much of a fonts and text layout guy so I'm not sure this is a good suggestion or not. But do you think it would be good idea to justify the content?
Comment 22 Ian Barlow (:ibarlow) 2013-06-05 13:43:23 PDT
Marcos, not without some kind of hyphenation support. Without it, we'll end up with large gaps in the text block that make legibility worse. Our own Crystal Beasley wrote a good blog post about this. http://skinnywhitegirl.com/blog/web-typography-hyphenation-justification/475/

Even with hyphenation, though, it's often down to user's personal preference so I would want that to be a setting that people could toggle on or off.
Comment 23 Marcos A. Di Pietro 2013-06-05 16:30:02 PDT
Created attachment 758876 [details]
Screenshots more margins/small devices

The attachment contains screenshots for Nexus One (landscape and portrait). The screenshots belong to the patch I'm about to submit.
Comment 24 Marcos A. Di Pietro 2013-06-05 16:32:48 PDT
Created attachment 758879 [details] [diff] [review]
Patch v2

Same as patch v1 (now obsolete) but with smaller max-width to allow wider margins accross devices, including those with low resolution.
Comment 25 :Margaret Leibovic 2013-06-05 17:16:20 PDT
Comment on attachment 758879 [details] [diff] [review]
Patch v2

Although this produces screenshots that seem close to what we want, I don't think this approach is totally correct.

So, there are three things we want:
1) maximum width for the block of text
2) centered block of text
3) minimum margins around the block of text

We already know we can accomplish the first two things with max-width and margin-left/margin-right: auto. To accomplish the third thing, I think we should just try using some padding.

Let's try getting rid of the margin-top/margin-bottom, and replace them with padding: 20px, which will give us some space on all sides of the text.

Also, we shouldn't need a width property. Let's just try max-width: 35em.
Comment 26 Marcos A. Di Pietro 2013-06-05 17:46:27 PDT
Margaret, what would happen if 35em exceeds the viewport's width?
The whole reasoning behind setting max-width to 90% is to having a fallback just in case width: 35em exceeds the viewport width. So, not only does the 90% prevent the text block from overflowing the viewport's width but it also ensures that there will be at least 5% padding on each side.
I'm not sure what the exact behavior of max-width is. Will it grow bigger than the viewport? Or does max-width work in a way were it will not overflow the screen?
Comment 27 :Margaret Leibovic 2013-06-06 09:16:33 PDT
(In reply to Marcos A. Di Pietro from comment #26)
> Margaret, what would happen if 35em exceeds the viewport's width?
> The whole reasoning behind setting max-width to 90% is to having a fallback
> just in case width: 35em exceeds the viewport width. So, not only does the
> 90% prevent the text block from overflowing the viewport's width but it also
> ensures that there will be at least 5% padding on each side.
> I'm not sure what the exact behavior of max-width is. Will it grow bigger
> than the viewport? Or does max-width work in a way were it will not overflow
> the screen?

The point of max-width is just to guarantee that the body is never wider than 35em, we don't mind if it's less wide than that if that's what fills the viewport.

The <meta name="viewport"/> tag is what guarantees that the body will never grow wider than the viewport:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.html?force=1#6

That's why it works fine right now without any width CSS rule on the body.

And setting padding explicitly on the element is a better way to ensure that there is always a minimum amount of padding on each side. I'd prefer to avoid using percentages, since that would make the padding dependent on the device width, which isn't what we want.
Comment 28 Marcos A. Di Pietro 2013-06-06 11:26:30 PDT
Created attachment 759287 [details]
Screenshots patch v3
Comment 29 Marcos A. Di Pietro 2013-06-06 11:27:32 PDT
Created attachment 759288 [details] [diff] [review]
Patch v3

Addresses Margaret's suggestions.
Comment 30 :Margaret Leibovic 2013-06-06 11:31:11 PDT
Comment on attachment 759288 [details] [diff] [review]
Patch v3

This looks great, thanks for addressing my feedback.
Comment 31 Ian Barlow (:ibarlow) 2013-06-06 11:33:55 PDT
Screenshots look great, thanks for posting them Marcos!
Comment 32 Marcos A. Di Pietro 2013-06-06 13:55:21 PDT
Thanks guys for your help. By the way Ian, I read the article about hyphenation support. Support is still not available right?
Anyways, I'd like to work on adding alignment options to the toolbar. It will help me get more familiar with the chrome subfolder. Is this something that would be of any value to the reader?
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2013-06-06 20:59:56 PDT
(In reply to Marcos A. Di Pietro from comment #32)
> Thanks guys for your help. By the way Ian, I read the article about
> hyphenation support. Support is still not available right?

Firefox does support hyphenation
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-06-07 05:42:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ae1381518a
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-06-07 12:31:50 PDT
https://hg.mozilla.org/mozilla-central/rev/c9ae1381518a

Note You need to log in before you can comment on or make changes to this bug.