Fixed scrollview inset when RN view is embedded in another view #27607

danielgindi · 2019-12-25T12:23:44Z

Summary

I'm using RNN, which embeds RN view inside native view controllers.

On iOS 13, a modal view controller is "floating" and is offset from the top of the screen.

This causes the calculation of inset in KeyboardAvoidingView incorrect as it mixes local view controller coordinate space, with keyboard's screen coordinate space.

Changelog

[iOS] [Fixed] - Fixed KeyboardAvoidingView inset in embedded views (i.e modal view controllers on iOS 13)

Test Plan

Tested before and after in a simple view controller (should stay the same)
Tested before and after in a modal view controller (should be offset before, and fixed after)
Repeated no. 2 with each device rotation (upsideDown, landscapeLeft, landscapeRight)

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/Keyboard/KeyboardAvoidingView.js

danielgindi · 2020-04-01T05:21:25Z

Closed by mistake

This happened to me mainly in modal view controllers on iOS 13, where the view controller is floating and does not start from the top.

analysis-bot · 2020-04-01T05:52:06Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,596,711	-8,422
android	hermes	armeabi-v7a	6,273,863	-7,318
android	hermes	x86	6,936,846	-8,296
android	hermes	x86_64	6,866,660	-8,352
android	jsc	arm64-v8a	8,905,008	-8,462
android	jsc	armeabi-v7a	8,545,489	-7,372
android	jsc	x86	8,729,192	-8,340
android	jsc	x86_64	9,305,222	-8,396

Base commit: 2b062ea

analysis-bot · 2020-04-01T06:11:03Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	966,896	0

Base commit: 2b062ea

danielgindi · 2020-04-01T06:25:35Z

@kelset This is an easy fix for modals presented in iOS 13, and iOS 14 is almost here....

kelset · 2020-04-01T08:37:12Z

hey Daniel, thanks for the ping. I can try to put this in front of the right people but can't make promises

elicwhite

This change will cause a delay in rendering as the measure is an async round trip to native.

I wonder if this change should be made on the native side and if the layout metrics are wrong for embedded views.

danielgindi · 2020-04-02T06:29:13Z

This change will cause a delay in rendering as the measure is an async round trip to native.

I wonder if this change should be made on the native side and if the layout metrics are wrong for embedded views.

I tested in dev and production, there’s no side effects.
The inset is used for when keyboard is visible, which is not the default.
So when the view shows, you calculate the correct position by measuring, not by causing another render. When keyboard shows- you use the precalculated value.

facebook-github-bot

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

elicwhite · 2020-04-02T06:41:05Z

Ah, alright. That seems reasonable to me.

react-native-bot · 2020-04-03T23:10:05Z

This pull request was successfully merged by @danielgindi in fb2900e.

^{When will my fix make it into a release? | Upcoming Releases}

elicwhite · 2020-04-07T01:21:35Z

This needed to be reverted: a37e45a

This caused an issue on the login screen of the Oculus app where the TextInputs were covered up with the login button. Hopefully @xyin96 can provide more info for you to debug and fix this.

danielgindi · 2020-04-07T12:37:12Z

@TheSavior @xyin96 Waiting for more info on this.
Are you sure the inputs weren't covered due to some workaround in the app?

danielgindi · 2020-04-12T06:02:06Z

@TheSavior I don't get it. This got reverted due to a regression in a different app (no matter who stands behind it) without even looking into the bug? Chances are there's a workaround in place because of this bug.

elicwhite · 2020-04-12T07:33:57Z

@danielgindi, I totally get where you are coming from. I'm hoping @xyin96 can give more info from the Oculus app so you two can figure out how to proceed.

Fwiw, there being a workaround to this bug that ends up being a problem with this fix is still reason for concern. Who knows how many other surfaces and apps have workarounds for the sad state of the KeyboardAvoidingView. If those screens all break during an upgrade, even if KeyboardAvoidingView is now more correct, it would be detrimental to those companies and apps and make the upgrade more difficult.

I'm hoping that @xyin96 can respond, and a path forward can be found that can make this work better out of the box but also not break existing screens with their current workarounds.

