Closed Bug 1179020 Opened 6 years ago Closed 5 years ago

Font inflation default value is "Tiny"

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox42 affected)

RESOLVED INVALID
Tracking Status
firefox42 --- affected

People

(Reporter: liuche, Unassigned, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(alam)
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".
Flags: needinfo?(alam) → needinfo?(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?
Flags: needinfo?(liuche) → needinfo?(alam)
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?
Flags: needinfo?(alam) → needinfo?(liuche)
Yeah, this makes no sense. The default size should be the middle size, and you can choose to increase or decease from there.
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 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.
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".
Mentor: liuche
Flags: needinfo?(liuche)
Whiteboard: [good first bug][lang=java]
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.
Attached patch 1179020.patch (obsolete) — Splinter Review
Attachment #8637643 - Flags: review?(liuche)
Hi,

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

Thanks
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.
Attachment #8637643 - Flags: review?(liuche) → feedback+
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.
Assignee: nobody → gioyik
Status: NEW → ASSIGNED
Attached patch 1179020.patchSplinter Review
Patch updated
Attachment #8638257 - Flags: review?(liuche)
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
Attachment #8637643 - Attachment is obsolete: true
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".
Attachment #8638257 - Flags: review?(liuche) → feedback+
We'll be removing this preference in bug 1193486 because it doesn't do anything now that font inflation is disabled (bug 1127441).
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Actually this is the second most used setting, so we should probably keep it. It's even ahead of DNT. un-WONTFIX-ing it.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
Depends on: 1196516
So, this bug is still valid? I just ask to continue working on this or just wait for a final discussion about this bug.
Flags: needinfo?(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.
Flags: needinfo?(liuche)
liuche, what's the status of this bug? It's confusing to have it in our list of good first bugs.
Flags: needinfo?(liuche)
Depends on: 1262529
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.
Flags: needinfo?(liuche)
Whiteboard: [good first bug][lang=java]
Assignee: gioyik → nobody
Obsoleted by bug 1328868.
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → INVALID
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.