1250121 - Regression: Font is not changing in reader view

Status: VERIFIED FIXED

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

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Go to news.google.com and choose an article
2. Enter reader view
3. Tap on the "Layout options"(Aa) from the toolbar
4. Switch between Sans-serif and Serif font

Expected results:
- The reader mode controls menu is triggered. The text of the page is changing according to the selected font.

Actual results:
- The text of the page is not changing according to the selected font.

Note:
-regression
good build: 17-02
bad build: 18-02
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=15621f98b53b1994c7ae2e2703a6e50203c5304c&tochange=1150ac4755c7bb35df4fc7504f6f1b6c257f400e

Comment 1

[:Margaret Leibovic]

This sounds like an issue with downloadable fonts. Might be a dupe of another bug.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, this seems to be caused by bug 1249354.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1249354 has been fixed and the font should be available eventually.

But there's still the possibility that reader mode is used before the font is downloaded.

Some ideas:
a) Search for the font in all known locations and then let the reader view know about the font's existence. This has the cost of duplicating the logic about where to find fonts.
b) Maybe the nsIFontEnumerator interface can be used to determine if the needed font is available: https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsIFontEnumerator.idl

I wish I could just use some CSS selector magic here to hide the button if the font is not available.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r?margaret

With downloadable fonts (Bug 1194338) our serif font "Charis SIL Compact"
might not be available until the download has been completed successfully.
Our other fallback fonts are not actually serif fonts (sans serif).

Android ships with a serif font called "Noto Serif". This patch adds this
font as fallback.

Review commit: https://reviewboard.mozilla.org/r/38933/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38933/

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

While exploring various implementations I wondered why there's no (serif) system font we use as fallback. Looking into Android's list of system fonts I found "Noto Serif" that is used by Android as font for "serif" and others:
https://github.com/android/platform_frameworks_base/blob/master/data/fonts/system_fonts.xml#L99-L117

Adding this font as fallback solved this problem here. If our "Charis SIL Compact" is not downloaded yet then "Noto Serif" is used.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8728386 [details]
MozReview Request: Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r?margaret

I'm fine with this approach, but we should get review from someone who has a better understanding of these font prefs.

Jonathan, can you take a quick look at this patch?

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8728386 [details]
MozReview Request: Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r?margaret

Seems reasonable to me.

Presumably, if you wait for a while before testing, the issue here doesn't occur because the expected font will have been downloaded and is available? Do we have some kind of check to ensure that's actually working as intended?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Presumably, if you wait for a while before testing, the issue here doesn't
> occur because the expected font will have been downloaded and is available?

Correct. This is to bridge the time until we have downloaded the font.

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Do we have some kind of check to ensure that's actually working as intended?

I did my local tests with the downloader disabled. This "download fonts at runtime" feature is currently only enabled in Nightly to find issues like this.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77eb7ddc538

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/ce916d9b26697a9cb9539562127715053e30184f
Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r=jfkthame

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728386 [details]
MozReview Request: Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r?margaret

Approval Request Comment

[Feature/regressing bug #]: I would like to uplift this in order to enable downloadable fonts in 47 (bug 1254491)

[User impact if declined]: If we enable downloadable fonts (bug 1254491) then users might not be able to enable the "serif font" in reader mode until the font download has been completed successfully in the background.

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: This patch adds one of Android's system fonts to the list of fallback fonts. This being a system font of Android I assume that this font is good for all languages - but this will need testing in the wild. This shouldn't affect most users because we use a different font as first default. This default is the font we download. It's our goal to download the actual font as soon as possible.

[String/UUID change made/needed]: -

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Sebastian, Margaret, given the risk of not knowing that "Noto Serif" is the right choice for Android for all languages and that we need more testing, should we let this bake in Central for a bit before uplifting to Aurora47? On the other hand, we probably have more end-users on Fennec Aurora and uplifting sooner might be better. Please let me know.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Hi Sebastian, Margaret, given the risk of not knowing that "Noto Serif" is
> the right choice for Android for all languages and that we need more
> testing, should we let this bake in Central for a bit before uplifting to
> Aurora47? On the other hand, we probably have more end-users on Fennec
> Aurora and uplifting sooner might be better. Please let me know.

I'm okay with waiting some days and see if this causes any troubles in Nightly. The only reason why I would like to uplift this is because this is a requirement to enable downloadable fonts in Aurora and let it ride the trains (bug 1254491) - but this can easily wait some more days.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Sebastian. I will take it in Aurora47 in a couple of days if all is well.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728386 [details]
MozReview Request: Bug 1250121 - Add "Noto Serif" as fallback "serif" font. r?margaret

This has been in Nightly for a week, seems safe to uplift to Aurora47

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0bff8f758f62

Comment 18

[Alex Caziuc]

Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6 and Aurora - 48.0a2(2016-05-15)

Comment 19

[Alex Caziuc]

I'll mark this bug as verified fixed since it is verified.