danielgindi · 2020-04-12T09:08:19Z

@TheSavior There is not a single release of RN that does not break a workaround somewhere, or even a valid piece of code. The problem with this issue is that it's actually is very difficult to workaround without glitches and re-rendering.
My "workaround" for RN projects includes a script that runs after npm install and changes some stuff - like migrating some Android imports to androidx. namespace, or applying fixes like the current one.

…book#27607) Summary: I'm using RNN, which embeds RN view inside native view controllers. On iOS 13, a modal view controller is "floating" and is offset from the top of the screen. This causes the calculation of inset in `KeyboardAvoidingView` incorrect as it mixes local view controller coordinate space, with keyboard's screen coordinate space. ## Changelog [iOS] [Fixed] - Fixed `KeyboardAvoidingView` inset in embedded views (i.e modal view controllers on iOS 13) Pull Request resolved: facebook#27607 Test Plan: 1. Tested before and after in a simple view controller (should stay the same) 2. Tested before and after in a modal view controller (should be offset before, and fixed after) 3. Repeated no. 2 with each device rotation (upsideDown, landscapeLeft, landscapeRight) Reviewed By: cpojer Differential Revision: D20812231 Pulled By: TheSavior fbshipit-source-id: fbd72739fb7152655028730e284ad26ff4a5da73

xyin96 · 2020-04-15T18:08:01Z

Hi @danielgindi, sorry for the late response.

I had begun to look into this a few days back, but got caught up in some other work items. It looks like in our case, the this breaks down because the KeyboardAvoidingView is not our container component.

e.g.:

<SafeAreaView style={{flex: 1}}>
  <KeyboardAvoidingView>
    {/* ... */}
  </KeyboardAvoidingView>
</SafeAreaView>

In this case, if you had tried to measure the layout of the KeyboardAvoidingView, it would have a height of 0.

I'm not too familiar with iOS. But I'm wondering if this fix is better done in the native ViewManager rather than in JS because this would also affect how the component works for Android.

danielgindi · 2020-04-15T21:31:29Z

Sounds weird, as it’s not the root in my case too. And it should not matter as the original code measures its frame relative to the root, not the parent. And the patch does the same, only considering that the root may be offset.
Is it possible though that some scroll offset is getting in the way here in your case?

danielgindi · 2020-05-04T07:26:24Z

@xyin96 Could you please post a sample screen that demonstrates an issue after the fix? I've tested this in all directions, can't see any issue.
Does it even happen on iOS or Android?
Are you using height, padding, or position mode? (on ios padding mode works best for scrollviews in my experience)

danielgindi · 2020-05-04T07:51:14Z

@TheSavior I request for this to be re-merged.

I've tested this thoroughly in every condition imaginable. The offsets always come out right. Wrapped in SafeAreaView, unwrapped, wrapped in ScrollView, unwrapped, with contentInsetAdjustmentBehavior='never' or default, with all behavior modes, inside an iOS 13 modal view controller, inside a normal view controller with navigation bar, without navigation bar, and all possible combinations of all options above.

elicwhite · 2020-05-05T21:29:55Z

@xyin96, can you help @danielgindi get a repro for the issue that Oculus had?

I can't remerge this as is. If this gets landed again as is, it would have the same effect. It would break Oculus and Oculus would revert it. It would require me getting familiar with the Oculus app and getting a repro for you myself which I don't have the bandwidth for, so I'm hoping that @xyin96 can help. I recognize this is unfortunate and doesn't leave you with an easy way to move forward. :-/

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 Dec 25, 2019

react-native-bot added Component: ScrollView Bug Platform: iOS iOS applications. labels Dec 25, 2019

danielgindi mentioned this pull request Dec 25, 2019

0.62.x Discussion react-native-community/releases#145

Closed

analysis-bot suggested changes Dec 29, 2019

View reviewed changes

Libraries/Components/Keyboard/KeyboardAvoidingView.js Outdated Show resolved Hide resolved

Libraries/Components/Keyboard/KeyboardAvoidingView.js Outdated Show resolved Hide resolved

