Closed Bug 857989 Opened 7 years ago Closed 7 years ago

Add Serif/Sans Serif font toggle to Reader Mode

Categories

(Firefox for Android :: Reader View, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
relnote-firefox --- 23+

People

(Reporter: lucasr, Assigned: Margaret)

References

Details

Attachments

(2 files, 4 obsolete files)

We now ship with Firefox with Charis SIL. We should use make it available as an option in Reader Mode.
Keywords: uiwanted
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → margaret.leibovic
Attachment #736333 - Flags: feedback?(lucasr.at.mozilla)
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
Attachment #736333 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(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?
Flags: needinfo?(ibarlow)
Attached image screenshot (obsolete) —
Ian, what do you think of this approach?
Attachment #737676 - Flags: feedback?(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.
Flags: needinfo?(ibarlow)
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #736333 - Attachment is obsolete: true
Attachment #737684 - Flags: review?(lucasr.at.mozilla)
Attached patch patch (obsolete) — Splinter Review
Oops, left in some debug code.
Attachment #737684 - Attachment is obsolete: true
Attachment #737684 - Flags: review?(lucasr.at.mozilla)
Attachment #737686 - Flags: review?(lucasr.at.mozilla)
Attached patch patch v2Splinter Review
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.
Attachment #737686 - Attachment is obsolete: true
Attachment #737686 - Flags: review?(lucasr.at.mozilla)
Attachment #737696 - Flags: review?(lucasr.at.mozilla)
Attached image screenshot v2
Attachment #737676 - Attachment is obsolete: true
Attachment #737676 - Flags: feedback?(ibarlow)
Attachment #737698 - Flags: feedback?(ibarlow)
Comment on attachment 737698 [details]
screenshot v2

Oops, missed Ian's comment earlier. Thanks!
Attachment #737698 - Flags: feedback?(ibarlow)
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.
Attachment #737696 - Flags: review?(lucasr.at.mozilla) → review+
Depends on: 862445
Keywords: uiwanted
https://hg.mozilla.org/mozilla-central/rev/13063c6cfcd6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Verified fixed on:
-build: Firefox for Android 23.0a1(2013-04-17)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1
Status: RESOLVED → VERIFIED
Blocks: 866766
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.
You need to log in before you can comment on or make changes to this bug.