Expose font size/font inflation settings

RESOLVED FIXED in Firefox 67

Status

defect
P2
normal
RESOLVED FIXED
Last year
5 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
All
Android
Dependency tree / graph

Firefox Tracking Flags

(geckoview64 wontfix, geckoview65 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [geckoview:fenix:p2], )

Attachments

(12 attachments, 1 obsolete attachment)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
Since those features (default text zoom and font inflation) apply globally to all pages, would GeckoRuntimeSettings be the place to add them?
Flags: needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
Assignee: nobody → jh+bugzilla
Duplicate of this bug: 1470839
Whiteboard: [geckoview:klar][focus:feature]
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.
Depends on: 1475875
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.
Attachment #8992213 - Flags: review?(snorp) → review+
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.
Product: Firefox for Android → GeckoView
P3 according to Vesta's list of Fenix requirements
Whiteboard: [geckoview:klar][focus:feature] → [geckoview:fenix:p3]
P2 for Fenix
Priority: P3 → P2
Whiteboard: [geckoview:fenix:p3] → [geckoview:fenix:p2]
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)?
Flags: needinfo?(snorp)

(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.

Flags: needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.

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.

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().

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.

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.

Attachment #8992213 - Attachment is obsolete: true
Attachment #9039342 - Attachment description: Bug 1460874 - Part 1: Cleanups. r?geckoview-reviewers → Bug 1460874 - Part 1: Cleanups. r?snorp
Attachment #9039343 - Attachment description: Bug 1460874 - Part 2: Rename instance variables to better match GeckoView code style. r?geckoview-reviewers → Bug 1460874 - Part 2: Rename instance variables to better match GeckoView code style. r?snorp
Attachment #9039344 - Attachment description: Bug 1460874 - Part 3: Rename initalize() method for more clarity. r?geckoview-reviewers → Bug 1460874 - Part 3: Rename initalize() method for more clarity. 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.

Attachment #9039345 - Attachment description: Bug 1460874 - Part 4: Make SharedPreferences watching optional. r?geckoview-reviewers → Bug 1460874 - Part 5: Move out SharedPreferences watching. 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.

Attachment #9039346 - Attachment description: Bug 1460874 - Part 5: Automatically attach GeckoFontScaleListener through GeckoRuntime. r?geckoview-reviewers → Bug 1460874 - Part 7: Automatically attach GeckoFontScaleListener through GeckoRuntime. r?snorp
Attachment #9039347 - Attachment description: Bug 1460874 - Part 6: Allow setting prefs via GeckoRuntimeSettings in Fennec, too. r?geckoview-reviewers → Bug 1460874 - Part 8: Allow setting prefs via GeckoRuntimeSettings in Fennec, too. r?snorp
Attachment #9039348 - Attachment description: Bug 1460874 - Part 7: Expose font size/inflation options via GeckoRuntimeSettings. r?geckoview-reviewers → Bug 1460874 - Part 9: Expose font size/inflation options via GeckoRuntimeSettings. r?snorp
Attachment #9039349 - Attachment description: Bug 1460874 - Part 8: Set font size settings in listener using GeckoViewRuntimeSettings. r?geckoview-reviewers → Bug 1460874 - Part 10: Set font size settings in listener using GeckoRuntimeSettings. r?snorp
Attachment #9039350 - Attachment description: Bug 1460874 - Part 9: Allow toggling font scale listener via GeckoRuntimeSettings. r?geckoview-reviewers → Bug 1460874 - Part 11: Allow toggling font scale listener via GeckoRuntimeSettings. 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.

Pushed by mozilla@buttercookie.de:
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

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.

Flags: needinfo?(jh+bugzilla)
Pushed by mozilla@buttercookie.de:
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

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)?

Flags: needinfo?(jh+bugzilla)

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.

Flags: needinfo?(jh+bugzilla)

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.

You need to log in before you can comment on or make changes to this bug.