1124848 - "Text size" setting is not respected

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Boris Dušek]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/600.2.5 (KHTML, like Gecko) Version/8.0.2 Safari/600.2.5

Steps to reproduce:

1. Settings.app > Display & Brightness > Text Size > Change the text size away from the default value (bigger/smaller) (by default, only 7 values are available, 5 more values - all of them bigger than the largest of the 7 - can be enabled using Settings.app > General > Accessibility > Larger Text > Larger Accessibility Sizes)
2. Open Firefox.app


Actual results:

No change in font size in the UI of the app with few exceptions (like Settings > Tabs switch label or search suggestions) nor in Reader


Expected results:

The font size in the UI of the app (that is outside of the web view) and in Reader mode gets adjusted according to the "Text Size" setting, yielding bigger/smaller text.

This is supported by Safari.

We can learn about the correct font size for a specific dynamic type category (e.g. UIFontTextStyleBody) using:

UIFontDescriptor.preferredFontDescriptorWithTextStyle(UIFontTextStyleBody).pointSize

If we need a font size that does not fall into the 6 Dynamic type categories, we could choose one of the 6 categories as a "base" category for that size and "derive" the needed size by applying percentage of the base size - e.g. "80% of the Caption1 style". This would preserve the relative differences of font sizes no matter what setting the user chose for "Text Size" and would still allow us to use more than the 6 base size categories to provide a custom experience.

This would improve the UI for low-vision people and senior citizens who have trouble seeing normal-sized text (provided they set the Text Size setting appropriately, or that someone sets it for them).

Comment 1

[Richard Newman [:rnewman]]

We'll take patches for this, but not block for the immediate first release.

Comment 2

[anon39483]

I also experience this bug in Firefox for Android. Stock, unrooted Nexus 4 running Lollipop 5.1.1 (latest stagefright OTA fix). Bug was present both in Firefox for Android versions 39 and 40. Changing the text size setting does not affect the Firefox menu text sizes nor any of the text in any websites.

Comment 3

[Aaron Train [:aaronmt]]

(In reply to anon39483 from comment #2)
> I also experience this bug in Firefox for Android. Stock, unrooted Nexus 4
> running Lollipop 5.1.1 (latest stagefright OTA fix). Bug was present both in
> Firefox for Android versions 39 and 40. Changing the text size setting does
> not affect the Firefox menu text sizes nor any of the text in any websites.

This bug is specific to iOS efforts. Text size adjustments in Firefox for Android can be misleading as its useful only on unoptimized for mobile for content — it has no affect on UI nor mobile-optimized content.

Comment 4

[Etienne Segonzac (:etienne)]

Attachment: Pull request

Hey Stephan, here's a first try at this one. What do you think about the approach?

* Introducing a `DynamicFontHelper` (singleton)
* Moving from `UIConstants.*` fonts to `DynamicFontHelper.defaultHelper.*` fonts
* Introducing new `UIConstants.*` fonts for the chrome (looks like Safari keeps the chrome unchanged too)
* Making sure autolayouted tableviews automagically reflow to the new font size when the setting changes
* Using a notification for the other cases


Once we're set on the design code-wise, I'll make a bunch of screenshots with different Text Size settings so this can be thoroughly ui-reviewed.
Please let me know if there's any good unit-tests/UITests to add too!

Cheers!

Comment 5

[Stephan Leroux [:sleroux]]

Comment on attachment 8693351 [details] [review]
Pull request

Huge thanks for this patch. We've been wanting to get this in but haven't got around to it. I like the approach you took with creating the DynamicFontHelper and having it listen to the UIContentSizeCategoryDidChangeNotification notification and forwarding the change to it's listeners. Left some small nits on the PR.

Overall I think this looks pretty good visually. The text sizes scale to sizes which makes sense and I'm surprised that our layouts are holding up to the changes. 

For testing, you have two options:

1. We use KIF [1] for automated blackbox UI testing. We have some test cases setup in the UITests folder in the project that you can use as a reference to get started. Most of the labels should have a way to refer to it using accessibility labels so you can use that to test the font changes. I'm not sure how to change the text size in the settings app using KIF though.

2. FBSnapshotTestCase [2]. This is more of a unit test for views. This is a new framework that I've recently added to allow us to do 'Ref testing' for our native views. I'm not sure if it's applicable here but you can check it out if you see it working better. These tests are located under NativeRefTests. There isn't any test cases yet on master but you can check out this branch [3] for how I've used it for an upcoming feature.

[1] https://github.com/kif-framework/KIF

[2] https://github.com/facebook/ios-snapshot-test-case

[3] https://github.com/mozilla/firefox-ios/commit/5445583e55cc8640e887ffb940c833d4bf8c7671 && https://github.com/mozilla/firefox-ios/commit/30938a9beda7d7416d32ee8a1c2dd22ba1a30016

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8693351 [details] [review]
Pull request

Comments addressed and tests added :)
I just pushed new commits to make it easier to review but let me know if you want me to squash.

Darrin, not sure what's the best way to ui-review this. You can install this build and play with the accessibility settings or I can make screenshots of each screens at the "extremes" maybe (smallest font, biggest font etc...), let me know what works best :)

Comment 7

[Darrin Henein [:darrin]]

This looks great! I tried it at the largest and smallest sizes and things seem to layout properly. Not sure if this was meant to be a full pass, I noticed a few areas that didn't respond to the text size (intro tour, for one).

Comment 8

[Stephan Leroux [:sleroux]]

Comment on attachment 8693351 [details] [review]
Pull request

Looks good - thanks for the tests!

:darrin, do we want to do an audit of which screens we want to apply dynamic text to before landing this?

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Darrin Henein [:darrin] from comment #7)
> This looks great! I tried it at the largest and smallest sizes and things
> seem to layout properly. Not sure if this was meant to be a full pass, I
> noticed a few areas that didn't respond to the text size (intro tour, for
> one).

The settings were already "dynamic", this patch adds support for:
- the various home panels
- the reader mode
- the tab tray

Missed the intro tour :)

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8693351 [details] [review]
Pull request

Just rebased and pushed a new version with support for the Intro :)

Comment 11

[Stephan Leroux [:sleroux]]

Comment on attachment 8693351 [details] [review]
Pull request

Looks good! I noticed the tab counter is bigger than it was though. Maybe the scale is off for that case?

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Stephan Leroux [:sleroux] from comment #11)
> Comment on attachment 8693351 [details] [review]
> Pull request
> 
> Looks good! I noticed the tab counter is bigger than it was though. Maybe
> the scale is off for that case?

It's part of the chrome and should have a constant size, I'll push a fix!

Comment 13

[Stephan Leroux [:sleroux]]

I'm going to go ahead and land this. If we have any other screens we've missed we can create new bugs.

Comment 14

[Stephan Leroux [:sleroux]]

Master 59337262d0d4551c9be428f29122c9c1600716ab