857989 - Add Serif/Sans Serif font toggle to Reader Mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Lucas Rocha (:lucasr)]

We now ship with Firefox with Charis SIL. We should use make it available as an option in Reader Mode.

Comment 1

[:Margaret Leibovic]

Attachment: WIP

This doesn't actually work right, but I'd like some feedback on the approach :)

I decided to just play around with making another set of buttons similar to the Light/Dark buttons. There are two main problems with this patch.

1) The buttons don't show up right, which suggestions some issue with the .segmented-buttons styles.
2) Toggling the buttons doesn't actually change the font. I assume I'm setting the styles on the wrong thing.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 736333 [details] [diff] [review]
WIP

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

The code looks right to me. There's probably some bug in the styling of segmented buttons, as you mentioned.

::: mobile/android/locales/en-US/chrome/aboutReader.properties
@@ +11,5 @@
>  aboutReader.colorSchemeLight=Light
>  aboutReader.colorSchemeDark=Dark
>  aboutReader.colorSchemeSepia=Sepia
>  
> +aboutReader.fontTypeSansSerif=Sans Serif

Maybe just "Sans" is enough? Also: is this a good for translation? Just wondering.

::: mobile/android/themes/core/aboutReaderContent.css
@@ +16,5 @@
>    color: #eeeeee;
>  }
>  
> +.sans-serif {
> +  font-family: "Open Sans","Droid Sans",sans-serif;

I guess the "Droid Sans" is not really necessary anymore.

@@ +20,5 @@
> +  font-family: "Open Sans","Droid Sans",sans-serif;
> +}
> +
> +.serif {
> +  font-family: "Charis SIL",sans-serif;

sans-serif -> serif

Comment 3

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #2)

> ::: mobile/android/locales/en-US/chrome/aboutReader.properties
> @@ +11,5 @@
> >  aboutReader.colorSchemeLight=Light
> >  aboutReader.colorSchemeDark=Dark
> >  aboutReader.colorSchemeSepia=Sepia
> >  
> > +aboutReader.fontTypeSansSerif=Sans Serif
> 
> Maybe just "Sans" is enough? Also: is this a good for translation? Just
> wondering.

Yeah, I was wondering about what strings would be good here. I imagine other languages have terms for Serif/Sans Serif, but it would be interesting to see what those terms are. I also wonder if the average English speaker even knows these English terms. I actually just looked at Pocket to see what they do, and they just have a font icon that acts as a toggle between Serif/Sans, so maybe something like that would be more user friendly.

Ian, have you thought about what you'd want this to look like?

Comment 4

[:Margaret Leibovic]

Attachment: screenshot

Ian, what do you think of this approach?

Comment 5

[Ian Barlow (:ibarlow)]

That'll do for now I think, yeah.

I'd like to make some adjustments to our Serif layout style, but I can file a separate bug for that. I may do the same for the Overall menu panel styling, but go ahead and land this for now.

Comment 6

[:Margaret Leibovic]

Attachment: patch

Asking for review, assuming Ian doesn't have too many problems with this :)

It turns out the button style issue had to do with the float rule on the <hr>. I also found that I needed to apply the font styles to aboutReader.css, not aboutReaderContent.css. Maybe I'm missing something, but I'm having a tough time finding where aboutReaderContent.html is used. Is it still used?

Comment 7

[:Margaret Leibovic]

Attachment: patch

Oops, left in some debug code.

Comment 8

[:Margaret Leibovic]

Attachment: patch v2

Sorry for the churn, I didn't realize that my last patch actually messed up the styling of the second <hr>. This does a better job.

Comment 9

[:Margaret Leibovic]

Attachment: screenshot v2

Comment 10

[:Margaret Leibovic]

Comment on attachment 737698 [details]
screenshot v2

Oops, missed Ian's comment earlier. Thanks!

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 737696 [details] [diff] [review]
patch v2

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

Looks great. Please file a follow-up for the in-content restyling for serif fonts that ibarlow mentioned.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13063c6cfcd6

Filed bug 862445 as the follow-up.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13063c6cfcd6

Comment 14

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a1(2013-04-17)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.