1179020 - Font inflation default value is "Tiny"

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

In bug 1127441, we talked about removing this pref altogether. There is still the use case where the lines are long so pinch-zoom requires a lot of left/right scrolling, and reader mode is not available (or appropriate).

We should at least make the default size option "Normal" or "Default" instead of Tiny.

Comment 1

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

Any reason why we feel like the default needs to be something other than "default" here? I think the issue with it in bug 1127441 was that it was set to "tiny"?

I'm fine with leaving the pref, but defaulting to "Default".

Comment 2

[Chenxia Liu [:liuche]]

Okay, let's update the strings then - currently the values are: Tiny, Small, Medium, Large, Extra Large.

What strings would you like to see instead?

Comment 3

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

Sorry, I should be more clear. The language of the presets is fine. But we should default to something more sensible.

What did we use to default to? Was it always "Tiny"? If so, why?

Comment 4

[:Margaret Leibovic]

Yeah, this makes no sense. The default size should be the middle size, and you can choose to increase or decease from there.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

We have never tested negative font inflation. For tiny to be the default in the middle we would need to use negative values for font.size.inflation.minTwips.

Font inflation is underinvested. It produces odd runs of text. In addition we are getting a lot more mobile content several of the major news sites that were an issue now give Firefox for Android mobile content. I routinely run into support issues with users who change this and don't understand why website text does not change.

Comment 6

[:Margaret Leibovic]

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> We have never tested negative font inflation. For tiny to be the default in
> the middle we would need to use negative values for
> font.size.inflation.minTwips.
> 
> Font inflation is underinvested. It produces odd runs of text. In addition
> we are getting a lot more mobile content several of the major news sites
> that were an issue now give Firefox for Android mobile content. I routinely
> run into support issues with users who change this and don't understand why
> website text does not change.

In that case, I would support us removing this preference. UI telemetry does show that people are using this, but if they're confused by the behavior, that's even worse than them not using it.

Comment 7

[Chenxia Liu [:liuche]]

There are definitely pages that are not reader-modeable, where the font is pretty tiny, so there's still a case to be made for keeping this pref in. We can keep this pref in by just removing three of the sizes so that the only ones we have remaining are "Normal, Large, Extra Large".

Comment 8

[Chenxia Liu [:liuche]]

This bug is for updating the options for font inflation to Normal, Large, and Extra Large.

The array to modify is here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/arrays.xml#26

The old strings for Tiny and Small should be removed, and a new one for Normal should be added.

Comment 9

[Giovanny Gongora [:gioyik]]

Attachment: 1179020.patch

Comment 10

[Giovanny Gongora [:gioyik]]

Hi,

I created a patch for this Bug. Could you review it and tell me if this is a good one.

Thanks

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8637643 [details] [diff] [review]
1179020.patch

Review of attachment 8637643 [details] [diff] [review]:
-----------------------------------------------------------------

Almost - let's get rid of the strings we're not using anymore too.

::: mobile/android/base/resources/values/arrays.xml
@@ -23,5 @@
>          <item>2</item>
>          <item>0</item>
>      </string-array>
>      <string-array name="pref_font_size_entries">
> -        <item>@string/pref_font_size_tiny</item>

Great - please also remove these strings from the string files, now that they won't be used any more.

These files are mobile/android/base/strings.xml.in and mobile/android/base/locales/en-US/android_strings.dtd.

Comment 12

[Chenxia Liu [:liuche]]

Also, add "r=liuche" at the end of the bug message - that way, when this patch lands, we can keep track of both the patch author and the reviewer.

Comment 13

[Giovanny Gongora [:gioyik]]

Attachment: 1179020.patch

Patch updated

Comment 14

[Giovanny Gongora [:gioyik]]

Hi Chenxia,

I attached a new patch with new changes you suggested and I added the review flag at the end of the bug message. Let me know if this is ok to land or needs something else.

Thanks

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8638257 [details] [diff] [review]
1179020.patch

Review of attachment 8638257 [details] [diff] [review]:
-----------------------------------------------------------------

Almost! A few more comments. Also, try to verify that your changes are doing what you expect them to, with no crashes. When I changed the font size from Normal to Extra Large, I encountered a crash.

I didn't catch this the first time around, but you should also be removing items from the other font_size array. See comment.

Additionally, one thing that I'm seeing is that in Settings > Display > Text Size is that when the text is set to the "Normal" size, the preview text looks really, really tiny. Try building this and looking at the preview from Settings - do you see that? Take a look at mobile/android/base/preferences/FontSizePreference.java and see if you can update that "Normal" font size preview to be something more reasonable. (I can give you more hints if you'd like, but I'll give you a chance to figure it on your own if you'd like.)

Let me know if you have any questions about this! Sometimes code removal can be tricky, because you have to figure out if there are side effects, or if you're leaving behind other leftover code that's no longer relevant.

::: mobile/android/base/resources/values/arrays.xml
@@ -30,5 @@
>          <item>@string/pref_font_size_large</item>
>          <item>@string/pref_font_size_xlarge</item>
>      </string-array>
>      <string-array name="pref_font_size_values">
>          <item>0</item>

You also need to remove items from pref_font_size_values, because these are the values that correspond to the entries. Leaving all of these in will cause a crash. Test this out by changing the font size from "Normal" to "Extra Large", and you'll see the crash.

One thing to note is that, contrary to what we're doing in the other places, you'll want to remove the last entry (240), not the first one (0). Does that make sense? "0" indicates "no font inflation", which should correspond to "Normal".

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We'll be removing this preference in bug 1193486 because it doesn't do anything now that font inflation is disabled (bug 1127441).

Comment 17

[Chenxia Liu [:liuche]]

Actually this is the second most used setting, so we should probably keep it. It's even ahead of DNT. un-WONTFIX-ing it.

Comment 18

[Chenxia Liu [:liuche]]

FWIW, bug 1127441 isn't what removed this functionality - that just turned off font inflation by default, but it was still possible to adjust it with the font inflation pref. Something else has happened since end of July (I think) to break this pref.

Comment 19

[Giovanny Gongora [:gioyik]]

So, this bug is still valid? I just ask to continue working on this or just wait for a final discussion about this bug.

Comment 20

[Chenxia Liu [:liuche]]

Mmm, I still need to decide whether this is worthwhile to pursue - I tried out a couple of desktop sites, and while the pref still works, it only works on some desktop sites. The fact that this resizing pref pretty much has no effect on virtually all mobile sites is a problem; we don't want to need to have space for a pref that is only useful in a very small number of cases.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

liuche, what's the status of this bug? It's confusing to have it in our list of good first bugs.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(if it's not actionable)

Comment 23

[Chenxia Liu [:liuche]]

We might add a completely different pref that's not "font inflation" but related; we'll discuss in bug 1262529 but yeah, this isn't a good first bug because it's not clear what we'll be doing.

Comment 24

[Jan Henning [:JanH]]

Obsoleted by bug 1328868.