-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Dimensions not updating on Android #31973
Conversation
Hi @jonnyandrew! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
Base commit: fcead14 |
Base commit: fcead14 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
333b832
to
d638ea4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
google-java-format
found some issues. See https://github.com/google/google-java-format
d638ea4
to
3a87a4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it seems we have a lot of unused code in DisplayMetrics. I'm wondering if you've tested as well with split screen and verified there are no changes with that behavior.
@@ -85,8 +90,7 @@ public void emitUpdateDimensionsEvent() { | |||
|
|||
if (mReactApplicationContext.hasActiveReactInstance()) { | |||
// Don't emit an event to JS if the dimensions haven't changed | |||
WritableNativeMap displayMetrics = | |||
DisplayMetricsHolder.getDisplayMetricsNativeMap(mFontScale); | |||
WritableMap displayMetrics = DisplayMetricsHolder.getDisplayMetricsWritableMap(mFontScale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the logic behind WritableNativeMap
and WritableMap
? Sorry I'm not familiar with the difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WritableNativeMap
is an implementation of the WritableMap
interface (and that is the implementation that is returned from DisplayMetricsHolder
). But in Java-only test code, WritableNativeMap
is not supported and needs to be switched out for JavaOnlyMap
. So in order to test this class, it needs to depend on WritableMap
instead.
Thanks for the review @lunaleaps! I have not tested in split screen but I will take a look. And if you have any particular split screen test cases in mind, please let me know. |
@lunaleaps I have run a few manual tests in split screen mode as requested, and I did not see any change in behaviour (except this fix as expected).
I ran the tests on a Pixel 3a emulator with Android 10. Let me know if you'd like me to post the video recordings. |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@lunaleaps merged this pull request in c18a492. |
Summary: When retrieving the device dimensions through the JS `Dimensions` utility, the result of `Dimensions.get` can be incorrect on Android. - facebook#29105 - facebook#29451 - facebook#29323 The issue is caused by the Android `DeviceInfoModule` that provides initial screen dimensions and then subsequently updates those by emitting `didUpdateDimensions` events. The assumption in that implementation is that the initial display metrics will not have changed prior to the first check for updated metrics. However that is not the case as the device may be rotated (as shown in the attached video). The solution in this PR is to keep track of the initial dimensions for comparison at the first check for updated metrics. [Android] [Fixed] - Fix Dimensions not updating Pull Request resolved: facebook#31973 Test Plan: 1. Install the RNTester app on Android from the `main` branch. 2. Set the device auto-rotation to ON 3. Start the RNTester app 4. While the app is loading, rotate the device 5. Navigate to the `Dimensions` screen 6. Either a. Observe the screen width and height are reversed, or b. Quit the app and return to step 3. Using the above steps, the issue should no longer be reproducible. See unit tests in `ReactAndroid/src/test/java/com/facebook/react/modules/deviceinfo/DeviceInfoModuleTest.java` https://user-images.githubusercontent.com/4940864/128485453-2ae04724-4ac5-4267-a59a-140cc3af626b.mp4 Reviewed By: JoshuaGross Differential Revision: D30319919 Pulled By: lunaleaps fbshipit-source-id: 52a2faeafc522b1c2a196ca40357027eafa1a84b
Summary: When retrieving the device dimensions through the JS `Dimensions` utility, the result of `Dimensions.get` can be incorrect on Android. - facebook#29105 - facebook#29451 - facebook#29323 The issue is caused by the Android `DeviceInfoModule` that provides initial screen dimensions and then subsequently updates those by emitting `didUpdateDimensions` events. The assumption in that implementation is that the initial display metrics will not have changed prior to the first check for updated metrics. However that is not the case as the device may be rotated (as shown in the attached video). The solution in this PR is to keep track of the initial dimensions for comparison at the first check for updated metrics. [Android] [Fixed] - Fix Dimensions not updating Pull Request resolved: facebook#31973 Test Plan: 1. Install the RNTester app on Android from the `main` branch. 2. Set the device auto-rotation to ON 3. Start the RNTester app 4. While the app is loading, rotate the device 5. Navigate to the `Dimensions` screen 6. Either a. Observe the screen width and height are reversed, or b. Quit the app and return to step 3. Using the above steps, the issue should no longer be reproducible. See unit tests in `ReactAndroid/src/test/java/com/facebook/react/modules/deviceinfo/DeviceInfoModuleTest.java` https://user-images.githubusercontent.com/4940864/128485453-2ae04724-4ac5-4267-a59a-140cc3af626b.mp4 Reviewed By: JoshuaGross Differential Revision: D30319919 Pulled By: lunaleaps fbshipit-source-id: 52a2faeafc522b1c2a196ca40357027eafa1a84b
Summary
When retrieving the device dimensions through the JS
Dimensions
utility, the result ofDimensions.get
can be incorrect on Android.Related issues
The issue is caused by the Android
DeviceInfoModule
that provides initial screen dimensions and then subsequently updates those by emittingdidUpdateDimensions
events. The assumption in that implementation is that the initial display metrics will not have changed prior to the first check for updated metrics. However that is not the case as the device may be rotated (as shown in the attached video).The solution in this PR is to keep track of the initial dimensions for comparison at the first check for updated metrics.
Changelog
[Android] [Fixed] - Fix Dimensions not updating
Test Plan
Steps to reproduce
main
branch.Dimensions
screena. Observe the screen width and height are reversed, or
b. Quit the app and return to step 3.
Verifying the fix
Manually
Using the above steps, the issue should no longer be reproducible.
Automatically
See unit tests in
ReactAndroid/src/test/java/com/facebook/react/modules/deviceinfo/DeviceInfoModuleTest.java
Video
Android-dimensions-issue.mp4