danielgindi force-pushed the patch-1 branch from e4e1077 to f25e6ca Compare December 29, 2019 07:50

kelset mentioned this pull request Jan 21, 2020

0.62.x RC Discussion react-native-community/releases#157

Closed

6 tasks

ashfurrow mentioned this pull request Jan 24, 2020

[MX-152] Opts out of new iOS 13 modal presentation style on iPhone artsy/eigen#2997

Merged

danielgindi force-pushed the patch-1 branch from f25e6ca to 30a9935 Compare April 1, 2020 05:19

danielgindi requested review from cpojer, hramos and shergin as code owners April 1, 2020 05:19

danielgindi closed this Apr 1, 2020

danielgindi deleted the patch-1 branch April 1, 2020 05:20

danielgindi restored the patch-1 branch April 1, 2020 05:21

danielgindi reopened this Apr 1, 2020

danielgindi force-pushed the patch-1 branch from 30a9935 to b0e548f Compare April 1, 2020 05:23

Fixed scrollview inset when RN view is embedded in another view

15d2c8a

This happened to me mainly in modal view controllers on iOS 13, where the view controller is floating and does not start from the top.

danielgindi force-pushed the patch-1 branch from b0e548f to 15d2c8a Compare April 1, 2020 05:33

elicwhite reviewed Apr 1, 2020

View reviewed changes

facebook-github-bot reviewed Apr 2, 2020

View reviewed changes

facebook-github-bot closed this in fb2900e Apr 3, 2020

react-native-bot added the Merged This PR has been merged. label Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed scrollview inset when RN view is embedded in another view #27607

Fixed scrollview inset when RN view is embedded in another view #27607

danielgindi commented Dec 25, 2019 •

edited

Loading

analysis-bot left a comment

danielgindi commented Apr 1, 2020

analysis-bot commented Apr 1, 2020

analysis-bot commented Apr 1, 2020 •

edited

Loading

danielgindi commented Apr 1, 2020

kelset commented Apr 1, 2020

elicwhite left a comment

danielgindi commented Apr 2, 2020 •

edited

Loading

facebook-github-bot left a comment

elicwhite commented Apr 2, 2020

react-native-bot commented Apr 3, 2020

elicwhite commented Apr 7, 2020

danielgindi commented Apr 7, 2020

danielgindi commented Apr 12, 2020

elicwhite commented Apr 12, 2020

danielgindi commented Apr 12, 2020

xyin96 commented Apr 15, 2020 •

edited

Loading

danielgindi commented Apr 15, 2020

danielgindi commented May 4, 2020

danielgindi commented May 4, 2020

elicwhite commented May 5, 2020

Fixed scrollview inset when RN view is embedded in another view #27607

Fixed scrollview inset when RN view is embedded in another view #27607

Conversation

danielgindi commented Dec 25, 2019 • edited Loading

Summary

Changelog

Test Plan

analysis-bot left a comment

Choose a reason for hiding this comment

danielgindi commented Apr 1, 2020

analysis-bot commented Apr 1, 2020

analysis-bot commented Apr 1, 2020 • edited Loading

danielgindi commented Apr 1, 2020

kelset commented Apr 1, 2020

elicwhite left a comment

Choose a reason for hiding this comment

danielgindi commented Apr 2, 2020 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

elicwhite commented Apr 2, 2020

react-native-bot commented Apr 3, 2020

elicwhite commented Apr 7, 2020

danielgindi commented Apr 7, 2020

danielgindi commented Apr 12, 2020

elicwhite commented Apr 12, 2020

danielgindi commented Apr 12, 2020

xyin96 commented Apr 15, 2020 • edited Loading

danielgindi commented Apr 15, 2020

danielgindi commented May 4, 2020

danielgindi commented May 4, 2020

elicwhite commented May 5, 2020

danielgindi commented Dec 25, 2019 •

edited

Loading

analysis-bot commented Apr 1, 2020 •

edited

Loading

danielgindi commented Apr 2, 2020 •

edited

Loading

xyin96 commented Apr 15, 2020 •

edited

Loading