1139174 - Use Georgia and Helvetica/Arial as Reader View Fonts until Fira and Charis land

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

Until Fira and Charis land, let's use Georgia and Helvetica/Arial as our font fall backs.

Order for fonts

San Serif:

font-family: "Fira Sans", Helvetica, Arial, Serif;

Serif:

font-family: "Charis SIL", Georgia, "Times New Roman", serif;

Comment 2

[Markus Jaritz [:designakt] (UX)]

Attachment: readerFontUpdate.diff

How about to repace/add those lines in the aboutReader.css
This will change the default fonts, as well as the font used in the configuration panel.

.sans-serif {
  font-family: "Fira Sans", Helvetica, Arial, sans-serif;
}

.serif {
  font-family: "Charis SIL", Georgia, "Times New Roman", serif;
}

.toolbar {
  font-family: "Fira Sans", Helvetica, Arial, sans-serif;
}

.sans-serif-button {
  font-family: "Fira Sans", Helvetica, Arial, sans-serif;
}

.serif-button {
  font-family: "Charis SIL", Georgia, "Times New Roman", serif;
}

Comment 3

[:Margaret Leibovic]

Comment on attachment 8579490 [details] [diff] [review]
readerFontUpdate.diff

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

Thanks for proposing a patch! This patch doesn't apply to my fx-team tree, I think you may need to update your tree.

This will of course depend on users having these fonts installed on their machine, but I don't see any problem at least trying to use these fonts if they're present.

::: toolkit/themes/windows/global/aboutReader.css
@@ +36,5 @@
>    background-color: #f0ece7;
>  }
>  
>  .sans-serif {
> +  font-family: "Fira Sans", Helvetica, Arial, sans-serif;

We should also update the .domain font-style rule, since that currently uses sans-serif as well.

@@ +413,5 @@
>    background-position: center;
>  }
>  
> +.sans-serif-button {
> +  font-family: "Fira Sans", Helvetica, Arial, sans-serif;

I don't think we need this, since this style will already be applied from the .toolbar style.

@@ +418,5 @@
> +}
> +
> +.serif-button {
> +  font-family: "Charis SIL", Georgia, "Times New Roman", serif;
> +}

You should update the existing .serif-button style, instead of adding this new one.

Comment 4

[Markus Jaritz [:designakt] (UX)]

Attachment: aboutReader.css

I cloned my tree newly but somehow it then only managed to get a diff containing a lot of information. So I can only attach the file itself, based on the tree from 1 hour ago.

Comment 5

[Markus Jaritz [:designakt] (UX)]

Thank you for the comments. Included .domain and left .sans-serif-button

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Updated patch

I updated your patch to the current tree, you might have been seeing conflicts with bug 1142298, which I just landed earlier.

Landed: https://hg.mozilla.org/integration/fx-team/rev/e2116c4bcba6

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Marco Mucci [:MarcoM]]

Hi Markus, can you provide a point value.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8579748 [details] [diff] [review]
Updated patch

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: reader view will use sub-optimal fonts
[Describe test coverage new/current, TreeHerder]: landed on nightly/aurora
[Risks and why]: low-risk, small CSS change
[String/UUID change made/needed]: none

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8579748 [details] [diff] [review]
Updated patch

Should be in 38 beta 2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/079e210207b9