From 546b8a4bf2b05980abe401af70cd1841e2b35998 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 8 Dec 2021 22:44:40 -0800 Subject: [PATCH] KeyboardAvoider: Reimplement iOS logic to abandon RN's KeyboardAvoidingView The immediate catalyst for this is #5162, but I've had earlier encounters with React Native's KeyboardAvoidingView, and I don't love it. See - our 70eca0716; that took a lot of painstaking investigation that would have been unnecessary if `keyboardVerticalOffset` had had a clear design from the beginning - the implementation, at https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js - the long list of open issues in RN that mention it: https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView including these two, which raise the same issue as #5162 and haven't been resolved: https://github.com/facebook/react-native/issues/29974 https://github.com/facebook/react-native/issues/31484 (patch-package isn't a resolution) I'm not optimistic that we can get #5162 fixed in bounded time unless we abandon KeyboardAvoidingView and implement the necessary logic ourselves. Fortunately, that seems pretty doable. I don't yet see a quick fix in KeyboardAvoidingView that doesn't risk making its code even more brittle and hard to reason about. My threshold for that is pretty low in this case: with the code in its current state, each additional piece of logic (even sound logic) comes with an unusually large cost of understanding/maintaining it. My current understanding is that the initial setup for maintaining our own react-native fork would be kind of a big project, but I could be wrong. None of my PRs to React Native can be accepted until Facebook starts checking its CLA-related mail; I should be covered by the corporate CLA signed by Kandra. See, e.g., https://github.com/facebook/react-native/pull/29926#issuecomment-775324593 . RN upgrades tend to take a while for us to do; see our most recent, to RN v0.64: #4426. RN v0.67 is already doing release candidates. Tested on my iPhone 13 Pro running iOS 15, with and without the "Prefer Cross-Fade Transitions" setting (for the sake of #5162). Fixes: #5162 --- src/chat/ChatScreen.js | 2 +- src/common/KeyboardAvoider.js | 43 ++------ src/common/KeyboardAvoiderIos.js | 173 +++++++++++++++++++++++++++++++ src/common/Screen.js | 5 +- 4 files changed, 181 insertions(+), 42 deletions(-) create mode 100644 src/common/KeyboardAvoiderIos.js diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 23eb933f40e..e7699b74bd1 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -181,7 +181,7 @@ export default function ChatScreen(props: Props): Node { ); return ( - + diff --git a/src/common/KeyboardAvoider.js b/src/common/KeyboardAvoider.js index dfe3e9faaec..4f4be36c837 100644 --- a/src/common/KeyboardAvoider.js +++ b/src/common/KeyboardAvoider.js @@ -1,58 +1,27 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; import type { Node } from 'react'; -import { KeyboardAvoidingView, Platform, View } from 'react-native'; +import { Platform, View } from 'react-native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; +import KeyboardAvoiderIos from './KeyboardAvoiderIos'; + type Props = $ReadOnly<{| - behavior?: ?('height' | 'position' | 'padding'), children: Node, style?: ViewStyleProp, - - /** How much the top of `KeyboardAvoider`'s layout *parent* is - * displaced downward from the top of the screen. - * - * If this isn't set correctly, the keyboard will hide some of - * the bottom of the screen (an area whose height is what this - * value should have been set to). - * - * I think `KeyboardAvoidingView`'s implementation mistakes `x` - * and `y` from `View#onLayout` to be a `View`'s position - * relative to the top left of the screen. In reality, I'm - * pretty sure they represent a `View`'s position relative to - * its parent: - * https://github.com/facebook/react-native-website/issues/2056#issuecomment-773618381 - * - * But at least `KeyboardAvoidingView` exposes this prop, which - * we can use to balance the equation if we need to. - */ - keyboardVerticalOffset?: number, |}>; /** - * Renders RN's `KeyboardAvoidingView` on iOS, `View` on Android. - * - * This component's props that are named after - * `KeyboardAvoidingView`'s special props get passed straight through - * to that component. + * Renders our `KeyboardAvoiderIos` on iOS, `View` on Android. */ export default class KeyboardAvoider extends PureComponent { render(): Node { - const { behavior, children, style, keyboardVerticalOffset } = this.props; + const { children, style } = this.props; if (Platform.OS === 'android') { return {children}; } - return ( - - {children} - - ); + return {children}; } } diff --git a/src/common/KeyboardAvoiderIos.js b/src/common/KeyboardAvoiderIos.js new file mode 100644 index 00000000000..45d365ed687 --- /dev/null +++ b/src/common/KeyboardAvoiderIos.js @@ -0,0 +1,173 @@ +/* @flow strict-local */ +import React, { useRef, useCallback, useState, useEffect } from 'react'; +import type { Node } from 'react'; +import { View, Keyboard, useWindowDimensions, LayoutAnimation, Platform } from 'react-native'; +import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; +import type { IOSKeyboardEvent } from 'react-native/Libraries/Components/Keyboard/Keyboard'; +import invariant from 'invariant'; + +type Props = $ReadOnly<{| + children: Node, + style?: ViewStyleProp, + contentContainerStyle?: ViewStyleProp, +|}>; + +type Measurements = null | $ReadOnly<{| x: number, y: number, width: number, height: number |}>; + +/** + * The near-current value of viewRef.measureInWindow; reacts to layout changes + * + * The returned `onLayout` function must be supplied to the View; see + * https://reactnative.dev/docs/view#onlayout. + * + * See also the doc for viewRef.measureInWindow: + * https://reactnative.dev/docs/0.64/direct-manipulation#measureinwindowcallback + * + * "Near-current" because viewRef.measureInWindow is an asynchronous query; + * experimentally, it seems to take 10-50ms. + * + * The X and Y coordinates passed to the `onLayout` callback are relative to + * the parent view. `measureInWindow` gives you coordinates relative to the + * entire window, which is often more useful -- but RN doesn't give us a + * direct hook into when `measureInWindow`'s result changes. This is meant + * to address that lack. + */ +function useViewMeasurementsInWindow(viewRef) { + const counter = useRef(0); + const [measurements, setMeasurements] = useState(null); + + const onLayout = useCallback( + _ => { + // Ignore the data passed to the callback; we just want to make sure + // `measureInWindow` runs on each layout change. + + counter.current++; + const counterValueNow = counter.current; + + if (!viewRef.current) { + return; + } + + viewRef.current.measureInWindow((x, y, width, height) => { + // `measureInWindow` is asynchronous. If another `measureInWindow` + // started while we were waiting for this one, don't commit the + // result. Otherwise, do. + if (counterValueNow === counter.current) { + setMeasurements({ x, y, width, height }); + counter.current %= Number.MAX_SAFE_INTEGER; + } + }); + }, + [viewRef], + ); + + return { + onLayout, + measurements, + }; +} + +/** + * Like RN's `KeyboardAvoidingView`, but with some assumptions/improvements. + * + * Unlike `KeyboardAvoidingView`, we assume: + * - We're on iOS + * - The offset is done with padding (like `behavior="padding"`) + * + * We make these improvements (as of RN v0.64): + * - No fiddly `keyboardVerticalOffset` that no one understands (see + * our 70eca0716) + * - It works even when the user has enabled "Prefer Cross-Fade Transitions" + * (UIAccessibilityPrefersCrossFadeTransitions); see + * https://github.com/facebook/react-native/issues/29974 + * - We hope to make it happier code (easier to read, debug, maintain, etc.) + */ +// We rely on https://reactnative.dev/docs/keyboard. `KeyboardAvoidingView` +// uses that internally too, and it's a public API. Hopefully that gives us +// some assurance about the stability of our component as RN evolves. +// TODO: Could write some tests for the Hooks logic; see +// `useHasStayedTrueForMs` for testing a hook, and for another possible +// approach, see +// https://github.com/zulip/zulip-mobile/pull/4997#issuecomment-916457062. +// However, we rely heavily on the behavior of +// https://reactnative.dev/docs/keyboard, and that wouldn't be naturally +// covered no matter how we test this component. +export default function KeyboardAvoiderIos(props: Props): Node { + const { children } = props; + + // The most recent IOSKeyboardEvent, if any since mount. + const [keyboardEvent, setKeyboardEvent] = useState(null); + useEffect(() => { + invariant(Platform.OS === 'ios', 'KeyboardAvoiderIos expected to be on iOS'); + + const sub = Keyboard.addListener('keyboardWillChangeFrame', (e: IOSKeyboardEvent) => { + setKeyboardEvent(e); + }); + return () => sub.remove(); + }, []); + + const viewRef = useRef(); + const { onLayout, measurements: viewMeasurementsInWindow } = useViewMeasurementsInWindow(viewRef); + const { height: windowHeight } = useWindowDimensions(); + const [paddingBottom, setPaddingBottom] = useState(0); + useEffect(() => { + if (!viewMeasurementsInWindow || !keyboardEvent) { + return; + } + + const { + y: viewYInWindow, + height: viewHeight, + // ignore view x and width + } = viewMeasurementsInWindow; + const { + endCoordinates: { + height: endKeyboardHeight, + screenY: endKeyboardY, + // Ignore keyboard end x and width + }, + easing, + duration, + // ignore startCoordinates and isEventFromThisApp + // TODO: Only proceed if isEventFromThisApp is true, right? + } = keyboardEvent; + + setPaddingBottom(currentValue => { + const newValue = + // In most environments, `endKeyboardHeight` is some positive value, + // and `endKeyboardY` goes between `windowHeight` (on closing + // keyboard) and something less than `windowHeight` (on opening). + // + // If UIAccessibilityPrefersCrossFadeTransitions, then on closing + // the keyboard, *everything* in `endCoordinates` will be zero, + // including `endKeyboardHeight`. As far as I can tell from reading + // code, those `endCoordinates` are pretty much passed straight + // through from Apple. See facebook/react-native#29974. + // + // We handle both cases with this expression. + endKeyboardY < windowHeight && endKeyboardHeight > 0 + ? viewYInWindow + viewHeight - endKeyboardY + : 0; + + if (currentValue === newValue) { + return currentValue; + } + + LayoutAnimation.configureNext({ + duration: Math.max(10, duration), // LayoutAnimation errors on <10ms + update: { + duration: Math.max(10, duration), // LayoutAnimation errors on <10ms + type: LayoutAnimation.Types[easing] || 'keyboard', + }, + }); + + return newValue; + }); + }, [keyboardEvent, viewMeasurementsInWindow, windowHeight]); + + return ( + + {children} + + ); +} diff --git a/src/common/Screen.js b/src/common/Screen.js index 76d79be5b5f..f0a8bba5a22 100644 --- a/src/common/Screen.js +++ b/src/common/Screen.js @@ -114,10 +114,7 @@ export default function Screen(props: Props): Node { )} {shouldShowLoadingBanner && } - + {scrollEnabled ? (