BREAKING: Android: Correct value of Dimensions.get('screen').fontScale

[rigdern]

The PR description has been updated to reflect the new approach.

Breaking Change Summary

On Android, the following properties now return a different number:

• Dimensions.get('window').fontScale
• Dimensions.get('screen').fontScale
• PixelRatio.getFontScale()

This is a breaking change to anyone who was using these properties because the meaning of these properties has now changed.

These properties used to return a value representing font scale times density (DisplayMetrics.scaledDensity). Now they return a value representing just font scale (Configuration.fontScale).

PR Description

This PR changes a few things:

• Correctly exposes the font scale to JavaScript as Dimensions.get('screen').fontScale. UIManager was exporting DisplayMetrics.scaledDensity under the name fontScale. However, this isn't correct because scaledDensity is actually density * fontScale. We now correctly export just fontScale under the name fontScale.
• didUpdateDimensions now fires whenever fontScale changes.
• Dimensions now fires a change event whenever a property within the Dimensions object changes. The primary benefit of this event over didUpdateDimensions is that this event provides an event object which has the same shape on iOS and Android. The event object for didUpdateDimensions uses different properties names on iOS than it does on Android.

Test plan (required)

Verified that the API works for different font settings in a test app. Also, verified that the didUpdateDimensions and change events fire with the proper information when changing the font settings and when rotating the device.

Adam Comella
Microsoft Corp.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @astreet and @AaaChiuuu to be potential reviewers.

[mkonicek]

Since it looks like the method and event deal purely with fonts, what do you think of the following naming?

getContentSizeMultiplier ->getFontScale
didUpdateContentSizeMultiplier -> didUpdateFontScale

But I see on iOS this reads bridge.accessibilityManager.multiplier so maybe it's related to other things than fonts and it makes sense to keep the name more generic?

Should the method and event be declared and documented in a JS file somewhere?

[astreet]

1. Just fyi, the activity will not restart if you've set android:configChanges="fontScale". I don't think that's very common though
2. If we did have a listener, though, we could just send this down in the initial UIManager constants instead of exposing a getter.

[rigdern]

@mkonicek

Should the method and event be declared and documented in a JS file somewhere?

Functionality-wise, everything works fine without declaring the method or event in a JS file. UIManager.js adds most of its methods automatically based on what was exported from native.

You raise a good point about documentation. Do you have any suggestions of where to document it? It doesn't look like there's any API reference page for UIManager on the website. There are at least a couple of other UIManager methods that aren't well documented:

• sendAccessibilityEvent is documented with a snippet on the website but doesn't have any API reference.
• takeSnapshot appears in a UIExplorer example but isn't mentioned on the website.

Perhaps the same solution should be used for documenting all of these UIManager methods.

Since it looks like the method and event deal purely with fonts, what do you think of the following naming?

getContentSizeMultiplier ->getFontScale
didUpdateContentSizeMultiplier -> didUpdateFontScale

But I see on iOS this reads bridge.accessibilityManager.multiplier so maybe it's related to other things than fonts and it makes sense to keep the name more generic?

I don't have a strong preference here.

I'm not an expert on the subject but looking at the iOS and Android docs, the native sources of getContentSizeMultiplier appear to be about font scaling:

• Android's fontScale property
• iOS's preferredContentSizeCategory property

Since this appears to be about font scaling, I can see why the name getFontScale makes sense. Here are some reasons getContentSizeMultiplier makes sense:

