993353 - [Tarako] Change default Bengali/Devanagari/Tamil fonts to Noto Sans

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Luke Chang [:lchang] (inactive)]

In Bug 987496, we've made a Hindi language input method.  However, the current default Devanagari (the writing system of Hindi language) font in B2G seems to be inaccuracy. Since Hindi/Tamil/Bengali languages are requirements of v1.3T, I propose replace their default fonts with Noto Sans [1], which are default fonts already used in Android 4.4.

[1] http://www.google.com/fonts/specimen/Noto+Sans

Comment 1

[Michael Wu [:mwu]]

We've already switched to Noto for Bengali fonts in bug 967177 - so you should request blocking on that bug for that particular language.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

Luke, if you can confirm bug 967177 can resolve this bug, can you please 1.3T? that bug and we can dup this bug over. thanks

Comment 3

[Jonathan Kew [:jfkthame]]

Bug 967177 changed the font used for Bengali only, so it doesn't address the issue reported with displaying the Devanagari (Hindi) keyboard.

Comment 4

[Luke Chang [:lchang] (inactive)]

Yes, bug 967177 has already addressed the Bengali font issue so let's change this title and focus on Devanagari and Tamil. I will also set 1.3T? on bug 967177.

Comment 5

[Luke Chang [:lchang] (inactive)]

Attachment: Lohit v.s. Noto Sans

It's the difference of some characters between Lohit font and Noto Sans font.

Comment 6

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Pull request for v1.3t

Luke, Can you help me try this patch and give me a feedback? Thanks!

Comment 7

[Kai-Zhen Li [:seinlin][:kli]]

After these fonts are replaced, the size of system.img increases slightly. I think the reason is 3 Lohit fonts were replaced by 6 Noto fonts.

before:    
 99299904 Apr  9 19:57 out/target/product/sp6821a_gonk/system.img
after:
 99500544 Apr  9 23:37 out/target/product/sp6821a_gonk/system.img

Luke, Can you also help me to confirm if we need both of Bold and Regular for each of them? Thanks!

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

are we licensed to use this font?

Comment 9

[Stephany Wilkes]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

Flagging Patryk on ui-review? as this is a font issue.

Comment 10

[Patryk Adamczyk [:patryk] UX]

It would be good to provide a before and after screenshot. But overall if we are able to use this font under its existing license, I am okay with it. We are using Android's fonts as a fall back if Fira doesn't support a language already.

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

ni? Steven to confirm comment 8

Comment 12

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Lohit vs Noto fonts

Patryk, Before and After screenshots are upates. These fonts are under Apache License, version 2.0, I think we can use it.

Comment 13

[Luke Chang [:lchang] (inactive)]

Agree with Kai-Zhen. We've already used two Noto Sans fonts in master branch.

Comment 14

[Luke Chang [:lchang] (inactive)]

I noticed that we're not using the compressed version of Bengali font in master branch but I guess it is important for Tarako. 

