Closed Bug 1328868 Opened 7 years ago Closed 7 years ago

Replace "Text Size" setting with system "Font size" setting

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(relnote-firefox 55+, firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
relnote-firefox --- 55+
firefox55 --- verified

People

(Reporter: barbara, Assigned: JanH, NeedInfo)

References

Details

Attachments

(11 files, 5 obsolete files)

Bug 1328868 - Part 0 - Remove unneeded imports.
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1328868 - Part 4 - Add a new switch offering to honour the system font size to Fennec's preferences.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1328868 - Part 5 - Add a Java-side listener watching the new pref and the Android system font scale.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1328868 - Part 6 - Remove the old font size preference and its associated code and resources.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1328868 - Part 7 - Move MakeObserver() into head.js
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1328868 - Part 8 - Add a test to check that flipping the Android pref enables/disables font inflation.
59 bytes, text/x-review-board-request
esawin
: review+
sebastian
: review+
Details
Bug 1328868 - Part 9 - Check that zooming via "font.size.systemFontScale" actually has any effect.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1328868 - Part 10 - Make about:config handle text zoom changes a bit more gracefully.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1328868 - Part 3 - Make the effective text zoom retrievable from JS.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details 
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
NI-ing Michelle for her thoughts on the pref here
Flags: needinfo?(mheubusch)
Takeaway from an extended discussion with snorp on IRC:
With this new setting off, we don't want to do any kind of text zoom or font inflation (-> minTwips = 0) and render pages just as they appear by default.

With the new "Honor system font sizes" setting turned on, we want to take a similar approach as Chrome does regarding font sizes, except that we simply obey the system's font size setting instead of providing our own additional slider.
That means that with the default system font size we want to do minTwips = 120 (so desktop pages appear at approximately the same effective font size as mobile pages) and text zoom for mobile pages at 100 %, and then scale both values proportionally up or down if the system font scale changes.
Assignee: nobody → jh+bugzilla
Snorp suggested you might be the person to ask for a first look at this, especially the Gecko-side implementation of this.
Flags: needinfo?(esawin)
Just so I don't forget it - the initialisation of the Java settings listener depends on code introduced in bug 1336734.
Depends on: 1336734
Comment on attachment 8836329 [details]
Bug 1328868 - Part 0 - Remove unneeded imports.

https://reviewboard.mozilla.org/r/109874/#review113312
Attachment #8836329 - Flags: review+
Comment on attachment 8836330 [details]
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.

https://reviewboard.mozilla.org/r/109866/#review113314

::: modules/libpref/init/all.js:3198
(Diff revision 1)
> + * the normal font size.
> + *
> + * Setting this to 0 completely deactivates this feature and bypasses
> + * the responsible code.
> + */
> +pref("font.size.systemFontScale", 0);

Why don't default to 1 instead of 0 to disable font scaling? This should avoid special-casing down the road.
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review113316

::: layout/base/PresShell.cpp:11121
(Diff revision 1)
> +    if (mGlobalZoomWasEnabled) {
> +      SetGlobalZoom(1.0f);
> +      mGlobalZoomWasEnabled = false;
> +    }
> +    return;
> +  }

This would be simplified with a default font scale of 1.

::: layout/base/PresShell.cpp:11125
(Diff revision 1)
> +    return;
> +  }
> +
> +  MOZ_ASSERT(mDocument, "our document should not be null");
> +
> +  if (!mFontSizeInflationEnabled && !mDocument->IsSyntheticDocument()) {

Can't we just get rid of the dedicated font size inflation flag, since all is controlled through the global font scale now?

::: layout/base/PresShell.cpp:11134
(Diff revision 1)
> +  }
> +  mGlobalZoomWasEnabled = true;
> +}
> +
> +void
> +nsIPresShell::SetGlobalZoom(const float& aZoomFactor)

Pass PODs by value, not const reference.
Comment on attachment 8836340 [details]
Bug 1328868 - Part 7 - Move MakeObserver() into head.js

