Closed Bug 1250121 Opened 4 years ago Closed 4 years ago

Regression: Font is not changing in reader view

Categories

(Firefox for Android :: Reader View, defect)

47 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- verified
firefox48 --- verified

People

(Reporter: TeoVermesan, Assigned: sebastian)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
This sounds like an issue with downloadable fonts. Might be a dupe of another bug.
Flags: needinfo?(s.kaspari)
Yeah, this seems to be caused by bug 1249354.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Depends on: 1249354
Flags: needinfo?(s.kaspari)
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.
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/
Attachment #8728386 - Flags: review?(margaret.leibovic)
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 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?
Attachment #8728386 - Flags: review?(margaret.leibovic)
Attachment #8728386 - Flags: review?(jfkthame)
Attachment #8728386 - Flags: feedback+
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?
Attachment #8728386 - Flags: review?(jfkthame) → review+
(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 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]: -
Attachment #8728386 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ce916d9b2669
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
Thanks Sebastian. I will take it in Aurora47 in a couple of days if all is well.
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
Attachment #8728386 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6 and Aurora - 48.0a2(2016-05-15)
I'll mark this bug as verified fixed since it is verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.