Closed Bug 1139174 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Reader Mode, defect, P2)

38 Branch
x86
All
defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mmaslaney, Assigned: designakt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=css])

Attachments

(1 file, 2 obsolete files)

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;
No longer depends on: 1136419
Component: General → Reader Mode
Product: Firefox → Toolkit
Summary: Ship Georgia and Helvetica/Arial as Reader View Fonts until Fira and Charis land → Use Georgia and Helvetica/Arial as Reader View Fonts until Fira and Charis land
Whiteboard: [Reader:P2]
Target Milestone: Firefox 38 → ---
Priority: -- → P2
Whiteboard: [Reader:P2]
Duplicate of this bug: 1120518
Flags: qe-verify-
Whiteboard: [lang=css]
Attached patch readerFontUpdate.diff (obsolete) — Splinter Review
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;
}
Attachment #8579490 - Flags: review?(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.
Attachment #8579490 - Flags: review?(margaret.leibovic) → feedback+
Assignee: nobody → mjaritz
Attached file aboutReader.css (obsolete) —
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.
Attachment #8579490 - Attachment is obsolete: true
Attachment #8579710 - Flags: review?(margaret.leibovic)
Thank you for the comments. Included .domain and left .sans-serif-button
Attached patch Updated patchSplinter Review
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
Attachment #8579710 - Attachment is obsolete: true
Attachment #8579710 - Flags: review?(margaret.leibovic)
Attachment #8579748 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e2116c4bcba6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Hi Markus, can you provide a point value.
Iteration: --- → 39.2 - 23 Mar
Flags: needinfo?(mjaritz)
Flags: firefox-backlog+
Blocks: 1132074
Points: --- → 2
Flags: needinfo?(mjaritz)
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
Attachment #8579748 - Flags: approval-mozilla-beta?
Comment on attachment 8579748 [details] [diff] [review]
Updated patch

Should be in 38 beta 2
Attachment #8579748 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.