https://reviewboard.mozilla.org/r/109936/#review113332
Attachment #8836340 - Flags: review+
Comment on attachment 8836341 [details]
Bug 1328868 - Part 8 - Add a test to check that flipping the Android pref enables/disables font inflation.

https://reviewboard.mozilla.org/r/109938/#review113328
Attachment #8836341 - Flags: review+
Nice work! This requires more changes than expected.

Please run the Gecko parts through kats and the Android/Java parts through Sebastian.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(esawin)
Flags: needinfo?(bugmail)
Looking at the Gecko-side changes I think a layout peer would be a better person to review. dbaron or tnikkel, perhaps.
Flags: needinfo?(bugmail)
Comment on attachment 8836330 [details]
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.

https://reviewboard.mozilla.org/r/109866/#review113314

> Why don't default to 1 instead of 0 to disable font scaling? This should avoid special-casing down the road.

This is a leftover from my original attempt at this. Originally, I didn't have the `mGlobalZoomWasEnabled` flag, so when turning this back off, I had to explicitly set the Gecko pref to 100 in order to reset the zoom state. So for platforms where we'd never want to enable this because we don't need it (e.g. Desktop, at least currently), I introduced the special casing for the 0 value which would completely bypass the zoom code.

This time round I found a solution for automatically resetting the text zoom when turning this off, but obviously didn't think far enough. Thanks for spotting this.
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review113316

> Can't we just get rid of the dedicated font size inflation flag, since all is controlled through the global font scale now?