• Font scaling happens automatically by RN so the reason an app developer would want to get at this value would be to manually scale their content (e.g. view sizes) to look better with the automatically scaled text.
• iOS picked the name preferredContentSizeCategory which is named after content size rather than font scaling (although the docs indicate it's about font scaling).

If you want me to rename it to getFontScale, let me know. Note that my PR for getContentSizeMultiplier was already merged on iOS. Is it the case that we can rename it as long as it hasn't yet made it into any releases? What about releases vs RCs?

Confusingly, RN Android already exposes a fontScale property off of Dimensions. It's based on Android's scaledDensity property. Reading the docs, I'm not entirely sure what it means. In testing, it was giving a different value than getContentSizeMultiplier and getContentSizeMultiplier's result was preferable.

[rigdern]

@astreet

1. Just fyi, the activity will not restart if you've set android:configChanges="fontScale". I don't think that's very common though
2. If we did have a listener, though, we could just send this down in the initial UIManager constants instead of exposing a getter.

Do you propose we do anything for the android:configChanges="fontScale" case? You mentioned it's uncommon and my team doesn't use that configuration so I'm happy to ignore the case until somebody needs it.

For (2), do you propose we expose that value to JS through Dimensions like RN does for width and height?

As I mentioned to Martin, I discovered that RN Android already exposes a fontScale property off of Dimensions. Do you know what that fontScale property is for and how it relates to the value in the getContentSizeMultiplier API I'm adding?

Confusingly, RN Android already exposes a fontScale property off of Dimensions. It's based on Android's scaledDensity property. Reading the docs, I'm not entirely sure what it means. In testing, it was giving a different value than getContentSizeMultiplier and getContentSizeMultiplier's result was preferable.

[astreet]

Whoops, I think you found a bug/misnaming in RN: According to http://androidxref.com/4.2_r1/xref/packages/apps/Settings/src/com/android/settings/Display.java#99, scaledDensity which we confusingly expose here

react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java

Line 111 in abb8ea3

"fontScale",

as fontScale, is actually fontScale * density. But it seems to be exposed and used as an actual fontScale in our JS code? I'm pretty confused, this seems very clowny as it would indicate some things using this weren't very well tested :P @foghina, am I reading this wrong?

[mkonicek]

You raise a good point about documentation. Do you have any suggestions of where to document it? It doesn't look like there's any API reference page for UIManager on the website. There are at least a couple of other UIManager methods that aren't well documented:

I agree, I checked the website before asking the original question and didn't see any docs for UIManager :( http://facebook.github.io/react-native/docs/getting-started.html

We do have a UIManager.js though, would it make sense to add the function there and document it? https://github.com/facebook/react-native/blob/master/Libraries/ReactNative/UIManager.js

Here are some reasons getContentSizeMultiplier makes sense:

Good points, let's keep getContentSizeMultiplier.

Confusingly, RN Android already exposes a fontScale property off of Dimensions.

Thanks for noticing this! It might be wrong and maybe we should deprecate it as part of this PR?
To make it more confusing we also have PixelRatio.getFontScale. By grepping the fb codebase I see only one place that uses this. Git blame suggests @foghina, I pinged him on messenger asking to comment here.

[foghina]

Ok. So.

1. I don't think we should add an event for when the font scale changes. Using android:configChanges="fontScale" is untested and I have a hunch it wouldn't really work, since we'd need to do a layout pass when it happens.
2. As @astreet pointed out, scaledDensity = density * fontScale. The reason why I exposed scaledDensity is because this is what the system uses when calculating the pixel value of an sp unit. To demonstrate, I've created a sample app with a TextView with textSize="18sp", then got the pixel value via TextView#getTextSize(). As you can see, the pixel value is always 18 * scaledDensity: http://imgur.com/a/DJtnG. I think the initial use case for this constant was to manually calculate pixel values, so scaledDensity made sense. Not sure where fontScale would be useful.

As for getContentSizeMultiplier, it's not clear to me what the use cases for it are. Sounds like you can easily calculate the fontScale by using the density and scaledDensity, which you can get from PixelRatio / Dimensions. IMO iOS should also expose it the same way, we should refrain from adding random getters on UIManagerModule.

[astreet]

Ok, thanks Felix. I don't think we should be doing anything new then, except for maybe renaming fontScale to something more accurate (this would be breaking though) or exposing a convenience method to PixelRatio.

[rigdern]

@foghina @astreet

You've convinced me that we don't need to add getContentSizeMultiplier to UIManager because Android already has APIs for extracting this value. Should we revert eddc2c9 which added this API to iOS?

As Felix mentioned, on Android we can calculate fontScale as scaledDensity / density. Our use case for fontScale is to adjust how things are sized to match scaled fonts. RN's size props (e.g. width, padding) assume the unit is dp. We multiply a size by fontScale in JS so that it ends up being treated as though its unit was sp by native. What is a use case for scaledDensity in JS? I imagine this is only useful if RN native code won't be converting from dp to pixels for you.

For iOS, we could expose density and scaledDensity just like RN Android does. Does anyone know what iOS APIs give the equivalent of Android's density and scaledDensity?

@foghina, you mentioned you don't think we should add random getters to UIManager. In general, should apps be directly calling any UIManager APIs from JS? If not:

• What's the reasoning?
• Earlier in this issue, I mentioned to Martin that UIManager has some APIs that aren't well documented (e.g. sendAccessibilityEvent and takeSnapshot). We were thinking about how to document them better. Instead, maybe they should be exposed to JS elsewhere.

[astreet]

Yeah, I would say we should revert eddc2c9. I'm not sure about the iOS APIs -- do they even have system-wide font scaling?

It also looks to me like our one internal use of getFontScale is incorrect: const adaptiveFontSize = Math.min(12 * PixelRatio.getFontScale(), 22); seems to not realize that getFontScale also includes density (which will be applied on the native side already)... and really that's not even correct because we will also apply the scaling factor automatically on the native side. I think we should actually just change the fontScale that's exported from UIManager to be the actual font scale and not the scaled density. I agree that scaled density doesn't seem useful.

[rigdern]

I'm not sure about the iOS APIs -- do they even have system-wide font scaling?

Yes, they do. eddc2c9 exposed the current font scale to JS.

---

Currently, Android exposes DisplayMetrics.density as scale and DisplayMetrics.scaledDensity as fontScale.

@astreet @foghina What do you think of this proposal:

• On Android:
  • Modify UIManager so that fontScale is DisplayMetrics.scaledDensity / DisplayMetrics.density instead of just DisplayMetrics.scaledDensity.
• On iOS:
  • Remove getContentSizeMultiplier from iOS by reverting eddc2c9
  • Expose a UIManager fontScale constant which contains the value getContentSizeMultiplier returned.
  • When the user changes the font scaling setting, fire didUpdateDimensions.
  • Don't expose the scale constant because we don't know how to get this value on iOS and we don't have an immediate need for it.

One question: do we need to make any changes to UIManager's scale constant on Android? Is its name still suitable?

[foghina]

That sounds like a breaking change on Android, so I'm not really in love with it. However, I did some grepping internally and I think we only have one place where we use this, so it should be easy to fix. I'd say let's go ahead with this plan.

Also, I just realized scaledDensity is exposed in two constants in UIManager: windowPhysicalPixels.fontScale and screenPhysicalPixels.fontScale. I'm guessing this is also iOS-inspired? Could we clean it up?

Expose a UIManager fontScale constant which contains the value getContentSizeMultiplier returned.

Can you change constants on iOS? You can't on Android (without restarting the bridge). I'm asking because we should stick with the getter if we can't change the constant. We don't want to end up with didUpdateDimensions firing and the constant still having the old value.

Last question: what number does iOS return from getContentSizeMultiplier? Is it similar to fontScale? Could you use it in the same way on both platforms?

[astreet]

That sounds like a breaking change on Android, so I'm not really in love with it. However, I did some grepping internally and I think we only have one place where we use this, so it should be easy to fix. I'd say let's go ahead with this plan.

I only saw that one place too and it should really just be removed: we already factor system font scale on the native side by converting from SP so they're double-factoring font scale.

@rigdern changing density is a much riskier change than changing fontScale -- density is used a ton of places so I think we should just not touch that.

Also, just expose getResources().getConfiguration().fontScale (instead of calculating it as you mentioned).

On iOS: When the user changes the font scaling setting, fire didUpdateDimensions

I'm less sure about the impacts of this change... would it cause us to do a re-layout? Should probably get @javache in on this.

[rigdern]

@foghina

Also, I just realized scaledDensity is exposed in two constants in UIManager: windowPhysicalPixels.fontScale and screenPhysicalPixels.fontScale. I'm guessing this is also iOS-inspired? Could we clean it up?

Why do you think this is iOS-inspired? Looking at the code, iOS only exposes window dimensions whereas Android exposes both window and screen dimensions. Do you have a suggestion for how this could be cleaned up?

Can you change constants on iOS? You can't on Android (without restarting the bridge). I'm asking because we should stick with the getter if we can't change the constant. We don't want to end up with didUpdateDimensions firing and the constant still having the old value.

I don't think you can change constants on iOS but I don't think this is important in this case because it looks like apps don't use these constants directly. Instead, they are accessed through the Dimensions object which gets updated whenever the dimensions change. We can update Dimensions whenever the fontScale changes as well.

Last question: what number does iOS return from getContentSizeMultiplier? Is it similar to fontScale? Could you use it in the same way on both platforms?

If by fontScale you mean Android's getResources().getConfiguration().fontScale then, yes, empirically they seem to be the same. My team has been using getResources().getConfiguration().fontScale in our Android app and getContentSizeMultiplier in our iOS app. I do not understand where the values for getContentSizeMultiplier come from on iOS. They are hard coded values based on the preferredContentSizeCategory

@astreet

@rigdern changing density is a much riskier change than changing fontScale -- density is used a ton of places so I think we should just not touch that.

Okay, I just noticed that iOS already exports a density value under the name scale so that's good.

On iOS: When the user changes the font scaling setting, fire didUpdateDimensions

I'm less sure about the impacts of this change... would it cause us to do a re-layout? Should probably get @javache in on this.

Here are the options I have in mind for notifying the app that fontScale has changed:

• Fire existing event didUpdateDimensions. Downside: this is an existing event. Apps might assume that when this event is fired, either width or height has changed and do something expensive in response. Firing it when fontScale has changed would break this assumption. (On a positive note, I did a search for didUpdateDimensions and it appears the RN framework itself doesn't do anything expensive.)
• Fire new event didUpdateFontScale.

Note that apps might access fontScale through Dimensions.get('window').fontScale and PixelRation.getFontScale().

I think didUpdateFontScale is the better option. Reusing didUpdateDimensions was tempting because both dimensions and fontScale happen to be bundled under the Dimensions object. However, I think it makes sense that didUpdateDimensions fires for width/height changes and didUpdateFontScale fires for fontScale changes. Also, I'm not sure that it'd be common to want to handle both of these cases together which lessens the appeal of bundling them in a didUpdateDimensions event.

[astreet]

I'm fine with creating a new event. Just making sure, updating the font scale causes us to perform a re-layout, right?

[rigdern]

Sorry for the delay on this.

Just making sure, updating the font scale causes us to perform a re-layout, right?

@astreet Yes. On iOS, text is marked as dirty and relaid out. On Android, the root view is destroyed and recreated.

I ran into some issues trying to implement this on Android:

1. Updating JS fontScale value in response to system setting change. After changing the system's font scale setting, the activity is destroyed and recreated. However, this does not result in the native constants being rexported so Dimensions.get('window').fontScale still refers to the initial value. I can fire the JS event didUpdateFontScale to update JS however I don't know what native event UIManager should listen to for this. Any ideas?
   Also, we'll probably end up 2 doing big rerenders. One for recreating the root view and one for JS code in the app that is listening to didUpdateFontScale. I'm not sure how to avoid 2 rerenders.

2. Exposing getResources().getConfiguration().fontScale. @astreet, you suggested exposing fontScale as getResources().getConfiguration().fontScale rather than calculating it from DisplayMetrics.scaledDensity / DisplayMetrics.density. However, there are 2 different fontScales exposed: one for windowPhysicalPixels and one for screenPhysicalPixels. Which one does getResources().getConfiguration().fontScale refer to and how do I calculate the other one?

3. window vs screen display metrics. The RN Java code says that getWindowDisplayMetrics is deprecated but PixelRatio.getFontScale() uses the window display metrics (Dimensions.get('window')) rather than the screen display metrics (Dimensions.get('screen')). Even more confusing, I think iOS doesn't even expose Dimensions.get('screen') which is supposedly recommended over the deprecated Dimensions.get('window').

[rigdern]

@astreet do you have any thoughts on my previous comment?

[astreet]

Updating JS fontScale value in response to system setting change.

Ah, that's a good point, you can listen from onConfigurationChanged.

Exposing getResources().getConfiguration().fontScale

The font scale for the window and the screen should be the same. Or am I misunderstanding you?

window vs screen display metrics

Same answer as above: the only reason window is deprecated is for consistency with iOS in width/height people are using (afaik), either will give the correct fontScale though

[rigdern]

@astreet

Thanks, I will try using onConfigurationChanged.

Regarding window vs screen, you answered my question. I wasn't aware of the difference between window and screen and now I know there is no difference as far as font scale goes.

I'm confused about the deprecation of window. The comment says window is deprecated but iOS only provides window. Should iOS start providing screen and then we can stop referencing window within the RN codebase?

[andreicoman11]

Re: window vs screen.

window is indeed deprecated and we should be using screen everywhere.
On iOS screen and window are the same. Native provides only window, but we actually expose that data as both window and screen because of this: https://github.com/facebook/react-native/blob/master/Libraries/Utilities/Dimensions.js#L56. Ideally, we should be providing everything directly as screen and removing window. Basically, what you mentioned.
On Android, window and screen are different, but both are provided. Eventually we should be not providing window at all and using screen everywhere.

[rigdern]

@andreicoman11 I see, thanks for the explanation. I suspect the code you linked to is not running on iOS. The reason is that the code is within an if statement checking for dim.windowPhysicalPixels. However, iOS doesn't export its dimensions under the property windowPhysicalPixels. Instead, iOS exports them as window.

So based on code inspection, I don't think iOS currently supports screen at all.

[rigdern]

@astreet I did all of the changes we discussed. I rebased this on top of my commit for breaking dependency cycles and I added a change event to Dimensions.

[astreet]

Can we add tests for this? This is such an infrequent event that I'm worried we might break it accidentally and not realize it.

[facebook-github-bot]

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[rigdern]

@astreet Tests for which event? didUpdateDimensions? The change event for the Dimensions object?

Normally, firing these events requires rotating the device or changing the font scale settings. How do you propose to test it? For change, I could call Dimensions.set() and very that change fires.

[astreet]

This would be a test to make sure that when we detect a font scale change, that JS receives an update and triggers listeners. We have instrumentation tests that test JS and native code together here: https://github.com/facebook/react-native/tree/master/ReactAndroid/src/androidTest/java/com/facebook/react/tests.

Simulating a configuration such that it changes between onResume calls might be difficult: since it's running as an actual Application on an actual device (so that we can test JS), we can't do standard mocking of static functions with PowerMockito afaik. An idea that comes to mind is creating a setter function on UIManager, but it's not great. Let me know if you have any ideas. Really sketchy other idea: fontScale on Configuration isn't final so you might be able to just manually set it...