Since Bug 987582 has already been landed in 1.3T, we may need file another bug to compress Noto Sans Bengali font after Bug 967177 is uplifted. Otherwise, I suggest we can replace all the necessary fonts of these three languages (including Bengali, just as the original title and Kai-Zhen's patch) at once in this bug and cancel the "1.3T+" flag in Bug 967177 as we don't need that anymore.

Joe, how do you think?

Comment 15

[Luke Chang [:lchang] (inactive)]

(In reply to Luke Chang [:lchang] from comment #14)
> Otherwise, I suggest we can replace all the necessary fonts of these three
> languages (including Bengali, just as the original title and Kai-Zhen's
> patch) at once in this bug

It's base on this bug will only be patched in v1.3t branch.

Comment 16

[Luke Chang [:lchang] (inactive)]

(In reply to Kai-Zhen Li from comment #7)
> After these fonts are replaced, the size of system.img increases slightly. I
> think the reason is 3 Lohit fonts were replaced by 6 Noto fonts.
> 
> before:    
>  99299904 Apr  9 19:57 out/target/product/sp6821a_gonk/system.img
> after:
>  99500544 Apr  9 23:37 out/target/product/sp6821a_gonk/system.img
> 
> Luke, Can you also help me to confirm if we need both of Bold and Regular
> for each of them? Thanks!

In general, using Bold-specific font will be much more beautiful than using system-simulated one but I'm not sure how the bold font works on FxOS.

Arvin, could you help confirm that? Thanks.

Comment 17

[helloarvin]

Dear Luke,

Frankly speaking, I've no idea about how the bold font works on FxOS as well. However, the current most important thing should be to ensure function well of IME and we could try to further optimize later. How do you think?

Comment 18

[Luke Chang [:lchang] (inactive)]

Actually, IME functions won't be affected no matter the Bold-version fonts exist or not. However, I propose leaving both of Regular and Bold since the increase of size seems acceptable.

Comment 19

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+, this is also partner request

Comment 20

[Luke Chang [:lchang] (inactive)]

Sorry for changing the title again. Let's replace the fonts of three languages on v1.3t.

Comment 21

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

Kai-Zhen, I think this patch can meet the partner's need. Thank you.

Comment 22

[Luke Chang [:lchang] (inactive)]

Kai-Zhen already has a patch.

Comment 23

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

Michael, Can you help me review this? Thanks!

Comment 24

[Kai-Zhen Li [:seinlin][:kli]]

Patryk, Just a soft remind that I already attached the screenshot files. Thanks!

Comment 25

[Axel Hecht [:Pike]]

Johnathan, I'm just wondering, do we need changes to all.js to pick up those changed fonts, or does this work through the magic "I'll use any font I can, and this is the only one that claims to work"?

Comment 26

[Patryk Adamczyk [:patryk] UX]

Great ship it. The Noto fonts are much thicker and easier to read.

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to Axel Hecht [:Pike] from comment #25)
> Johnathan, I'm just wondering, do we need changes to all.js to pick up those
> changed fonts, or does this work through the magic "I'll use any font I can,
> and this is the only one that claims to work"?

They should "just work", thanks to the usual fallback behavior.

Providing appropriate font prefs in all.js would be good in principle, although I'm not sure much text would actually hit those, depending on language tagging, etc.

Also, for better fallback efficiency, we really should provide a B2G version of gfxAndroidPlatform::GetCommonFallbackFonts. This currently tries the Droid Sans Devanagari and Tamil font families for the respective scripts, and those aren't present on B2G.

Comment 28

[Michael Wu [:mwu]]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

This looks good, but can you provide a PR against master?

Comment 29

[Axel Hecht [:Pike]]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

Re-requesting the review from mwu, AFAICT, this is taking the Noto content from master and uplifting it to 1.3T. Thus, no PR against master required.

Comment 30

[James Zhang (Spreadtrum)]

(In reply to Axel Hecht [:Pike] from comment #29)
> Comment on attachment 8404051 [details] [review]
> Replaced Bengali/Devanagari/Tamil by Noto fonts.
> 
> Re-requesting the review from mwu, AFAICT, this is taking the Noto content
> from master and uplifting it to 1.3T. Thus, no PR against master required.

And please use .woff file for shrink ROM size.

Comment 31

[Michael Wu [:mwu]]

(In reply to Axel Hecht [:Pike] from comment #29)
> Comment on attachment 8404051 [details] [review]
> Replaced Bengali/Devanagari/Tamil by Noto fonts.
> 
> Re-requesting the review from mwu, AFAICT, this is taking the Noto content
> from master and uplifting it to 1.3T. Thus, no PR against master required.

Nope - this also switches from Devanagari and Tami to the Noto fonts.

Comment 32

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Wu [:mwu] from comment #31)
> 
> Nope - this also switches from Devanagari and Tami to the Noto fonts.

Michael, yes, we are requested to use Noto fonts not only for Bengali but also Devanagari and Tamil. I'll send PR to master later.

Comment 33

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Pull request for master

Michael, this is for master branch. As Bengali is already with Noto, I only replaced Devanagari and Tamil. BTW, I think base of v1.3t is different from master, so we'll still need to use PR 38 for v1.3t.

Comment 34

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to James Zhang from comment #30)
> 
> And please use .woff file for shrink ROM size.

James, both .ttf and .woff are included. By default .woff will be used in v1.3t.

Comment 35

[Kai-Zhen Li [:seinlin][:kli]]

Joe, patch from this bug will also be merged into master, I'll clear tarako_only in Whiteboard.

Comment 36

[Kai-Zhen Li [:seinlin][:kli]]

Patryk, per comment 26, I think it is positive for ui-review, would you mind update ui-review flag in attachment? Thanks!

Comment 37

[Michael Wu [:mwu]]

Comment on attachment 8405834 [details] [review]
Pull request for master

Thanks.

Comment 38

[Michael Wu [:mwu]]

Comment on attachment 8404051 [details] [review]
Pull request for v1.3t

I think comment 26 is enough. Go ahead and land on master and 1.3t. Also set the status-b2g-v1.3T flag on bug 967177 when you land on 1.3t since this PR is also a backport of that.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/moztt/commit/ce95d372e6d285725b96490afdaaf489ad8f9ca9
v1.3t:  https://github.com/mozilla-b2g/moztt/commit/21717069ed0ed7d9337ab9ae0d01c4dfaa757aad

Not sure if there's any desire to get this on v1.4 or not. Please nominate it for approval if yes.

Comment 40

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Pull request for v.14

I think it will be better to get them on v1.4.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): no
String or IDL/UUID changes made by this patch: no

Comment 41

[Michael Wu [:mwu]]

That PR is really messy. Can you cherry pick the commit on master and fix it up to work? 1.4 doesn't have compressed font support, so you'd have to adjust the fonts.mk part to apply.

Comment 42

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Wu [:mwu] from comment #41)
> That PR is really messy. Can you cherry pick the commit on master and fix it
> up to work? 1.4 doesn't have compressed font support, so you'd have to
> adjust the fonts.mk part to apply.

OK, if 1.4 is not going to support woff, I'll update PR to replace Bengali/Devanagari/Tamil simply by Noto fonts.

Comment 43

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Wu [:mwu] from comment #41)
> That PR is really messy. Can you cherry pick the commit on master and fix it
> up to work? 1.4 doesn't have compressed font support, so you'd have to
> adjust the fonts.mk part to apply.

Michael, PR for 1.4 is updated. Could you approve it?

Comment 44

[Kai-Zhen Li [:seinlin][:kli]]

License is clear, need no ni from styang.

Comment 45

[Michael Wu [:mwu]]

(In reply to Kai-Zhen Li from comment #43)
> Michael, PR for 1.4 is updated. Could you approve it?

Looks good. The important part is approval-mozilla-aurora though, which I can't give.

Comment 46

[Kai-Zhen Li [:seinlin][:kli]]

Michael, Thanks! Let's wait the right person to approve it.

Comment 47

[bhavana bajaj [:bajaj]]

(In reply to Kai-Zhen Li from comment #40)
> Created attachment 8405963 [details] [review]
> Pull request for v.14
> 
> I think it will be better to get them on v1.4.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: no
> Testing completed (on m-c, etc.): yes
> Risk to taking this patch (and alternatives if risky): no
> String or IDL/UUID changes made by this patch: no

Can you please follow the guidelines https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments and give more details on the request here.

At this point in the cycle, we've stopped granting approvals for changes to 1.4/aurora unless absolutely critical. So may be the details you provide might help us decided if this is going to be a huge win with very low risk to the overall code-base

Comment 48

[Kai-Zhen Li [:seinlin][:kli]]

Bhavana, Thanks!  Comment is updated as below.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Hindi language input method
User impact if declined: Some script of these languages, Bengali/Devanagari/Tamil, won't render properly
Testing completed (on m-c, etc.): Manual test on v1.3t
Risk to taking this patch (and alternatives if risky): low because these fonts are from aosp kk
String or IDL/UUID changes made by this patch: none

Comment 49

[Kai-Zhen Li [:seinlin][:kli]]

Need to be landed on v1.4. Thanks!

Comment 50

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/moztt/commit/7beaabe1187b688ef560607912577c614a038533