We can't - applied font inflation will prevent global zooming, but otherwise it's still an independent feature that needs to keep track of its own state.
(In reply to Eugen Sawin [:esawin] from comment #25)
> Please run the Gecko parts through kats and the Android/Java parts through
> Sebastian.

Aye! Just flag me if it's ready. :)
Flags: needinfo?(s.kaspari)
Attachment #8836332 - Attachment is obsolete: true
Attachment #8836339 - Attachment is obsolete: true
Comment on attachment 8836335 [details]
Bug 1328868 - Part 4 - Add a new switch offering to honour the system font size to Fennec's preferences.

https://reviewboard.mozilla.org/r/109876/#review115004
Attachment #8836335 - Flags: review?(s.kaspari) → review+
Comment on attachment 8836337 [details]
Bug 1328868 - Part 6 - Remove the old font size preference and its associated code and resources.

https://reviewboard.mozilla.org/r/109880/#review115008
Attachment #8836337 - Flags: review?(s.kaspari) → review+
Can you explain the goal of this bug? It's not clear to me.
I'm not sure why I was asked to review this code since I'm not a layout peer.
On Android, we have only a single - global - user-facing font size setting.
At the moment, this setting is implemented purely by font size inflation, that is changing the setting in the UI manipulates the font.size.inflation.minTwips Gecko pref.

The problem with this is that font size inflation only scales the text on "desktop" pages - those are rendered using a fictious viewport width of 980 px (browser.viewport.desktopWidth) and then scaled down to match the actual device screen width. This preserves the layout, but results in very small text if no further processing is applied. Therefore we offer font inflation, which tries to strike a balance between making the text readable again and not breaking the page layout too much, by - at least in theory - only scaling the main content while leaving any other minor elements alone.

"Mobile-friendly" pages (detected through the appropriate meta viewport tags) on the other hand should normally display at a readable text size by default (otherwise why would the page author have declared them as such?), so we turn off font inflation there and display the page with its original, unmodified layout.

In practice however users might of course want to scale the text size even on "mobile" pages for various reasons, which leaves them with the problem that currently this is not possible using the inbuilt setting, since that one won't have any effect on mobile pages at all.

Hence we want to switch to a model where the font size setting in our settings menu applies to all pages. 
For desktop pages we want to continue using font inflation and just scale minTwips up or down according to the desired zoom factor, whereas for everything else we want to use a plain (text) zoom for scaling, which coincidentally is also the principle used by Chrome on Android.

To implement this, we therefore want to be able to transmit a zoom factor to Gecko that will be applied automatically on all pages - unless font size inflation is enabled for the respective document.
@Bill

I asked you since this comment in prefapi.cpp (https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/modules/libpref/prefapi.cpp#777) said to get review from a DOM peer when touching the whitelist in ContentPrefs.cpp.
(In reply to Jan Henning [:JanH] from comment #47)
> Hence we want to switch to a model where the font size setting in our
> settings menu applies to all pages. 
> For desktop pages we want to continue using font inflation and just scale
> minTwips up or down according to the desired zoom factor, whereas for
> everything else we want to use a plain (text) zoom for scaling, which
> coincidentally is also the principle used by Chrome on Android.

Why not just use text zooming on all pages, on desktop pages it can increase the size of all text (whether font inflated or not)? Does that lead to worse results?
Arguably yes. It's not super dramatic, but I'd still think that
- if the user has indicated to be happy with a smaller font size, I'd find it preferable to use that margin for reducing the amount of font inflation applied and therefore the size differential between inflated and non-inflated text
- conversely if the user wants bigger text, scaling the rest of the page text up as well can eat into the space available for the main page content, since applying text zoom to everything else also makes any footers/headers/sidebars/... larger as well.

Also, it seems like applying text zoom (at least when making things smaller) at the same time as font inflation can result in some additional layout breakage as compared to using either only text zoom or else only font inflation.
Comment on attachment 8836333 [details]
Bug 1328868 - Part 3 - Copy the global zoom state over into new presShell when navigating/reloading.

https://reviewboard.mozilla.org/r/109872/#review115700

Why do we need to record this state on the document? Can't it be computed from information available from the document (ie whether it is marked mobile friendly or not) and preferences?
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review115698

::: layout/base/PresShell.cpp:11158
(Diff revision 3)
> +  docShell->GetContentViewer(getter_AddRefs(cv));
> +  if (!cv) {
> +    return;
> +  }
> +
> +  cv->SetTextZoom(aZoomFactor);

Isn't this just going to overwrite whatever value is in the textzoom?

::: layout/base/nsIPresShell.h:1670
(Diff revision 3)
>  
> +  /**
> +   * If font inflation is not enabled for our document, apply a global
> +   * zoom factor according to the system font scale held in nsLayoutUtils.
> +   */
> +  void HandleGlobalZoom();

We already have at least three completely different notions of "zoom" on a document:

1) full zoom (changing the ratio of css pixels to device pixels
2) text zoom
3) "pinch" zoom from the async pan zoom controller.

Adding another thing called "global zoom" is just going to confuse things ever further. Can we think of a better name for this?
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review115698

> Isn't this just going to overwrite whatever value is in the textzoom?

It is - this was written from the perspective that I think by default Android uses only pinch zoom so overwriting the text zoom wouldn't matter, although thinking about it an add-on could still possibly set these there as well.

Another approach, which I think might also adress your comment on part 3, would be to store the system font scale separately on the presContext and compute a new EffectiveTextZoom = SystemFontScale * TextZoom (clamped by min and max zoom) there.

nsDocument and nsDocumentViewer would continue reporting the plain TextZoom, since they interface with any external consumers that want to play around with the text zoom and are also responsible for the text zoom value that gets transferred over into any new contentViewers/PresContexts/... on navigation.

All other consumers of presContext::TextZoom() that are actually going to use this value for rendering will be switched over to EffectiveTextZoom().

That would mean that we won't clobber the text zoom set by the front-end anymore. Since the font scale on the pres context/content viewer is no longer intermingled with the normal text zoom and also won't be carried over into any new PresShell + PresContext + ..., we also wouldn't have to copy the enabled status across presShells.

If I've figured this out correctly, printing/print preview always clones the document and creates a new PresContext, so it should be possible to exclude this case by checking [the PresContext's type](https://dxr.mozilla.org/mozilla-central/rev/c7b015c488cfb2afbcff295a9639acd85df332f8/layout/printing/nsPrintEngine.cpp#2116).

> We already have at least three completely different notions of "zoom" on a document:
> 
> 1) full zoom (changing the ratio of css pixels to device pixels
> 2) text zoom
> 3) "pinch" zoom from the async pan zoom controller.
> 
> Adding another thing called "global zoom" is just going to confuse things ever further. Can we think of a better name for this?

