Closed Bug 999069 Opened 6 years ago Closed 6 years ago

Use clear sans for about: pages

Categories

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

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: Margaret, Assigned: marcosadp)

Details

Attachments

(3 files, 1 obsolete file)

We need to audit the fonts we're currently using, but it looks like most of our font rules are for Roboto, followed by Droid Sans.
Examples of the Clear Sans font can be found here: http://antlam.github.io/fx-branded/fonts.html
I'm ready to take on this bug now that I was finally able to successfully build and run fennec. What is required to address this bug?
Flags: needinfo?(margaret.leibovic)
(In reply to Marcos A. Di Pietro from comment #2)
> I'm ready to take on this bug now that I was finally able to successfully
> build and run fennec. What is required to address this bug?

Awesome! You'll need to look at the font declarations for our about: pages and update them to use Clear Sans instead of Roboto or Droid Sans, which is what we currently use in a lot of these pages.

I just did this quick MXR search to find various font declarations in our product:
http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=font-family%3A

From this list, it looks like you'll want to update:
about.css
aboutBase.css
aboutPage.css
config.css
aboutReader.css
aboutFeedback.css
aboutPrivateBrowsing.css

I think that we should just update these rules to be:
font-family: "Clear Sans",sans-serif;

We ship Clear Sans, so it should always be available, but it's good practice to have a default fallback.
Assignee: margaret.leibovic → marcosadp
Flags: needinfo?(margaret.leibovic)
Thanks!  That clears it pretty much for me.  Wasn't sure if I had to add to the list or replace the existing once.  Should be back with a patch pretty soon.
(In reply to Marcos A. Di Pietro from comment #4)
> Thanks!  That clears it pretty much for me.  Wasn't sure if I had to add to
> the list or replace the existing once.  Should be back with a patch pretty
> soon.

Awesome! On IRC, kbrosnan also suggested that we could just use sans-serif, since Clear Sans is the default sans-serif font that we ship.

We'll also need to make sure to test all these pages to make sure they look good.
So, which approach should I take?  The one suggested by you or the one kbrosnan suggested?  I would also need your help identifying the pages that need to be looked at after making these changes.
(In reply to Marcos A. Di Pietro from comment #6)
> So, which approach should I take?  The one suggested by you or the one
> kbrosnan suggested?  I would also need your help identifying the pages that
> need to be looked at after making these changes.

Thinking about this more, I think we should be explicit and use "Clear Sans", since these are pages that we want to match a certain design standard.

I can help you find the pages you need to test. Most of these file names give good hints as to what pages they're used on (e.g. aboutFeedback.css is used for about:feedback), but you should grep through the code to find the the HTML files where these CSS files are included. For example, aboutBase.css is included in multiple HTML files.

Figuring out how to load those HTML files can be confusing, since we use the AboutRedirector component to make those magical about: URLs load certain HTML pages. You can see those here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#9

You can also directly load these HTML pages using chrome:// URLs, such as chrome://browser/content/about.xhtml.
Attached file Screenshots
Here are the screenshots of the pages touched by the changes in this patch.  I can barely make out the difference.  But again, I don't have a trained eye for typography.  The attachment has a index.html that shows a side (old and new) by side of three of the pages.  And one can appreciate in the about screenshots that the Q belonging to FAQ is slightly different.
Attached patch patch.diff (obsolete) — Splinter Review
Changes font in about stylesheets.
Attachment #8410740 - Flags: review?(margaret.leibovic)
Comment on attachment 8410740 [details] [diff] [review]
patch.diff

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

Thanks so much for the patch, and for the very through testing! I agree some of them look the same, and that may actually be us already falling back to the sans-serif rule for fonts that aren't supported (e.g. about:config uses Open Sans right now, but I don't think we ship that anymore).

I just had one comment to address, then I'll give this an r+ and land it!

::: mobile/android/themes/core/aboutBase.css
@@ +6,4 @@
>  %include defines.inc
>  
>  html {
> +  font-family: "Clear Sans",helvetica,arial,clean,sans-serif;

Let's get rid of the helvetical,arial,clean in the middle, and just make "Clear Sans" and sans-serif the only options. The same applies to all the other font styles.
Attachment #8410740 - Flags: review?(margaret.leibovic) → feedback+
Attached patch patch_v2.diffSplinter Review
Attachment #8411031 - Flags: review?(margaret.leibovic)
Comment on attachment 8411031 [details] [diff] [review]
patch_v2.diff

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

Excellent, thank you!
Attachment #8411031 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Awesome!  Should you need help with another bug, just let me know.
This has major conflicts on fx-team. Please rebase.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> This has major conflicts on fx-team. Please rebase.

Oh, it looks like the patch isn't formatted properly. Splinter says it's a "Windows Patch", so it probably has the wrong line endings. Marcos, could you try changing your diff settings to generate a Unix-formatted diff?

This MDN article has some example .hgrc settings, maybe those will help:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attached patch patch_v3.diffSplinter Review
Unix-formatted diff
Attachment #8411945 - Flags: review?(margaret.leibovic)
Comment on attachment 8411945 [details] [diff] [review]
patch_v3.diff

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

This applies for me, thanks. I'll just land it myself.
Attachment #8411945 - Flags: review?(margaret.leibovic) → review+
Thanks, again!
https://hg.mozilla.org/integration/fx-team/rev/09e329985ca2

I'll keep an eye out for more good bugs for you, but as I mentioned before, this is a good list to look for bugs:
http://www.joshmatthews.net/bugsahoy/?mobile=1
https://hg.mozilla.org/mozilla-central/rev/09e329985ca2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Verified as fixed on Alcatel One Touch 8008D(Android 4.1.2), on Nightly 32.0a1(2014-05-08).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.