fix: make RCTScreenSize take horizontal orientation into account #51444
Conversation
facebook-github-bot
added
CLA Signed
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Hi @okwasniewski, thanks for the PR. I left a comment that I'd like you to address!
okwasniewski
force-pushed
the
fix/horizontal-size
branch
from
bd1b325 to
e7a15db
Compare
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| RCTUnsafeExecuteOnMainQueueSync(^{ | ||
| size = [UIScreen mainScreen].bounds.size; | ||
| }); | ||
| }); | ||
|
|
||
| return size; | ||
|
|
||
| UIDeviceOrientation orientation = RCTDeviceOrientation(); | ||
|
|
||
| BOOL isLandscape = orientation == UIDeviceOrientationLandscapeLeft || orientation == UIDeviceOrientationLandscapeRight; | ||
|
|
||
| if (isLandscape) { | ||
| return CGSizeMake(size.height, size.width); | ||
| } else { | ||
| return CGSizeMake(size.width, size.height); | ||
| } |
There was a problem hiding this comment.
I'm not really sure whether this block of code works fine in all the situations.
When the app is opened in landscape mode, the dispatch_once will run immediately, and we are going to get a height < width... 🤔
But we are returning a cg size that says that the width of the CGSize is the height of the screen, so the width of the CGSize is the small side... I'm not sure that's correct...
There was a problem hiding this comment.
Ah.. yea that's true, what do you think about getting "absolute" value in the RCTUnsafeExecuteOnMainQueueSync so if it's not portrait we reverse it? Or how would you approach it?
There was a problem hiding this comment.
Yeah, I think it makes sense.
I think that we should check for the orientation also in the RCTUnsafeExecuteOnMainQueueSync and then track what's the "long side" and what's the "short side" and then return the CGSize accordingly.
as a nit, ObjC has a method UIDeviceOrientationIsLandscape(UIDeviceOrientation orientation) we should use, instead of checking manually orientation == UIDeviceOrientationLandscapeLeft || UIDeviceOrientationLandscapeRight.
If we use it, we can probably do something like:
if (UIDeviceOrientationIsLandscape(RCTDeviceOrientation())) {
and get rid of the UIDeviceOrientation orientation variable
There was a problem hiding this comment.
@cipolleschi Thanks for the suggestion, I didn't know about UIDeviceOrientationIsLandscape. Pushed a fix!
okwasniewski
force-pushed
the
fix/horizontal-size
branch
from
e7a15db to
afa1b0a
Compare
|
@RSNara were you looking at a replacement for this to avoid the UI thread dispatch? |
| return size; | ||
|
|
||
| if (UIDeviceOrientationIsLandscape(RCTDeviceOrientation())) { | ||
| return CGSizeMake(portraitSize.height, portraitSize.width); |
There was a problem hiding this comment.
I thought we were dispatching sync to avoid calling functions that required being on the UI thread? Is this safe?
| // FIXME: this caches whatever the bounds were when it was first called, and then | ||
| // doesn't update when the device is rotated. We need to find another thread- | ||
| // safe way to get the screen size. |
There was a problem hiding this comment.
Does screen size change if we have something insetting the window? Like, a keyboard open, or if a window on an iPad is resized?
I agree with the comment, that this is pretty sketchy 😀
|
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
This PR improves
RCTScreenSizeto handle horizontal orientations. This was causing a flicker whenever opening a Modal in horizontal orientation. Currently,RCTScreenSizeis used to supply initial state forModalHostViewState:react-native/packages/react-native/ReactCommon/react/renderer/components/modal/ModalHostViewState.h
Line 31 in f169754
This works great in portrait mode, but causes onLayout the be called twice in horizontal orientation (first with screen size and then with actual size..)
Changelog:
[IOS] [FIXED] - make RCTScreenSize take horizontal orientation into account
Test Plan:
Open modal in horizontal orientation