You're right, I wasn't entirely happy with that naming, but couldn't think of anything better. I'll probably just use system font scale within the PresShell etc. as well.
Comment on attachment 8836333 [details]
Bug 1328868 - Part 3 - Copy the global zoom state over into new presShell when navigating/reloading.

https://reviewboard.mozilla.org/r/109872/#review115700

Unless I'm overlooking something not with the current approach. The problem is that any zoom (text or full) will carry over indefinitely into any following presShells/contexts/contentViewers that get created when navigating, yet on the other hand we've no idea what the global zoom state was on the previous document (was the pref enabled or not, was the document mobile friendly or not).

Since as you pointed out the current implementation clobbers any previously set text zoom, I don't want to touch the text zoom when the system font scale is 1.0, however in order to this, I need to reset the text zoom back to default before I can stop modifying it, which means I need to know not only the current state, but also the previous state as well.

I'll be trying a modified approach, though, which might remove the need for this - see my comment on part 2.
Comment on attachment 8836330 [details]
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.

https://reviewboard.mozilla.org/r/109866/#review115936

::: dom/ipc/ContentPrefs.cpp:268
(Diff revision 3)
>    "ui.key.menuAccessKeyFocuses",
>    "ui.popup.disable_autohide",
>    "ui.use_activity_cursor",
> -  "view_source.editor.external"};
> +  "view_source.editor.external",
> +  "zoom.maxPercent",
> +  "zoom.minPercent",

The patch on bug 1341414 will remove the need for adding these prefs to the white list
Attachment #8836330 - Flags: review?(blassey.bugs) → review-
Comment on attachment 8836330 [details]
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.

https://reviewboard.mozilla.org/r/109866/#review115936

> The patch on bug 1341414 will remove the need for adding these prefs to the white list

Right, thanks for the heads-up. It might also be that I no longer need these two two prefs quite as early and could also turn the font scale pref back into a pref var cache, in which case I won't need to touch the white list at all.
Comment on attachment 8836336 [details]
Bug 1328868 - Part 5 - Add a Java-side listener watching the new pref and the Android system font scale.

https://reviewboard.mozilla.org/r/109878/#review116048

::: mobile/android/base/java/org/mozilla/gecko/GeckoFontScaleListener.java:30
(Diff revision 3)
> +    private static final GeckoFontScaleListener sInstance = new GeckoFontScaleListener();
> +
> +    private Context mApplicationContext;
> +    private boolean mInitialized;
> +    private boolean mRunning;

nit: In most new code we avoid prefixes (m/s).
Attachment #8836336 - Flags: review?(s.kaspari) → review+
Comment on attachment 8836338 [details]
Bug 1328868 - Part 7 - Detect the previous font size setting and migrate it to the new shared preference.

https://reviewboard.mozilla.org/r/109932/#review116050

::: mobile/android/base/java/org/mozilla/gecko/GeckoFontScaleListener.java:126
(Diff revision 3)
> +    private void migrateOldSetting(final SharedPreferences prefs) {
> +        PrefsHelper.getPref(PREF_FONT_INFLATION, new PrefsHelper.PrefHandlerBase() {
> +            @Override
> +            public void prefValue(String pref, int value) {
> +                // The first setting step above "Tiny" used to correspond to a value of 80.
> +                prefs.edit().putBoolean(GeckoPreferences.PREFS_SYSTEM_FONT_SIZE, (value > 60)).apply();
> +            }
> +        });
> +    }

Was this discussed further? I'm a bit worried that this will lead to suprises and users thinking something broke if suddenly in the next release fonts appear different. The previous setting was more or less a random thing from user's point of view. But then again this will only have an effect if those users additionally change the system setting, right?
Attachment #8836338 - Flags: review?(s.kaspari) → review+
Comment on attachment 8836341 [details]
Bug 1328868 - Part 8 - Add a test to check that flipping the Android pref enables/disables font inflation.

https://reviewboard.mozilla.org/r/109938/#review116054
Attachment #8836341 - Flags: review?(s.kaspari) → review+
Comment on attachment 8836342 [details]
Bug 1328868 - Part 9 - Check that zooming via "font.size.systemFontScale" actually has any effect.

