Closed Bug 984087 Opened 6 years ago Closed 5 years ago

Thai Fonts size needs adjusting

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: arky, Assigned: jfkthame)

Details

Attachments

(3 files)

Enabling Khmer locale on FxOS Peak increases all font sizes. See attached screenshot 

Device: Firefox OS Peak 
Image: March 16 Geeksphone Nightly
Created a video that reproduces the bug:

https://www.youtube.com/watch?v=iUzuBDbty9I

Test device: Peak running geeksphone nightly (1.5)
Sorry I think this language is 'Thai' not Khmer as originally reported. Thanks @flod for catching that.
Summary: Khmer Fonts size needs adjusting → Thai Fonts size needs adjusting
Jonathan, Any idea how we can fix this problem?
Flags: needinfo?(jfkthame)
(In reply to arky [:arky] from comment #3)
> Jonathan, Any idea how we can fix this problem?

Not offhand - I'm not sure where the UI font sizes in Gaia are ultimately specified, or why they'd all change due to a locale change.

One possibility: does the font.minimum-size.<script> pref from the selected system locale have an effect here? AFAIK, most scripts don't have a minimum specified, but Thai does. I don't know how that relates to the sizes Gaia is using, but perhaps it's too big?
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> One possibility: does the font.minimum-size.<script> pref from the selected
> system locale have an effect here? AFAIK, most scripts don't have a minimum
> specified, but Thai does. I don't know how that relates to the sizes Gaia is
> using, but perhaps it's too big?

Great! We could test this on both keon and Flames at upcoming Firefox OS Thai L10N sprint.
FWIW, I tried flashing my Peak with a new build from Geeksphone, but I don't see Thai in the list of languages, and so I can't currently reproduce this behavior.
Just used the new noto thai font on Flame. That doesn't solve the problem. How/where do I change the font.minimum-size.<script> pref?
Flags: needinfo?(jfkthame)
Initial defaults are set in modules/libpref/init/all.js, unless the FxOS build overrides this somewhere (not that I'm aware of). I don't know whether you can modify that in some way directly on the device, or if you'd need to do a new build in order to change the setting. Probably someone in #b2g would know...
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> Initial defaults are set in modules/libpref/init/all.js, unless the FxOS
> build overrides this somewhere (not that I'm aware of). I don't know whether
> you can modify that in some way directly on the device, or if you'd need to
> do a new build in order to change the setting. Probably someone in #b2g
> would know...

Thanks, Am going to corner someone in #b2g and post to the b2g mailing-list as well. 

BTW I can't spot any definitions in all.js.  
https://github.com/mozilla/gecko-dev/blob/master/modules/libpref/init/all.js

Here is my build.props from the Flame running 2.x 

http://pastebin.mozilla.org/6555717
(In reply to arky [:arky] from comment #9)
> BTW I can't spot any definitions in all.js.  
> https://github.com/mozilla/gecko-dev/blob/master/modules/libpref/init/all.js

No? I see   pref("font.minimum-size.th", 13);   at:

  https://github.com/mozilla/gecko-dev/blob/master/modules/libpref/init/all.js#L3387

which I think would go into the FxOS build, afaict. I don't know what sizes Gaia is actually using, but given that most other scripts don't declare a minimum-size, it seems worth checking on this at least.
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> > BTW I can't spot any definitions in all.js.  
> > https://github.com/mozilla/gecko-dev/blob/master/modules/libpref/init/all.js
> 
> No? I see   pref("font.minimum-size.th", 13);   at:
> 

My bad! Am going to dig deeper. Might have to get a bigger disk and do some try builds with various options.
An update. After half a dozen Flame-KK builds with different font.minimum-size.th didn't make any difference. 

Also noticed that 'Greek (el)' also changes the character width of other fonts. Can someone confirm that ?
An update. After half a dozen Flame-KK builds with different font.minimum-size.th didn't make any difference. 

Also noticed that enabling 'Greek (el)' on Flame-kk master builds also changes the character width of other fonts. Can someone confirm that ?
Could be that your change isn't being picked up in your build for some reason.

Mwu might know, since he's been wrangling fonts-on-device for our custom fonts, etc. Ni? to him.
Flags: needinfo?(mwu)
Are you sure your changes were picked up? There's two lines in all.js that specify font.minimum-size.th. Make sure they're all set. Also as an alternative to rebuilding everything, you can override any pref by putting the pref in /system/b2g/defaults/pref/. You can put the pref in any file in that directory as long as the name ends in ".js". See other pref files in there if you need an example.
Flags: needinfo?(mwu)
Thank you Dietrich, Michael. Am going to recheck the builds and try the alternative solution.
Visual comparison of Thai minimum size settings from 10 to 13. The test was done on Flame(kk) running latest development builds. 

The pref("font.minimum-size.th", 10); settings seems to be the best.
Attachment #8504667 - Flags: review?(jfkthame)
OK, so it looks like the minimum-size pref is indeed what's affecting Gaia font sizes. I wonder whether we should simply not impose a minimum for Thai, at least on B2G; other scripts don't use this, and I don't see any reason Thai should be so special. (At a wild guess, it may be a legacy of what fonts were available on old Windows systems, or something like that...)
Attachment #8504667 - Flags: review?(jfkthame)
I suspect this setting was just cargo-culted from old Windows or Linux code, and has never really made sense on mobile; I suggest we simply remove it.
Attachment #8504741 - Flags: review?(padamczyk)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8504741 [details] [diff] [review]
Remove inappropriate font.minimum-size.th setting for Android/B2G platforms.

Looks good to me.
Attachment #8504741 - Flags: review?(padamczyk) → review+
https://hg.mozilla.org/mozilla-central/rev/1d5c4859fa41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Test the nightly build. Confirm this fixes the problem. Thanks!
You need to log in before you can comment on or make changes to this bug.