Fix Dimensions window values on Android < 15

[zoontek]

Summary:

This PR (initially created for edge-to-edge opt-in support, rebased multiple times) fixes the Dimensions API window values on Android < 15, when edge-to-edge is enabled.

Currently the window height doesn't include the status and navigation bar heights (but it does on Android >= 15):

Using WindowMetricsCalculator from AndroidX:

Fixes #47080

Changelog:

[Android] [Fixed] Fix Dimensions window values on Android < 15 when edge-to-edge is enabled

Test Plan:

Run the example app on an Android < 15 device.

[cortinico]

@zoontek can you look into the test failures as they look related?

[cortinico]

Specifically the first one:

RootViewTest > testCheckForKeyboardEvents FAILED
    java.lang.IllegalStateException at DisplayMetricsHolder.kt:36

RootViewTest > testTouchEmitter FAILED
    java.lang.IllegalStateException at RootViewTest.kt:111

372 tests completed, 2 failed

[zoontek]

@cortinico It should fix it.

Thing is, the issue is that we cannot delay display metrics init until we get a non-null Activity instance. But if we keep initDisplayMetricsIfNotInitialized, it happens that the initial called is not an Ui context, then the second is, but the values are not updated (because already init).

So we need to make subsequent calls to initDisplayMetrics, and update window metrics only if within Ui context.

[facebook-github-bot]

@cortinico has imported this pull request. If you are a Meta employee, you can view this in D78738516.

[zoontek]

Hey @cortinico

I wasn't super happy with the first proposed solution, did some extensive testing on Android 7 / 9 / 14, on old and new architecture (zoontek#2)

Like suggested on Discord:

The clean way to do it would be to properly separate the screen metrics and the window metrics init

So:

• I splitted initDisplayMetrics into initScreenDisplayMetrics and initWindowDisplayMetrics
• I splitted initDisplayMetricsIfNotInitialized into initScreenDisplayMetricsIfNotInitialized and initWindowDisplayMetricsIfNotInitialized
• I made sure that initWindowDisplayMetrics and initWindowDisplayMetricsIfNotInitialized are always called with UI context (tested on Android 7 / 9 / 14, with: old & new arch, orientations changes)
• And because it's kinda the same code, I also fixed this issue (just by removing the ReactNativeFeatureFlags.enableFontScaleChangesUpdatingLayout() checks that wrapped init*DisplayMetrics on configuration change)

demo.mp4

[efstathiosntonas]

hey folks, is there any chance this to be backported to 0.79.x? Seems like a big issue in the UI consistency. Thanks!

[alanleedev]

@zoontek Wanted to give you an update. I fixed some internal build errors but see this failing some tests. I haven't got around to look into it yet and want to be more cautious with this diff as past attempts has caused issues for us. Will try to get to this soon.

[facebook-github-bot]

@alanleedev has imported this pull request. If you are a Meta employee, you can view this in D78738516.

[alanleedev]

@zoontek Noticing the implementation changed a bit since we first imported this PR. What is the reason for splitting initScreenDisplayMetrics() and initWindowDisplayMetrics() instead of previous initDisplayMetrics()?

[zoontek]

@alanleedev Since screenDisplayMetrics can be initialized with any type of context, but windowDisplayMetrics now requires a UI context, the initialization of both values may not happen at the same time (calls to initialize windowDisplayMetrics are sometimes wrapped in activity null checks).

Previously, both were initialized in the same method: initDisplayMetrics.

I initially added a new check, isUiContext, but for that to work, I had no choice but to remove initDisplayMetricsIfNotInitialized. That method only checked whether screenDisplayMetrics was set, and calling initDisplayMetrics no longer guaranteed both values would be initialized.

In the end, instead of relying on a suite of conditions, I opted to split the initialization into two separate methods. This makes the code much easier to maintain and reason about.

[alanleedev]

@zoontek That makes sense. I'm going to add some comments around this to make it clear.

Also link to previous comment on why UiContext is needed in case someone is curious.

[alanleedev]

@zoontek I think the current PR adds behavior change which we may not be handled correctly.
One example of crash observed:

08-04 06:18:08.062 7793 7836 E AndroidRuntime: java.lang.IllegalStateException: DisplayMetricsHolder must be initialized with initWindowDisplayMetricsIfNotInitialized or initWindowDisplayMetrics
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.uimanager.DisplayMetricsHolder.getWindowDisplayMetrics(DisplayMetricsHolder.kt:37)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.uimanager.PixelUtil.getDisplayMetricDensity(PixelUtil.kt:69)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.fabric.FabricUIManagerBinding.register(FabricUIManagerBinding.kt:96)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.fabric.FabricUIManagerProviderImpl.createUIManager(FabricUIManagerProviderImpl.kt:56)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.panelapp.ReactNativeHost$getUIManagerProvider$1.createUIManager(ReactNativeHost.kt:156)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.ReactInstanceManager.createReactContext(ReactInstanceManager.java:1518)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.ReactInstanceManager.lambda$runCreateReactContextOnNewThread$2$com-facebook-react-ReactInstanceManager(ReactInstanceManager.java:1233)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at com.facebook.react.ReactInstanceManager$$ExternalSyntheticLambda1.run(D8$$SyntheticClass:0)
08-04 06:18:08.062 7793 7836 E AndroidRuntime: 	at java.lang.Thread.run(Thread.java:1012)

[zoontek]

@alanleedev I think that some initWindowDisplayMetricsIfNotInitialized needs to be added / moved. Currently, they are next to initScreenDisplayMetricsIfNotInitialized.

Let me have a look. Do you know how can I reproduce?

[alanleedev]

@zoontek It is with internal Meta app so you probably cannot repro. I'll have further look into this.
One concern with PixelUtil is that where we were calling initMetrics which we only can call initScreenMetrics then this could be a potential issue as PixelUtil could be called without proper initialization. Wondering if there is a better way to handle this.

[zoontek]

@alanleedev If an additional initWindowDisplayMetricsIfNotInitialized is added immediately on Activity creation, whouldn't it solves the issue?

[alanleedev]

@alanleedev If an additional initWindowDisplayMetricsIfNotInitialized is added immediately on Activity creation, whouldn't it solves the issue?

@zoontek App in question is partially RN and uses Fragments so will need to understand how things get initialized. But overall, PixelUtil has an unclear initialization dependency on DiplayMetricsHolder.windowDisplayMetrics which does not seem too desirable so wondering if there is a better way to handle this.

[zoontek]

@alanleedev Nearly all calls to getWindowDisplayMetrics could be replaced with getScreenDisplayMetrics, as what matters is those contexts are scale / fontScale / densityDpi, not height and width (and those values are similar on both screen and window metrics).

The only valid usage is located in ReactRootView.java.
I can push the change if you want.

[alanleedev]

@alanleedev Nearly all calls to getWindowDisplayMetrics could be replaced with getScreenDisplayMetrics, as what matters is those contexts are scale / fontScale / densityDpi, not height and width (and those values are similar on both screen and window metrics).

@zoontek That is a good point. Looking at TypedValue.java following seems to be used;
metrics.density
metrics.fontScaleConverter
metrics.scaledDensity
metrics.xdpi

If you could post an update that would be great.
Actually, I'll do the update as I need to make also internal code changes anyway and need to re-apply additional internal changes if I re-import the PR.

[facebook-github-bot]

@alanleedev merged this pull request in 3b185e4.

[facebook-github-bot]

This pull request has been reverted by 352e440.