https://reviewboard.mozilla.org/r/111624/#review116056
Attachment #8836342 - Flags: review?(s.kaspari) → review+
Comment on attachment 8838851 [details]
Bug 1328868 - Part 10 - Make about:config handle text zoom changes a bit more gracefully.

https://reviewboard.mozilla.org/r/113660/#review116058
Attachment #8838851 - Flags: review?(s.kaspari) → review+
Comment on attachment 8836338 [details]
Bug 1328868 - Part 7 - Detect the previous font size setting and migrate it to the new shared preference.

https://reviewboard.mozilla.org/r/109932/#review116050

> Was this discussed further? I'm a bit worried that this will lead to suprises and users thinking something broke if suddenly in the next release fonts appear different. The previous setting was more or less a random thing from user's point of view. But then again this will only have an effect if those users additionally change the system setting, right?

This'll preserve the fact that font inflation was enabled, although not the magnitued, because that is now governed by the system font scale. Aditionally yes, if the system font scale is set to something other than default all other text will be zoomed in/out as well.
I'm asking antlam for feedback on this...
Attachment #8836338 - Flags: review?(alam)
Comment on attachment 8836336 [details]
Bug 1328868 - Part 5 - Add a Java-side listener watching the new pref and the Android system font scale.

https://reviewboard.mozilla.org/r/109878/#review116048

> nit: In most new code we avoid prefixes (m/s).

I wasn't sure about this either - I think I looked at some recently added files and the result was rather inconclusive if I remember correctly. Then I looked [at MDN](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Java_practices), which currently says to
> Start class variable names with 'm' prefix (e.g. mSomeClassVariable) and static variables with 's' prefix (e.g. sSomeStaticVariable)

