Skip to content
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

Update window size when the in-call status bar is toggled #19919

Closed

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jun 27, 2018

When the in-call status bar is toggled, the Dimensions module window doesn't resize properly. The current code assumes that window and screen is always the same on iOS but it is not the case for when the in-call status bar is on. The viewport size is reduced by 20px in that case.

This results in the following behaviour changes:

  • The Dimensions module change event is now triggered when the in-call status bar is toggled
  • The window and screen values of the Dimensions module are not always the same on iOS, currently only different when the in-call status bar is opened.
  • The Dimensions module change event is now triggered when the status bar is shown / hidden. Right now this is not ideal but I'm planning to add safeAreaInsets to the Dimensions module and in that case the change event will be meaningful.

RCTKeyWindow().rootViewController.view.bounds seemed the best way I found to get the proper value for when the in-call status bar is on. Might want to verify this also work properly in non-pure RN apps.

Test Plan:

  • Tested that a view that has exactly the size of the window (using Dimensions.get('window') still fits properly when the in-call status bar is on.
  • Tested that the Dimensions module values are still updated properly on screen rotation (and only once). Also works if the status bar is hidden.

Release Notes:

[IOS] [BREAKING] [Dimensions] - Update window size when the in-call status bar is toggled

@janicduplessis janicduplessis requested a review from shergin as a code owner June 27, 2018 03:22
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 27, 2018
@janicduplessis janicduplessis added the Platform: iOS iOS applications. label Jun 27, 2018
@vovkasm
Copy link
Contributor

vovkasm commented Jul 1, 2018

But in-call status bar does not reduces window height. Same as normal status bar, it draws over application window, but not changes it's size. References:

@janicduplessis
Copy link
Contributor Author

@vovkasm I haven’t tested on all iOS versions but on iOS 11 it does resize the window by 20pt when the in call status bar is toggled. I assume apple did this to make it easier to manage since you can assume the status bar always covers 20pt at the top of the screen, instead of having to consider that it sometimes cover 40pt when the in call status bar is on.

@vovkasm
Copy link
Contributor

vovkasm commented Jul 1, 2018

@janicduplessis I just did some tests:

  1. Normal status bar (we can see normal UIWindow size, and normal root view, that covers whole UIWindow):
    Normal status bar

  2. In-call status bar (we still see normal UIWindow size, but UIViewController automatically adjust constraints of its view)
    In-call status bar

I'm as a developer can set any constraints in UIViewController and make RCTRootView do not relayout on in-call status bar. Right?

@vovkasm
Copy link
Contributor

vovkasm commented Jul 1, 2018

I think what you want to use Dimensions for, can be easily implemented by onLayout props of root view, or its child with felx: 1 style. Window, screen and root view can all be different in size and we need all these metrics.

@janicduplessis
Copy link
Contributor Author

With the default RN setup, window size should be the same as the rootview. You can always get the real screen size using “screen”. In this case I am just measuring the root VC so if you decide to remove thr default 20 top constraint it would still return the right value.

@vovkasm
Copy link
Contributor

vovkasm commented Jul 1, 2018

With the default RN setup, window size should be the same as the rootview.

  1. Why it should? ))) It is not the case currently (as we can see above) and if this should be fixed, then it should be fixed by instantiate custom VC (say RCTRootViewController) instead of pristine UIViewController that will implement proper layout logic for its root view.

  2. The default RN setup - not the only one used.

You can always get the real screen size using “screen”.

Screen - is totally different thing... Do not mix it with window please.

  • Screen size - should be constant for main screen of application. And can change only if application needs to manage distinct view on other screen (as I know RN currently can't deal with this without native coding)
  • Window size - is the area where an application can draw. In split view mode on iPad it will differ from screen size

And again, why you can't get size of root view with onLayout event? It will work always - for rotations, for split view, for custom layout logic in custom view controller, etc...

@facebook-github-bot
Copy link
Contributor

@janicduplessis I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@janicduplessis
Copy link
Contributor Author

Alright, let's leave this as is. I'm working on a new api to get the root view layout metrics instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications. Type: Breaking Change💥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants