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

New LayoutContext API to access the frame and safe area of the root view #20999

Closed
wants to merge 10 commits into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Sep 6, 2018

Motivation:

There is currently no way to access layout metrics from the root view that contains the component tree. This can be useful when you need to access these values from JS synchronously without having to rely on the onLayout event of a top level view which is async and can cause flickers.

It is roughly the equivalent of the Dimensions module for screen / window size but instead based on the RootView that the component is rendered in with a more modern Context based api. This also be useful for hybrid apps where the root view might not cover the entire screen.

Another goal of this is to be able to expose safeAreaInset values which cannot be accessed currently from JS. SafeAreaView is sometimes not flexible enough if we want to apply the insets manually in JS (for example use margins instead of padding).

Test Plan:

iOS RNTester

The red square is a view that has width and height set to the LayoutContext frame with padding equal to the LayoutContext safeAreaInsets. The blue square is a view that fills the content of its parent to show the safe areas.

Sample code from the RNTester example to illustrate this

return (
  <LayoutContext>
    {({layout, safeAreaInsets}) => (
      <View
        style={{
          width: layout.width,
          height: layout.height,
          paddingTop: safeAreaInsets.top,
          paddingRight: safeAreaInsets.right,
          paddingBottom: safeAreaInsets.bottom,
          paddingLeft: safeAreaInsets.left,
          backgroundColor: 'red',
        }}>
          <View style={{flex: 1, backgroundColor: 'blue'}} />
        </View>
    )}
  </LayoutContext>
);

iOS

  • Test that safe area insets and frame match the actual app frame for iOS 11 and iOS < 11 (polyfilled version)

  • Test insets on iPhone X with screen rotation

  • Test insets on iPhone X with root view that doesn't cover the screen

  • Test that showing and hiding the status bar updates the safe area. This doesn't work on iOS < 11 but also doesn't work for the SafeAreaView component. I couldn't find a way to make it work since the status bar frame changed event is not triggered when hiding / showing. Also kvo for the statusBarHidden prop did not work either for some reason... This isn't a big deal anyway.

  • Test toggling the on-call status bar

Android

  • Test changing the status bar hidden prop
  • Test changing the status bar translucent prop
  • Test screen rotation
  • Test making the navigation bar transparent (https://stackoverflow.com/a/31596735)

Release Notes:

[General] [Added] - New LayoutContext API to access the frame and safe area of the root view

@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 Sep 6, 2018
@elicwhite
Copy link
Member

Cc @yungsters @shergin, what do you think of this approach?

@yungsters
Copy link
Contributor

This is pretty cool. My main concern would be that when the values change, we must trigger a re-render. With this approach, my understanding is that when something causes the safe area insets to change (e.g. orientation change), we would have to force a re-render with the new values.

Ideally, we would not have to necessitate a re-render in JavaScript. But in reality, I think this solves the problem very elegantly and 99% of the time, people will not experience consequences of my concern.

@shergin, I know you had more thoughts than anyone else about the future of SafeAreaView. What do you think about this approach?

@janicduplessis
Copy link
Contributor Author

@yungsters I think in a lot of cases the re-render will get blocked by some pure component up in the tree, and even if it doesn't I guess re-rendering on orientation change is not a bad thing.

@janicduplessis
Copy link
Contributor Author

The main difference between this API and SafeAreaView is that SafeAreaView does take into consideration it's position inside the RootView and adjusts its insets accordingly. The drawback is that you have no control over how the insets are applied to the view and cannot know their value.

This API has the advantage of exposing the inset values to JS with a synchronous API so they can be used during the first render, this allow more flexibility to choose how the insets are being applied to views. The main drawback is that it only considers the position of the RootView in the screen so you have to make sure the view covers the whole RootView if you want to apply the insets directly. In the case the view doesn't cover the screen or you don't need the flexibility of knowing values in JS then SafeAreaView might be a better choice.

I think the APIs are kind of complementary for now since they both have different tradeoffs.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Dec 29, 2018

Finally got some time to look at this again!

  • Renamed frame to layout to match other events
  • Moved the subscription to changes and updating the provider to a separate component, this will prevent re-rendering the whole app when updating the context (great article about how this works https://frontarm.com/articles/react-context-performance/). It was also a problem to have this in AppContainer since it is also used in modals.

What's missing:

  • Use the notch api on Android sdk 28+
  • Update the context on changes (screen rotation, status bar hidden, etc) on Android

@janicduplessis
Copy link
Contributor Author

@yungsters This should address your concern about re-rendering.

@facebook facebook deleted a comment from pull-bot Dec 29, 2018
@pull-bot
Copy link

pull-bot commented Dec 30, 2018

Messages
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against d87cf93

@brunolemos
Copy link
Contributor

I've been meaning to have a useSafeAreaInsets hook and it seems this pr will allow this 🙌🙌

:shipit:

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sahrens
Copy link
Contributor

sahrens commented Apr 5, 2019

Quick update - there are a lot of flow errors internally trying to pull this in. I'll work through them soon.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sahrens sahrens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts after working with this internally a bit (and some from earlier comments that might have been forgotten):

  • re: @shergin's concern with exposing {x: 0, y: 0} - is that information really necessary? Should we just remove it? I think the problem is that it could change due to some native parent changing without notifying JS, so if JS is using the values, they might not be correct.
  • We should fix the naming - LayoutContext feels like it would match onLayout, but it's only about the root - should probably call it RootViewLayoutContext. We should also make the filename match the export (RootViewLayout.js)
  • We should export the Context itself so it's easy to use from class components and hooks. The consumer would be used as <RootViewLayoutContext.Consumer>{ctx => ...}</...>
  • would be nice to get it working in Fabric too, but we can do that in followups.
  • Did you test the behavior with Modal's that originate from smaller embedded root views? I wonder if we should do something simpler that's more about the entire App rather than the rootView - it would basically just report the screen dimensions most of the time, but on orientation change or split screen multitasking it would change.

@janicduplessis
Copy link
Contributor Author

Also little update: I’ve shipped this to production for a few weeks and so far iOS is solid but I got a crash that happens kind of infrequently on Android that I need to investigate before we can ship.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Apr 17, 2019

@sahrens

  • You are talking about https://github.com/facebook/react-native/pull/20999/files#diff-5fb3b29563ba1e3091b5b0ce41f006d2R32 right? It is basically there just to avoid having to type the context as nullable since it does require to pass an initial value but it will always be initialized when the RootView is created. Open to alternative though.
  • Agree
  • Agree, I also ran into this when creating a hook version in my app and made the change already in my fork.
  • Good point, it probably doesn't work at the moment. The modal could create a new context to provide its size to components that are inside of it. Not sure if this is blocking to land this, especially if we make it clear that the size is the one of the current RootView.
  • AFAIK Fabric integration is just a matter of re-implementing the logic to pass the layout context in the Fabric surface initialProps and calling DeviceEventEmitter when the layout context changes.
  • Making the LayoutContext tied to the RootView instead of some global value like the old Dimensions module solves a lot of technical issues because some elements like safeAreaInsets are only available at the View level. I think it also plays nicely with multiple RootViews and eventually multiple screens.

@sahrens
Copy link
Contributor

sahrens commented Apr 17, 2019

(1) I'm more concerned with people relying on x/y and having them change without an event firing as opposed to just the initial value in javascript.

(3) can you update this PR? I still see this:

  get LayoutContext() {
    return require('RootViewLayout').Context.Consumer;
  },

(4) Fabric decouples starting the app with the initial props from the root view so it will be tricky to link the two for a clean initial render.
(5) I think if the naming is clear then lack of modal support isn't blocking, but would be nice. I think a lot or most people conceptually think of modals as a new root, especially from a layout perspective.

@janicduplessis
Copy link
Contributor Author

@sahrens I'm still not sure I get (1), x/y should get updated whenever it changes (in layoutSubview on iOS and the global layout listener on Android). Or do you mean these values should always be zero? Like bounds vs frame on UIView.

@sahrens
Copy link
Contributor

sahrens commented May 2, 2019

I got this working internally but unfortunately it really hurt our performance benchmarks on iOS. I’m guessing the delay of bundleFinishedLoading is the most likely issue - any ideas?

@shergin
Copy link
Contributor

shergin commented May 9, 2019

@janicduplessis @sahrens

By this:

Exposing {x: 0, y: 0} is not reliable. The coordinates might change without notifying RCTRootView.

I mean that the action position of a RootView depends on all views around it and seems there is no easy/cheap way to listen for those changes. :(

@nyanev
Copy link

nyanev commented May 24, 2019

I got this working internally but unfortunately it really hurt our performance benchmarks on iOS. I’m guessing the delay of bundleFinishedLoading is the most likely issue - any ideas?

Have you fixed the performance?

@janicduplessis
Copy link
Contributor Author

@sahrens Can you give me more info about what the benchmark is and what it measures?

@janicduplessis
Copy link
Contributor Author

janicduplessis commented May 24, 2019

@shergin In this case maybe we should just make x,y always 0,0 like UIKit bounds. I don't think these values are very useful anyway.

@sahrens
Copy link
Contributor

sahrens commented Jun 6, 2019

@janicduplessis - sorry for the delay. The performance benchmark simply measures initial render time, that is, the time from the user tapping a button to the new content being visible on screen. The test in question is for the Marketplace tab in the Facebook app, so it's a brownfield cold start, and the "done" time is marked in native code via a special native component that listens for something like onAttachedToWindow. It would actually be really nice if we had a simple equivalent in Open Source...any idea how other folks measure performance?

@janicduplessis
Copy link
Contributor Author

@sahrens Thanks for the info, would it be possible to re-run the tests without the change to delay of bundleFinishedLoading. This would help isolate the issue.

I know @axe-fb did some work on perf measurement in OSS. Could be interesting to see if we can include that in RNTester and maybe integration tests.

// do a best effort polyfill.
// TODO: Use the DisplayCutout api when we target sdk 28+
RootViewInsets windowInsets;
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have this version check?WindowInsets is actually available from SDK level 20.

@LaszloDev
Copy link

Any updates on this?

@janicduplessis
Copy link
Contributor Author

Building something similar outside of core for now, gonna loop back if it makes sense to include at some point.

https://github.com/th3rdwave/react-native-safe-area-context

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. Contributor A React Native contributor. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.