1262529 - Replace old font inflation pref

Status: RESOLVED DUPLICATE of bug 1328868

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

We turned off the old font inflation capability in bug 1127441 because there were problems with it, but it's likely that people still want to be able to change (or at least increase) text size.

We should replace the old Gecko pref for font inflation (that doesn't work) with a new one that controls text size or zoom backed by code that actually works.

snorp, I'm not familiar with the existing code for text zoom, so can you summarize a little here what exists/needs to be done?

Comment 1

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

We may need a new pref here, but I think the idea would be to set a default 'full page zoom' percentage. This is different from the pinch kind of zooming. We might already have a way to do this, but I'm not sure. Eugen, please look into it. I think nsPresShell::SetResolution is what would end up being called (NOT nsPresShell::SetResolutionAndScaleTo).

Comment 2

[Kartikaya Gupta (email:[e-mail])]

It's nsPresContext::SetFullZoom, I believe. nsPresShell::SetResolution will not do full zoom.

Comment 3

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> It's nsPresContext::SetFullZoom, I believe. nsPresShell::SetResolution will
> not do full zoom.

Oops, I think that's right.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: [Pixel C 10"] 1.5x text zoom on Wikipedia

This is what 1.5x text zoom looks like on Wikipedia.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: [Pixel C 10"] minTwips=320 on Wikipedia

This is what Wikipedia looks like with font inflation (minTwips=320) enabled.
I've tried to match the 1.5x text zoom scaling for the main content.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: [Pixel C 10"] 1.5x page zoom on Wikipedia

This is what 1.5x page zoom looks like on Wikipedia.

Comment 7

[Eugen Sawin [:esawin]]

In bug 1127441 we set the minTwips pref to 0 by default, which disables font inflation.
Setting minTwips > 0 enables font inflation and based on some limited local testing, it seems to work as intended. 

I don't think we have any Gecko prefs for font inflation that "don't work".

The issue with basic text zooming is that all text is inflated equally, which breaks the layout of a page and may reduce the available screen space for "content" drastically.

I've attached some screenshots with increased text/page zoom and a font inflation setting for comparison (captured on a Pixel C device).

Comment 8

[Jan Henning [:JanH]]

(In reply to Eugen Sawin [:esawin] from comment #7)
> In bug 1127441 we set the minTwips pref to 0 by default, which disables font
> inflation.
> Setting minTwips > 0 enables font inflation and based on some limited local
> testing, it seems to work as intended. 
> 
> I don't think we have any Gecko prefs for font inflation that "don't work".

I think there are two different use cases here: People who want to bump up font sizes everywhere, even on supposedly mobile-friendly/optimised/... pages and people who only want larger fonts for desktop pages.
I agree that for people in the first group, the naming of the setting is somewhat confusing, because it seems natural to expect that it will change font sizes everywhere.
However such a setting wouldn't work that well for people in the second group (Disclaimer: I'm one of them). I for example am perfectly happy with font sizes on mobile pages, whereas desktop pages are more or less unusable in portrait mode and barely usable in landscape mode without some sort of increased font size.

> The issue with basic text zooming is that all text is inflated equally,
> which breaks the layout of a page and may reduce the available screen space
> for "content" drastically.

Exactly. Font inflation's heuristics aren't perfect and apparently in some cases they might even break a page, which seems to have prompted the decision to turn it off by default, but in my experience and for the desktop pages I'm regularly visiting on my phone font inflation still seems to be working quite well enough [1]. And as an aside, I've checked Chrome Dev and as far as I can tell, even the latest version still contains Google's version of font inflation.

A compromise - at the cost of an additional settings option and having to continue maintaining font inflation - could be to have the "text size" setting provide full page/text zooming by default, but also have a checkbox called something like "Font Inflation - Increase font sizes for desktop pages only" which switches the behaviour of the text size setting to today's font inflation.

[1] It has some difficulties with forum posts containing only a few words, plus I've seen some more benign variety of bug 1132870 cropping up again occasionally, but other that that it works quite well in bumping up the font size for the main content only.

Comment 9

[Jan Henning [:JanH]]

What I've forgot to add:
Having just one setting which affects font sizes everywhere would also be doubly inconvenient for people in the second group, because changing font sizes is a more involved procedure on mobile:
On desktop, changing font sizes is at least simply a matter of pressing Ctrl and rotating the mouse wheel, whereas in Android, you have to go several levels into the settings menu. So having to switch font sizes every time you switch between visiting a desktop page and a mobile page would be quite annoying (even if hypothetically speaking the font size setting was available directly from the main menu, it would still be more uncomfortable to use than today's automatic font inflation).

Comment 10

[:Margaret Leibovic]

NI barbara to think about the product questions raised in the previous comments.

Comment 11

[Eugen Sawin [:esawin]]

Attachment: [S3 Mini 4"] 2x page zoom on Wikipedia

Comment 12

[Eugen Sawin [:esawin]]

Attachment: [S3 Mini 4"] 2x text zoom on Wikipedia

Comment 13

[Eugen Sawin [:esawin]]

Attachment: [S3 Mini 4"] minTwips=160 on Wikipedia

Comment 14

[Eugen Sawin [:esawin]]

I've attached more screenshots of the desktop Wikipedia site, this time captured on the 4" Samsung S3 Mini device.

Comment 15

[Eugen Sawin [:esawin]]

To provide a (somewhat hand-wavy) overview, I've collected some findings (see screenshots) here for discussion.

Mobile-optimized pages
  * Font inflation
    - No effect
  * Text zoom
    + Increases all text size
    - Some potential to break layout/UI (e.g., on http://en.m.wikipedia.com not possible to select languages)
  * Page zoom
    + Increases all page element sizes
    - High potential to break layout/UI

Desktop pages
  * Font inflation
    + Increases content text size (based on heuristics)
    + Mostly conserves layout
    - Non-content text size is not increased (e.g., pinch-zooming required for navigation)
  * Text zoom
    + Increases all text size
    - Reduces "main content" space
    - Affects layout
  * Page zoom
    + Increases all page element sizes
    - Strongly reduces "main content" space
    - Strongly affects layout

Comment 16

[:Margaret Leibovic]

NI to me to make Aha card.

Comment 17

[Barbara Bermes [:barbara] - NI please!]

de-prioritizing this for now

Comment 18

[Barbara Bermes [:barbara] - NI please!]

> A compromise - at the cost of an additional settings option and having to
> continue maintaining font inflation - could be to have the "text size"
> setting provide full page/text zooming by default, but also have a checkbox
> called something like "Font Inflation - Increase font sizes for desktop
> pages only" which switches the behaviour of the text size setting to today's
> font inflation.

I'm ok with it only if these things contextually are really different and we are not trying to avoid or circumvent another issue.

I see this being the former and therefore ok. NI'ing UX.

Do we feel font inflation is terminology normal users understand?

Comment 19

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

I don't think we should ever have 'font inflation' in our UI. We should just kill it entirely. People don't understand what's going on when it's activated at all. A generic "increase the size of all text" knob is something people can definitely understand.

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Barbara Bermes [:barbara] from comment #18)
> > A compromise - at the cost of an additional settings option and having to
> > continue maintaining font inflation - could be to have the "text size"
> > setting provide full page/text zooming by default, but also have a checkbox
> > called something like "Font Inflation - Increase font sizes for desktop
> > pages only" which switches the behaviour of the text size setting to today's
> > font inflation.
> 
> I'm ok with it only if these things contextually are really different and we
> are not trying to avoid or circumvent another issue.
> 
> I see this being the former and therefore ok. NI'ing UX.
> 
> Do we feel font inflation is terminology normal users understand?

(In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> I don't think we should ever have 'font inflation' in our UI. We should just
> kill it entirely. People don't understand what's going on when it's
> activated at all. A generic "increase the size of all text" knob is
> something people can definitely understand.

I do not know what "font inflation" means.

I've tried following this bug and the comments posted, but I can't seem to figure out what the goal of this is. 

What's the goal/ user benefit here?

Comment 21

[Jan Henning [:JanH]]

As I understand it, the intention behind "font inflation" is/was to bump up font sizes for desktop/non-mobile-friendly pages while not breaking page layout too much. To that end
- font inflation is only working for non-mobile friendly pages. If a viewport width=device-width/height=device-height or initial-scale=1.0 tag is detected, we assume it's a mobile page and turn it off
- through some sort of heuristic, font inflation tries to identify the main content of a page and only increases the font size for that content. The intention is for footers/headers, navigation elements, menus etc. to remain at their original size, so as not to break the page layout too much and to leave as much space as possible for the main content (compare also esawin's screenshots to see the effect of font inflation compared to normal page or text zoom).

The user benefit is:
You don't have to keep changing the font size settings each time you enter or leave a desktop page, which is great, because desktop pages are only barely readable even in landscape mode and changing the font size on a phone (click through several menu levels) is much more involved than on desktop (Ctrl + mouse wheel, and the setting is remembered per domain I think?), which is a bit ironic, because if it wasn't for font inflation, I'd have to change font sizes far more often on my phone than on desktop.

However I acknowledge that font inflation's heuristics aren't perfect (though they're definitively not horrible, either) and that it doesn't do anything for people who want to scale font sizes everywhere - mobile pages included.


See also http://www.jwir3.com/font-inflation-fennec-and-you/ for some historic background from when this was first implemented.

Comment 22

[Kevin Brosnan [Ex-Mozilla]]

Jan has the history. The other part is to create new feature to enable full page zoom for all pages and create some UI around that. With the intention that the new feature will replace font inflation which is poorly understood.

Comment 23

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

Can we get this moving again? Anthony, comment #5 has a good link about what font inflation was all about, but I feel like the fact that you don't know what it is might already be a good indication that we should kill it.

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah, I'm not convinced this is a high priority item right now. But Barbara might have other thoughts?

NI-ing for her input too

Comment 25

[Mike Kaply [:mkaply]]

Looking at Chrome, it's not about page zoom, it's just about just making fonts bigger. And if the page breaks, that's life. There's only so much we can do. Can we do something like that?

As far as priority goes, we have an OEM that has marked this as a problem.

To a typical user, this feature simply looks broken. Adjust text size in prefs does nothing on most pages...

Comment 26

[Jan Henning [:JanH]]

(In reply to Mike Kaply [:mkaply] from comment #25)
> Looking at Chrome [...]

One important difference being that Chrome still does its own equivalent of "font inflation" on desktop/non-mobile pages.

As far as I can see, their text size setting seems to work as follows:

On "mobile" pages (i.e. any page where font inflation is not activated), all the text on a page is simply scaled by the factor set by the user, i.e. just a plain text zoom.

On "desktop" pages (any page where font inflation is active) on the other hand, the text zoom is applied only to text that has already been bumped up by font inflation, so the setting effectively controls the amount of font inflation that is applied, just as our text size setting currently works.


If we'd be okay with turning font inflation back on by default, we could in theory mirror this:
"Medium" would correspond to normal font inflation and text zoom for mobile pages at 100 %, while any step above "Medium" would increase both the font inflation size as well as the text zoom for all other pages. Similarly, in the other direction we'd just turn down (and eventually deactivate) font inflation as well as decrease the text zoom to below 100 %.

Comment 27

[Barbara Bermes [:barbara] - NI please!]

Mike, if possible, can you private ping me and give some more details about the OEM.  Did they flag this as a web compat issue for a specific website they tested this on?

Comment 28

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

(In reply to Anthony Lam (:antlam) from comment #20)
> 
> I do not know what "font inflation" means.

Isn't that enough reason to kill it? I am not sure how you can tolerate having a feature in the browser that you yourself don't understand.

> 
> I've tried following this bug and the comments posted, but I can't seem to
> figure out what the goal of this is. 
> 
> What's the goal/ user benefit here?

The goal is to make the current 'font size' settings do what the user expects.

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I talked to Sebastian about this some more. There's a lot of confusion here caused by unexpected behaviour, language, and just general bugs. I actually thought this was just broken and it was supposed to affect all the text I see in Firefox.

So our proposal is to user test the following:

1) Fix the pref to affect all text sizes rendered from web pages 
2) Revise message to reflect expected beaviour
3) After the change, all Fennec users will have this reset to "default"

Barbara - thoughts?

Comment 30

[Jan Henning [:JanH]]

Attachment: PresShell debug logging

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

Comment 31

[Jan Henning [:JanH]]

Attachment: Reenable font inflation by default on Fennec and add a pref controlling global zoom

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

Comment 32

[Jan Henning [:JanH]]

Attachment: Re-disable font inflation in Android reftests

(compare bug 1298023)

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

Comment 33

[Jan Henning [:JanH]]

Attachment: Split out checking whether the document is eligible for font size inflation into its own function.

Currently, we don't differentiate between font inflation being disabled because of the preferences, or because the document has been detected as being mobile-friendly. Because we now need to differentiate between the two cases for enabling or disabling global zooming, the latter check has been moved forward so it is run ever time.

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

Comment 34

[Jan Henning [:JanH]]

Attachment: access new globalZoom pref through nsLayoutUtils

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

Comment 35

[Jan Henning [:JanH]]

Attachment: Add pref and nsLayoutUtils access to switch between text and full zoom

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

Comment 36

[Jan Henning [:JanH]]

Attachment: Implement zooming when non-font inflated pages

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

Comment 37

[Jan Henning [:JanH]]

Attachment: more debug logging

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

Comment 38

[Jan Henning [:JanH]]

Attachment: 1262529 - update preferences UI to handle new font size settings

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

Comment 39

[Jan Henning [:JanH]]

Attachment: More PresShell debug logging

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

Comment 40

[Jan Henning [:JanH]]

Attachment: Set zoom via content viewer

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

Comment 41

[Jan Henning [:JanH]]

Attachment: set zoom debug logging

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

Comment 42

[Jan Henning [:JanH]]

Attachment: Don't run global zoom in reftests, because it can interfere with reftest-zoom

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

Comment 43

[Jan Henning [:JanH]]

This is a somewhat rough WIP patch series - because of my lack of knowledge there, at least the Gecko-side implementation probably has some room for improvement. Some random thoughts/remarks:
- There might be a better place to run this code from, especially since with the current implementation the PresShell's RecomputeDocEligibleForFontSizeInflation() function has gained some additional side effects (setting full/text zoom levels if the functionality is enabled), which feels a bit hacky.
- This probably doesn't play nicely with anything else that's trying to set zoom levels during page load, e.g. the per-page zoom settings (browser-fullZoom.js) on Desktop (although as long as we don't enable this on desktop it won't matter) or reftest-zoom. Regarding the latter, strangely enough only the reftest for bug 621253 seems to be actually failing (the cheap solution would be just not enabling this zoom on reftests, just like we don't run reftests with font inflation).
- I've no idea whether the SetFullZoom()/SetTextZoom() functions can be called directly on the PresContext or whether we should call them via the ContentViewer, which seems to do some additional work.
- Whether we want to use text or full zoom - I've defaulted to text zooming, but for now this can be controlled via the "browser.zoom.globalZoomFull" pref.
- The relation between the zoom and the corresponding twips values might or might not still need some fine-tuning (as does maybe the preview for the smallest size in the preferences dialogue)?

In any case, a try build for playing around with can be found here:
https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-d1b130590fd2fc8f2bc34570bf4f769e5e8a1413/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk

Comment 44

[Mike Kaply [:mkaply]]

Information sent to Barbara.

Comment 45

[Barbara Bermes [:barbara] - NI please!]

I scheduled a quick huddle for next week, Jan, we'll update you after in this bug.

Comment 46

[Barbara Bermes [:barbara] - NI please!]

Outcomes and next steps from meeting January 4

- Remove existing "Text Size" setting from Settings>Accessibility
- Include setting (toggle on/off) under Settings>Accsibility to honor system font size (we will discuss later if this should be on/off by default)
- The new setting will apply to mobile and desktop web pages

Platform and Frontend UI will work together on this

Comment 47

[Barbara Bermes [:barbara] - NI please!]

created new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1328868