The MDN link to our wiki is no longer quite correct, but in any case once find the correct page [it doesn't mention anything to the contrary](https://wiki.mozilla.org/Mobile/Fennec/Android/CommonTips#Coding_Style) and refers back to MDN. And finally, MDN also refers to the [AOSP style guide](http://source.android.com/source/code-style.html#follow-field-naming-conventions), which recommends the use of prefixes as well.

I've got no problem with dropping them if you think that's our way forward, but we should update our documentation in that case in order to reduce the confusion there.
(In reply to Jan Henning [:JanH] from comment #66)
> Another approach, which I think might also adress your comment on part 3,
> would be to store the system font scale separately on the presContext and
> compute a new EffectiveTextZoom = SystemFontScale * TextZoom (clamped by min
> and max zoom) there.

This seems to make more conceptual sense to me.
Attachment #8836333 - Attachment is obsolete: true
Attachment #8836333 - Flags: review?(tnikkel)
Attachment #8836334 - Attachment is obsolete: true
Attachment #8836334 - Flags: review?(tnikkel)
Comment on attachment 8836338 [details]
Bug 1328868 - Part 7 - Detect the previous font size setting and migrate it to the new shared preference.

I'll leave the code review up to Sebastian :)
Attachment #8836338 - Flags: review?(alam)
Well, not a code review per se, but do you have any feedback on the proposed settings migration?
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review123342

::: layout/base/PresShell.cpp:11004
(Diff revision 4)
>  bool
>  nsIPresShell::FontSizeInflationEnabled()
>  {
>    if (mFontSizeInflationEnabledIsDirty) {
>      RecomputeFontSizeInflationEnabled();
> +    HandleSystemFontScale();

Don't we need to call this anytime font inflation can change? ie from RecomputeFontInflationEnabled?

Also, shouldn't we call it from PresShell::Init? To take into account mDocument->IsSyntheticDocument()?

::: layout/base/nsPresContext.h:944
(Diff revision 4)
>  
>    bool IsDynamic() { return (mType == eContext_PageLayout || mType == eContext_Galley); }
>    bool IsScreen() { return (mMedium == nsGkAtoms::screen ||
>                                mType == eContext_PageLayout ||
>                                mType == eContext_PrintPreview); }
> +  bool IsPrintingOrPP() { return (mType == eContext_Print || mType == eContext_PrintPreview); }

Just expand PP, better to be clear.
Comment on attachment 8836330 [details]
Bug 1328868 - Part 1 - Allow setting a global zoom factor via nsLayoutUtils.

https://reviewboard.mozilla.org/r/109866/#review123346
Attachment #8836330 - Flags: review?(tnikkel) → review+
Comment on attachment 8841322 [details]
Bug 1328868 - Part 3 - Make the effective text zoom retrievable from JS.

https://reviewboard.mozilla.org/r/115564/#review123348
Attachment #8841322 - Flags: review?(tnikkel) → review+
So it doesn't have to be a formal review, but do you have any comments on
- the proposed strings for the new settings menu entry?
- the proposed migration from the old to the new setting (anybody who has the current font size set to anything other than "Tiny" gets the new setting turned on)
Flags: needinfo?(alam)
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review123342

> Don't we need to call this anytime font inflation can change? ie from RecomputeFontInflationEnabled?
> 
> Also, shouldn't we call it from PresShell::Init? To take into account mDocument->IsSyntheticDocument()?

This is the only place where RecomputeFontSizeInflationEnabled() is called from, but you're right, the two always go together, so I've slightly rearranged them to make sure they're always called together.

As for PresShell::Init, I don' think so. Every fresh PresContext starts out with the default font scale of 1.0, so if we actually are a synthetic document, that won't change anything. If we aren't, we should still wait until the font inflation state has been properly determined at least once. For this, PresShell::Init() already marks the font inflation state as dirty, so it'll be recomputed the next time it is queried.

> Just expand PP, better to be clear.

Fixed.
Comment on attachment 8836331 [details]
Bug 1328868 - Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated.

https://reviewboard.mozilla.org/r/109868/#review124130
Attachment #8836331 - Flags: review?(tnikkel) → review+
(In reply to Jan Henning [:JanH] from comment #96)
> So it doesn't have to be a formal review, but do you have any comments on

Yes! Sorry I thought you wanted a code review. 

> - the proposed strings for the new settings menu entry?

Since the system mentions "Font size" in their Accessibility settings, I think this makes the most sense:

Use system font size 

> - the proposed migration from the old to the new setting (anybody who has
> the current font size set to anything other than "Tiny" gets the new setting
> turned on)

I think we should just reset this for everyone. That is, OFF by default to avoid confusion.
Flags: needinfo?(alam)
^ thanks JanH! 

(also happy to follow michelle's content suggestions if she has some too :)
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8836335 [details]
Bug 1328868 - Part 4 - Add a new switch offering to honour the system font size to Fennec's preferences.

Question already answered via needinfo.
Attachment #8836335 - Flags: review?(alam)
Attachment #8836338 - Attachment is obsolete: true
Attachment #8836335 - Flags: review?(alam)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/77ec4c9d23c7
Part 0 - Remove unneeded imports. r=esawin
https://hg.mozilla.org/integration/autoland/rev/52d7e9ce0dfb
Part 1 - Allow setting a global zoom factor via nsLayoutUtils. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/345ec75c89d6
Part 2 - Apply the system font scale as an additional text zoom factor to all pages that are not font inflated. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/68a4a734f6e1
Part 3 - Make the effective text zoom retrievable from JS. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/505a6c976a0b
Part 4 - Add a new switch offering to honour the system font size to Fennec's preferences. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/96ad7f172a08
Part 5 - Add a Java-side listener watching the new pref and the Android system font scale. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a20ab74f5f7a
Part 6 - Remove the old font size preference and its associated code and resources. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0fac2d0c6f34
Part 7 - Move MakeObserver() into head.js r=esawin
https://hg.mozilla.org/integration/autoland/rev/e74b4860339d
Part 8 - Add a test to check that flipping the Android pref enables/disables font inflation. r=esawin,sebastian
https://hg.mozilla.org/integration/autoland/rev/3e9fce8066be
Part 9 - Check that zooming via "font.size.systemFontScale" actually has any effect. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/1c0b16d1747c
Part 10 - Make about:config handle text zoom changes a bit more gracefully. r=sebastian
Flags: needinfo?(jh+bugzilla)
Release Note Request (optional, but appreciated)
[Why is this notable]: Old font size setting (which only worked on "desktop" pages) has been replaced by a setting to optionally scale all web content (including mobile-friendly pages) according to Android's global font size setting.
[Affects Firefox for Android]: Only.
[Suggested wording]:
[Links (documentation, blog post, etc)]:
Somehow the flag got reset during editing, so here it goes

Release Note Request (optional, but appreciated)
[Why is this notable]: Old font size setting (which only worked on "desktop" pages) has been replaced by a setting to optionally scale all web content (including mobile-friendly pages) according to Android's global font size setting.
[Affects Firefox for Android]: Only.
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: qe-verify?
Hi Ioana, 
At this moment I'm not sure what the best process to request for QA testing efforts yet so I'll just leave a needinfo directly.
This bug is something we would need QA coverage.
Flags: needinfo?(ioana.chiorean)
any update on QA for this feature? And is this still going to uplift into beta?
(In reply to Nicole Yee (:nicoleyee) from comment #138)
> And is this still going to uplift into beta?

Huh, did I miss something? Any intention to uplift this is certainly news to me.
(In reply to Jan Henning [:JanH] from comment #139)
> (In reply to Nicole Yee (:nicoleyee) from comment #138)
> > And is this still going to uplift into beta?
> 
> Huh, did I miss something? Any intention to uplift this is certainly news to
> me.
No. We don't have plan to uplift to beta. 
Just need the verification from QA to ride on 55.
Hello, I'll take this feature as QA. I'll update this bug in the following days with a Test plan for tracking the testing efforts.
Flags: needinfo?(ioana.chiorean)
QA Contact: oana.horvath
Flags: qe-verify?
Apologies! I meant riding on 55.
I've put together a Test plan: https://goo.gl/xPrASM and some basic test cases: https://goo.gl/r0YPEF for this feature. 
Please have a look and let me know if you have any suggestions or questions.
I started testing and didn't find any issues so far. I will update the test plan with my progress.
We do have the relnote nomination but i'm not sure how the process goes.
I'm leaving a NI to 55 release owner to make sure this will be triaged to get release note or not.
Flags: needinfo?(jcristau)
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Suggested wording for the release notes entry would be helpful, since I'm not familiar with what this all means.  I'll make an initial pass over 55 relnotes nominations next week.
Flags: needinfo?(jcristau)
Joe could you help to draft the wordings?
Flags: needinfo?(jcheng)
For wording, how about, "Font size on web pages now uses system font size settings"
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #147)
> For wording, how about, "Font size on web pages now uses system font size
> settings"

... optionally.
Hi Jan, just to be sure, is this feature currently being pref'd off by default? Since both PM and QA have sign-off the feature we are ready to ride Firefox 55 (current in beta stage), so if it's off would you help turn it on so the end users can get this in 55 release? Thanks!
Flags: needinfo?(jh+bugzilla)
As far as I can tell the old code was removed entirely (https://hg.mozilla.org/integration/autoland/rev/a20ab74f5f7a), and only the new option to use system font size remains (disabled by default).  So there shouldn't be anything to do now.
Yes, this is pre-Dawn, so there's no switch to flip between this and the old font size setting.
Flags: needinfo?(jh+bugzilla)
Thank you Julien and Jan, clear!
(In reply to Jan Henning [:JanH] from comment #148)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #147)
> > For wording, how about, "Font size on web pages now uses system font size
> > settings"
> 
> ... optionally.

Hi Joe, would you help on the release wording from product's point of view? Thanks.
"Font size on web pages now uses system font size settings optionally"
this sounds fine to me
Flags: needinfo?(jcheng)
In that case the "optionally" should probably go in the middle, i.e. like "Font size on web pages now optionally uses system font size settings".
"Added option to accessibility settings to respect the system's set font size when displaying web pages" is in https://www.mozilla.org/en-US/firefox/android/55.0/releasenotes/
relnote-firefox: ? → 55+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: