Skip to content

Commit

Permalink
KeyboardAvoider: Reimplement iOS logic to abandon RN's KeyboardAvoidi…
Browse files Browse the repository at this point in the history
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; 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 zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#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 RN starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#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 zulip#5162).

Fixes: zulip#5162
  • Loading branch information
chrisbobbe committed Dec 9, 2021
1 parent 8c91495 commit 1b9b013
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 42 deletions.
2 changes: 1 addition & 1 deletion src/chat/ChatScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ export default function ChatScreen(props: Props): Node {
);

return (
<KeyboardAvoider style={[componentStyles.screen, { backgroundColor }]} behavior="padding">
<KeyboardAvoider style={[componentStyles.screen, { backgroundColor }]}>
<ChatNavBar narrow={narrow} editMessage={editMessage} />
<OfflineNotice />
<UnreadNotice narrow={narrow} />
Expand Down
43 changes: 6 additions & 37 deletions src/common/KeyboardAvoider.js
Original file line number Diff line number Diff line change
@@ -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<Props> {
render(): Node {
const { behavior, children, style, keyboardVerticalOffset } = this.props;
const { children, style } = this.props;

if (Platform.OS === 'android') {
return <View style={style}>{children}</View>;
}

return (
<KeyboardAvoidingView
behavior={behavior}
// See comment on this prop in the jsdoc.
keyboardVerticalOffset={keyboardVerticalOffset}
style={style}
>
{children}
</KeyboardAvoidingView>
);
return <KeyboardAvoiderIos style={style}>{children}</KeyboardAvoiderIos>;
}
}
173 changes: 173 additions & 0 deletions src/common/KeyboardAvoiderIos.js
Original file line number Diff line number Diff line change
@@ -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<number>(0);
const [measurements, setMeasurements] = useState<Measurements>(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 (
<View style={[props.style, { paddingBottom }]} ref={viewRef} onLayout={onLayout}>
{children}
</View>
);
}
5 changes: 1 addition & 4 deletions src/common/Screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,7 @@ export default function Screen(props: Props): Node {
)}
<OfflineNotice />
{shouldShowLoadingBanner && <LoadingBanner />}
<KeyboardAvoider
behavior="padding"
style={[componentStyles.wrapper, padding && styles.padding]}
>
<KeyboardAvoider style={[componentStyles.wrapper, padding && styles.padding]}>
{scrollEnabled ? (
<ScrollView
contentContainerStyle={[centerContent && componentStyles.content, style]}
Expand Down

0 comments on commit 1b9b013

Please sign in to comment.