871524 - Fine tune Reader Mode margins for tablet landscape mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: 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)]

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]

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]

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)]

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]

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]

Is 640px min-device-width a good media query to heuristically determine if the device is a tablet?

Comment 7

[Lucas Rocha (:lucasr)]

(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)]

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]

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)]

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]

OK. I'll wait for someone to weigh in on this.

Comment 12

[Lucas Rocha (:lucasr)]

(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)]

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]

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]

Any news on how to proceed with this bug?

Comment 16

[:Margaret Leibovic]

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)]

Thanks Margaret, yes that was more or less what I was getting at.

Comment 18

[Marcos A. Di Pietro]

Attachment: 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]

Attachment: 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)]

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]

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)]

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]

Attachment: 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]

Attachment: 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]

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]

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]

(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]

Attachment: Screenshots patch v3

Comment 29

[Marcos A. Di Pietro]

Attachment: Patch v3

Addresses Margaret's suggestions.

Comment 30

[:Margaret Leibovic]

Comment on attachment 759288 [details] [diff] [review]
Patch v3

This looks great, thanks for addressing my feedback.

Comment 31

[Ian Barlow (:ibarlow)]

Screenshots look great, thanks for posting them Marcos!

Comment 32

[Marcos A. Di Pietro]

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?)]

(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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ae1381518a

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9ae1381518a