1460874 - Expose font size/font inflation settings

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[Jan Henning [:JanH]]

The general font size setting could internally just use our global text zoom setting from https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/mobile/android/base/java/org/mozilla/gecko/GeckoFontScaleListener.java#107.

As for font inflation, I'd make it just a simple on/off pref and then we'd internally calculate the required pref value depending on the general font size setting, similar to https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/mobile/android/base/java/org/mozilla/gecko/GeckoFontScaleListener.java#100.

Comment 1

[Jan Henning [:JanH]]

Since those features (default text zoom and font inflation) apply globally to all pages, would GeckoRuntimeSettings be the place to add them?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jan Henning [:JanH] from comment #1)
> Since those features (default text zoom and font inflation) apply globally
> to all pages, would GeckoRuntimeSettings be the place to add them?

Yup.

Comment 4

[Chris Peterson [:cpeterson]]

Sebastian says system font size support would be a new Focus feature (https://github.com/mozilla-mobile/focus-android/issues/2752), not a Focus+GV regression compared to Focus+WebView.

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Expose font size/inflation options via GeckoRuntimeSettings.

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

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8992213 [details]
Bug 1460874 - Expose font size/inflation options via GeckoRuntimeSettings.

https://reviewboard.mozilla.org/r/257072/#review266198

I think we found that inflation just confused people and broke layouts. I think we should just do the scaling factor. r+ if you remove the inflation bits for now.

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8992213 [details]
Bug 1460874 - Expose font size/inflation options via GeckoRuntimeSettings.

https://reviewboard.mozilla.org/r/257072/#review266198

I could take that out for now, but

> broke layouts

Okay, bug 1475875 *really* should be fixed first to get things working again under e10s (I'll try to do so as soon as I have some more time), and unfortunately bug 1428670 hasn't helped either, and that latter one is appearing slightly more common than some of the other layout issues afflicting our implementation.

> just confused people

Wasn't the confusing bit mainly the way our font size setting had been originally implemented, i.e. it would only control the amount of font inflation, but leave the font size on mobile pages alone? That would no longer have to be the case today. By turning on font inflation and hooking the fontSizeFactor option up to a percentage slider in the preferences you could for example more or less implement the same behaviour as used by Chrome.

Also what do you propose as an alternative? Yes, with font inflation some (but certainly *not* all) non-mobile pages might have more or less subtly broken layouts (and we really should fix at least bug 1428670), but without font inflation **allmost all** of those pages will have unreadably small text, which doesn't really seem the better option, does it?

Plus I'd like to point out once again that Chrome *always* does font inflation (unless you set their font size option to minuscule, but that makes mobile pages unreadably small as well), so without something comparable you'll just get the effect that a page appears readable in Chrome, but not in Firefox.

Comment 8

[Chris Peterson [:cpeterson]]

P3 according to Vesta's list of Fenix requirements

Comment 9

[Chris Peterson [:cpeterson]]

P2 for Fenix

Comment 10

[Jan Henning [:JanH]]

Should the ability to automatically set the amount of font size adjustment depending on the corresponding Android pref (and also to keep tracking that value, which means registering a system preferences listener akin to today's GeckoFontScaleListener) also be a part of the GeckoView API, or should that remain the responsibility of the embedding app (respectively some android-components thingy)?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jan Henning [:JanH] from comment #10)

Should the ability to automatically set the amount of font size adjustment
depending on the corresponding Android pref (and also to keep tracking that
value, which means registering a system preferences listener akin to today's
GeckoFontScaleListener) also be a part of the GeckoView API, or should that
remain the responsibility of the embedding app (respectively some
android-components thingy)?

It seems like we should just have a toggle to use the system font size adjustment in GV. Apps can then choose to expose that to users or just set a default.

Comment 12

[Jan Henning [:JanH]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)

(In reply to Jan Henning [:JanH] from comment #10)

Should the ability to automatically set the amount of font size adjustment
depending on the corresponding Android pref (and also to keep tracking that
value, which means registering a system preferences listener akin to today's
GeckoFontScaleListener) also be a part of the GeckoView API, or should that
remain the responsibility of the embedding app (respectively some
android-components thingy)?

It seems like we should just have a toggle to use the system font size adjustment in GV. Apps can then choose to expose that to users or just set a default.

Right - one reason I also left that bit out in my original attempt was because I had no good idea on how to integrate the GeckoFontScaleListener logic into GeckoView. By now I have learned a little more and I should have something ready soon that includes automatic font size setting if desired by the embedder.

Comment 13

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 1: Cleanups. r?snorp

The GeckoFontScaleListener is intended to live as long as the app (and there-
fore Gecko) remains alive and consequently keeps a reference to the
application context, so the linter warning can be safely suppressed.

Also reorder imports to match coding style.

Comment 14

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 2: Rename instance variables to better match GeckoView code style. r?snorp

Comment 15

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 3: Rename initalize() method for more clarity. r?snorp

Going forward, we want to be able to toggle the enabled state at all times, and
being able to do this before calling initialize() seems a bit inconsistent.
Hence, we rename it to the more neutral-sounding attachToContext().

Comment 16

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 5: Move out SharedPreferences watching. r?snorp

For easier testing in Fennec, we still preserve the ability to watch a shared
preferences key and automatically enable/disable the font scale listener in
response, but now as an optional feature only.
For pure GeckoView apps, the intention is that those will - via GeckoRuntime-
Settings - directly call setEnabled() as required.

For this, we decouple the enabled state of the listener from its attached state.
The enabled state can now be toggled at all times, but unless the listener is
also attached to a context, it simply won't have any practical effect.

With the dependencies on Fennec's GeckoPreferences and GeckoSharedPrefs gone, we
also move the class into GeckoView in preparation for hooking it up to
GeckoViewRuntimeSettings.

Comment 17

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 7: Automatically attach GeckoFontScaleListener through GeckoRuntime. r?snorp

Comment 18

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 8: Allow setting prefs via GeckoRuntimeSettings in Fennec, too. r?snorp

Comment 19

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 9: Expose font size/inflation options via GeckoRuntimeSettings. r?snorp

Some callers might prefer to manually set the font size in analogy to WebView's
WebSettings.setTextZoom(), respectively allow their users to do so.

Subsequently, we're also going to switch the GeckoFontScaleListener to operate
on those settings.

Because the effects of font inflation are more difficult to quantify than a
plain text zoom, we just check that operating the runtime setting sets the
corresponding Gecko pref. Besides, there already are further platform (ref)tests
checking the actual operation of font inflation itself.

Comment 20

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 10: Set font size settings in listener using GeckoRuntimeSettings. r?snorp

Comment 21

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 11: Allow toggling font scale listener via GeckoRuntimeSettings. r?snorp

Comment 22

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 4: Clean up threading assumptions. r?snorp

In practice, everything in the GeckoFontScaleListener will run on the UI thread,
so get rid of the synchronized methods and just enforce the threading
assumptions in the public API.

Comment 23

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 6: Move GeckoFontScaleListener into GeckoView. r?snorp

With the dependencies on Fennec's GeckoPreferences and GeckoSharedPrefs gone, we
can now move the class into GeckoView in preparation for hooking it up to the
GeckoViewRuntimeSettings.

Comment 24

[Jan Henning [:JanH]]

Attachment: Bug 1460874 - Part 12: Enforce sensible API usage for manual font size settings. r?snorp

Modifying the manual font size settings while the GeckoFontScaleListener is
active is theoretically possible, but probably not the most sensible way of
using that API. Therefore, we prohibit it and throw an exception in that case.

There is one complication, though: The very same API is used by the font scale
listener itself in order to modify the font size settings according to the
system font scale. Therefore, we have to move the GeckoFontScaleListener into
the GeckoView package itself, so that we can provide a package-private internal
API that bypasses the above usage checks.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d5e902b5b88
Part 1: Cleanups. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/d42e39761eb8
Part 2: Rename instance variables to better match GeckoView code style. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/c7fce6c79bef
Part 3: Rename initalize() method for more clarity. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/02f24dec5ae4
Part 4: Clean up threading assumptions. r=snorp
https://hg.mozilla.org/integration/autoland/rev/81c4e88003a5
Part 5: Move out SharedPreferences watching. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/78f59df737fc
Part 6: Move GeckoFontScaleListener into GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/1026b786e779
Part 7: Automatically attach GeckoFontScaleListener through GeckoRuntime. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/17fbff3da236
Part 8: Allow setting prefs via GeckoRuntimeSettings in Fennec, too. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/9fc145133f20
Part 9: Expose font size/inflation options via GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/2020cb134d1f
Part 10: Set font size settings in listener using GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/3d4297e781cb
Part 11: Allow toggling font scale listener via GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/593a2316ac28
Part 12: Enforce sensible API usage for manual font size settings. r=snorp

Comment 26

[Cristian Brindusan [:cbrindusan]]

Backed out 12 changesets (Bug 1460874) for geckoview failures at geckoview.test.RuntimeSettingsTest.automaticFontSize.

Backout: https://hg.mozilla.org/integration/autoland/rev/cd616c3e3eea88cf4acdd4040a2c68ec29a35551

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=593a2316ac28567aaa38f6affb606cd1dde2fac9&selectedJob=228248581

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228248581&repo=autoland&lineNumber=2068

Comment 27

[Jan Henning [:JanH]]

For some unfathomable reason, the 4.3 emulator internally returns null when querying its FONT_SCALE, even though that setting is supposed to exist since API1. This we means that we use the fallback value instead, which unfortunately is a bit inconsistent at the moment: In the test it's 1.0, whereas the GeckoFontScaleListener just keeps the previous value in that case.
All in all, I suppose it might make more sense after all to use 1.0f as a fallback value in the font scale listener, too.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f8f4dd57d09
Part 1: Cleanups. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/552a5ef6bc17
Part 2: Rename instance variables to better match GeckoView code style. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/9da386549713
Part 3: Rename initalize() method for more clarity. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/3daaa6dff87f
Part 4: Clean up threading assumptions. r=snorp
https://hg.mozilla.org/integration/autoland/rev/eea63b0bc4eb
Part 5: Move out SharedPreferences watching. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/9491387c0b66
Part 6: Move GeckoFontScaleListener into GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/c6bce565d3a9
Part 7: Automatically attach GeckoFontScaleListener through GeckoRuntime. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/21762137cbb6
Part 8: Allow setting prefs via GeckoRuntimeSettings in Fennec, too. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/2fb9e3f4153a
Part 9: Expose font size/inflation options via GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/3e3ba34b6a3a
Part 10: Set font size settings in listener using GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/34352fc3a314
Part 11: Allow toggling font scale listener via GeckoRuntimeSettings. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/41ae95deaa37
Part 12: Enforce sensible API usage for manual font size settings. r=snorp

Comment 29

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3f8f4dd57d09
https://hg.mozilla.org/mozilla-central/rev/552a5ef6bc17
https://hg.mozilla.org/mozilla-central/rev/9da386549713
https://hg.mozilla.org/mozilla-central/rev/3daaa6dff87f
https://hg.mozilla.org/mozilla-central/rev/eea63b0bc4eb
https://hg.mozilla.org/mozilla-central/rev/9491387c0b66
https://hg.mozilla.org/mozilla-central/rev/c6bce565d3a9
https://hg.mozilla.org/mozilla-central/rev/21762137cbb6
https://hg.mozilla.org/mozilla-central/rev/2fb9e3f4153a
https://hg.mozilla.org/mozilla-central/rev/3e3ba34b6a3a
https://hg.mozilla.org/mozilla-central/rev/34352fc3a314
https://hg.mozilla.org/mozilla-central/rev/41ae95deaa37

Comment 30

[Chris Peterson [:cpeterson]]

Jan, is font inflation enabled by default? Do we need to file a bug (for Fennec, geckoview_example, Fenix, and/or Reference Browser) to add setting UI for toggling font inflation? Should font inflation be a per-site setting (so I can enable it for one site but disable for another)?

Comment 31

[Jan Henning [:JanH]]

If you really mean font inflation as in the specific feature that scales font sizes only on non mobile-friendly pages [1], then

1. It's off by default, as are the other font size related options I added.
2. It's enabled automatically as part of RuntimeSettings.setAutomaticFontSizeAdjustment(), just like Fennec's "Use system font size" in the accessibility settings works today.

Do we need to file a bug (for Fennec, geckoview_example, Fenix, and/or Reference Browser) to add setting UI for toggling font inflation?

For now, if you want a setting, I'd do something similar to the current setting in Fennec, which should then accordingly be wired up to setAutomaticFontSizeAdjustment(), although maybe unlike Fennec we might want to try enabling it by default again?
For Focus there is https://github.com/mozilla-mobile/focus-android/issues/2752 and https://github.com/mozilla-mobile/focus-android/issues/3057 and for the reference-browser https://github.com/mozilla-mobile/reference-browser/issues/165. Fenix still needs a bug, as might geckoview_example in that case I guess?

If somebody wants to implement something like Chrome's font size setting (font sizes are scaled automatically depending on the device's font size setting, but there's also a manual settings slider with which users can further adjust the result, i.e. the final font size is the product of the user adjustment and the automatic system adjustment), that would require a little further work on the GeckoView side.

Should font inflation be a per-site setting (so I can enable it for one site but disable for another)?

Currently it's a global setting. Making it apply per page would be possible, but very likely a non-trivial amount of work, which I'd rather avoid.
However while I can't promise any timescale, either, I'm still hoping to eventually get around to bug 766599, which would make any font size setting changes through this API instantaneous instead of requiring a page reload, which might be good enough if we ever need a quick toggle.

[1] For mobile-friendly pages, we just do a plain text zoom if you use setFontSizeFactor(), respectively toggle setAutomaticFontSizeAdjustment() with a non-default font size setting on your phone.

Comment 32

[Chris Peterson [:cpeterson]]

Thanks. I'll file bugs for Fenix and geckoview_example asking for a UI setting to toggle setAutomaticFontSizeAdjustment(), linking to the Focus and Reference Browser issues you already filed.

Comment 33

[Chris Peterson [:cpeterson]]

GVE bug 1528893
Fenix issue: https://github.com/mozilla-mobile/fenix/issues/577