Skip to content

Commit 369c785

Browse files
authored
fix: animation stutter on edit (#3537)
## 🎯 Goal This PR fixes an Android timing bug in the message edit flow when edit is triggered from the message overlay that caused the keyboard to be immediately closed afterwards. It also fixes severe visual stuttering when changing the state to editing within the `MessageComposer` on both platforms. The bug was caused by the edit action focusing the message input before `PortalWhileClosingView` had fully returned the teleported composer back into its normal tree. On Android, that caused the `TextInput` to lose focus mid keyboard animation, which immediately closed the keyboard again. The stuttering part is caused by the fact that the keyboard opening and the state changing to editing essentially animate the layout of the same component, but through 2 different state values - often offset by a single frame. This means that it is very realistic for the `MessageComposer` to animate its layout height, bottom padding and other layout props all offset from each other by a frame, making the animations feel wonky. The fix separates the problem into two explicit stages: 1. wait for portal close handoff to settle 2. wait for the keyboard to be open before running the edit callback This preserves the original edit behavior while making the focus timing safe. When long pressing a message and tapping `Edit`, the expected behaviour is: 1. close the overlay 2. focus the composer input 3. open the keyboard 4. enter edit mode On Android, that flow was unstable when the overlay was still closing through PortalWhileClosingView. What was actually happening: 1. the overlay started closing 2. the composer was still teleported into the overlay host 3. the edit action focused the input too early 4. `react-native-teleport` moved the composer back into the normal tree after focus had already been applied 5. the input lost focus during that handoff 6. the keyboard started opening and then immediately closed again This showed up much more clearly in release builds than in debug builds, because of the fact that no animation frames are actually skipped. In other words, the invocation of the scheduled actions to execute after the context menu closes happens way too fast in the React lifecycle. So, the fix for both of these things looks something like this: - Immediate focus after overlay close and only invoke edit after the keyboard has been opened (if applicable) - Waiting for two `requestAnimationFrames` worked reliably: - one frame for the portal retarget / React commit - one additional frame for the native view hierarchy and `TextInput` host to settle We also considered solving this directly in the overlay queue, but the committed fix keeps the behavior local to the edit action instead. In the future, if we figure that 2 RAFs are not enough (on `120mhz` phones for example), there is an alternative solution that can introduce a relatively complex registry on the `PortalWhileClosingView` layer that waits for true layout settle before doing any teleport sensitive actions (like focusing an input). I've yet to reproduce a case where they aren't enough, though - so let's keep it simple for now. ## 🛠 Implementation details <!-- Provide a description of the implementation --> ## 🎨 UI Changes <!-- Add relevant screenshots --> <details> <summary>iOS</summary> <table> <thead> <tr> <td>Before</td> <td>After</td> </tr> </thead> <tbody> <tr> <td> <!--<img src="" /> --> </td> <td> <!--<img src="" /> --> </td> </tr> </tbody> </table> </details> <details> <summary>Android</summary> <table> <thead> <tr> <td>Before</td> <td>After</td> </tr> </thead> <tbody> <tr> <td> <!--<img src="" /> --> </td> <td> <!--<img src="" /> --> </td> </tr> </tbody> </table> </details> ## 🧪 Testing <!-- Explain how this change can be tested (or why it can't be tested) --> ## ☑️ Checklist - [ ] I have signed the [Stream CLA](https://docs.google.com/forms/d/e/1FAIpQLScFKsKkAJI7mhCr7K9rEIOpqIDThrWxuvxnwUq2XkHyG154vQ/viewform) (required) - [ ] PR targets the `develop` branch - [ ] Documentation is updated - [ ] New code is tested in main example apps, including all possible scenarios - [ ] SampleApp iOS and Android - [ ] Expo iOS and Android
1 parent 4a66852 commit 369c785

6 files changed

Lines changed: 162 additions & 13 deletions

File tree

package/src/__tests__/offline-support/offline-feature.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,13 @@ export const Generic = () => {
338338
act(() => dispatchConnectionChangedEvent(chatClient));
339339
await act(async () => await chatClient.offlineDb.syncManager.invokeSyncStatusListeners(true));
340340

341-
await waitFor(async () => {
342-
expect(screen.getByTestId('channel-list')).toBeTruthy();
343-
await expectAllChannelsWithStateToBeInDB(screen.queryAllByLabelText);
344-
});
341+
await waitFor(
342+
async () => {
343+
expect(screen.getByTestId('channel-list')).toBeTruthy();
344+
await expectAllChannelsWithStateToBeInDB(screen.queryAllByLabelText);
345+
},
346+
{ timeout: 5000 },
347+
);
345348
});
346349

347350
it('should fetch channels from the db correctly even if they are empty', async () => {

package/src/components/Message/hooks/useMessageActionHandlers.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,20 @@ import type { MessageContextValue } from '../../../contexts/messageContext/Messa
1212
import type { MessagesContextValue } from '../../../contexts/messagesContext/MessagesContext';
1313

1414
import { useTranslationContext } from '../../../contexts/translationContext/TranslationContext';
15-
import { useStableCallback } from '../../../hooks';
15+
import {
16+
useAfterKeyboardOpenCallback,
17+
usePortalSettledCallback,
18+
useStableCallback,
19+
} from '../../../hooks';
1620
import { useTranslatedMessage } from '../../../hooks/useTranslatedMessage';
1721
import { NativeHandlers } from '../../../native';
1822

23+
const useWithPortalKeyboardSafety = <T extends unknown[]>(callback: (...args: T) => void) => {
24+
const callbackAfterKeyboardOpen = useAfterKeyboardOpenCallback(callback);
25+
26+
return usePortalSettledCallback(callbackAfterKeyboardOpen);
27+
};
28+
1929
export const useMessageActionHandlers = ({
2030
channel,
2131
client,
@@ -114,7 +124,7 @@ export const useMessageActionHandlers = ({
114124
}
115125
});
116126

117-
const handleEditMessage = useStableCallback(() => {
127+
const handleEditMessage = useWithPortalKeyboardSafety(() => {
118128
setEditingState(message);
119129
});
120130

package/src/components/MessageInput/MessageComposer.tsx

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,6 @@ const MessageComposerWithContext = (props: MessageComposerPropsWithContext) => {
219219
closePollCreationDialog,
220220
CreatePollContent,
221221
createPollOptionGap,
222-
editing,
223222
InputView,
224223
MessageComposerLeadingView,
225224
MessageComposerTrailingView,
@@ -272,12 +271,6 @@ const MessageComposerWithContext = (props: MessageComposerPropsWithContext) => {
272271
[closeAttachmentPicker],
273272
);
274273

275-
useEffect(() => {
276-
if (editing && inputBoxRef.current) {
277-
inputBoxRef.current.focus();
278-
}
279-
}, [editing, inputBoxRef]);
280-
281274
/**
282275
* Effect to get the draft data for legacy thread composer and set it to message composer.
283276
* TODO: This can be removed once we remove legacy thread composer.
@@ -746,6 +739,7 @@ export const MessageComposer = (props: MessageComposerProps) => {
746739
closePollCreationDialog,
747740
compressImageQuality,
748741
CreatePollContent,
742+
// TODO: probably not needed anymore, please check
749743
editing,
750744
Input,
751745
InputView,

package/src/hooks/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@ export * from './useStableCallback';
77
export * from './useLoadingImage';
88
export * from './useMessageReminder';
99
export * from './useQueryReminders';
10+
export * from './useAfterKeyboardOpenCallback';
1011
export * from './useClientNotifications';
1112
export * from './useInAppNotificationsState';
13+
export * from './usePortalSettledCallback';
1214
export * from './useRAFCoalescedValue';
1315
export * from './useAudioPlayerControl';
1416
export * from './useAttachmentPickerState';
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { useEffect, useRef } from 'react';
2+
import { EventSubscription, Keyboard, Platform } from 'react-native';
3+
4+
import { useKeyboardVisibility } from './useKeyboardVisibility';
5+
6+
import { useStableCallback } from './useStableCallback';
7+
8+
import { KeyboardControllerPackage } from '../components/KeyboardCompatibleView/KeyboardControllerAvoidingView';
9+
import { useMessageInputContext } from '../contexts/messageInputContext/MessageInputContext';
10+
11+
/**
12+
* A utility hook that returns a stable callback which focuses the message input
13+
* and invokes the callback once the keyboard is open.
14+
*
15+
* @param callback - callback we want to run once the keyboard is ready
16+
* @returns A stable callback that will wait for the keyboard to be open before executing.
17+
*/
18+
export const useAfterKeyboardOpenCallback = <T extends unknown[]>(
19+
callback: (...args: T) => void,
20+
) => {
21+
const isKeyboardVisible = useKeyboardVisibility();
22+
const { inputBoxRef } = useMessageInputContext();
23+
const keyboardSubscriptionRef = useRef<EventSubscription | undefined>(undefined);
24+
// This callback runs from a keyboard event listener, so it must stay fresh across rerenders.
25+
const stableCallback = useStableCallback(callback);
26+
27+
/** Clears the pending keyboard listener, if any. */
28+
const clearKeyboardSubscription = useStableCallback(() => {
29+
keyboardSubscriptionRef.current?.remove();
30+
keyboardSubscriptionRef.current = undefined;
31+
});
32+
33+
useEffect(() => clearKeyboardSubscription, [clearKeyboardSubscription]);
34+
35+
return useStableCallback((...args: T) => {
36+
clearKeyboardSubscription();
37+
38+
const runCallback = () => {
39+
clearKeyboardSubscription();
40+
stableCallback(...args);
41+
};
42+
43+
if (!inputBoxRef.current) {
44+
runCallback();
45+
return;
46+
}
47+
48+
if (isKeyboardVisible) {
49+
inputBoxRef.current.focus();
50+
runCallback();
51+
return;
52+
}
53+
54+
const keyboardEvent = Platform.OS === 'ios' ? 'keyboardWillShow' : 'keyboardDidShow';
55+
56+
keyboardSubscriptionRef.current = KeyboardControllerPackage?.KeyboardEvents
57+
? KeyboardControllerPackage.KeyboardEvents.addListener(keyboardEvent, runCallback)
58+
: Keyboard.addListener(keyboardEvent, runCallback);
59+
60+
inputBoxRef.current.focus();
61+
});
62+
};
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
import { useEffect, useRef } from 'react';
2+
import { Platform } from 'react-native';
3+
4+
import { useStableCallback } from './useStableCallback';
5+
6+
/**
7+
* Number of frames we wait before invoking input focus sensitive work after the
8+
* overlay closes.
9+
*/
10+
const SETTLE_FRAMES = Platform.OS === 'android' ? 2 : 0;
11+
12+
/**
13+
* Runs a callback after a fixed number of animation frames.
14+
*
15+
* We use RAFs here because the settling work we care about is tied to the next
16+
* rendered frames after the overlay close transition.
17+
*
18+
* @param callback - callback to run once the frame budget has elapsed
19+
* @param frames - number of frames to wait
20+
* @param rafIds - accumulator used for later cancellation/cleanup
21+
*/
22+
const scheduleAfterFrames = (callback: () => void, frames: number, rafIds: number[]) => {
23+
if (frames <= 0) {
24+
callback();
25+
return;
26+
}
27+
28+
const rafId = requestAnimationFrame(() => scheduleAfterFrames(callback, frames - 1, rafIds));
29+
rafIds.push(rafId);
30+
};
31+
32+
/**
33+
* Returns a stable callback that is safe to run after a `PortalWhileClosingView`
34+
* has settled back into its original tree.
35+
*
36+
* Some followup actions are sensitive to that handoff window. If they run
37+
* while a view is still being returned from a portal host to its in place host,
38+
* they can target a node that is about to be reattached. On Android, that is
39+
* especially noticeable with focus sensitive work, where the target can lose
40+
* focus again mid keyboard animation.
41+
*
42+
* Two frames are intentional here:
43+
* - frame 1 lets the portal retarget and React commit the component tree
44+
* - frame 2 lets the native view hierarchy settle in its final host
45+
*
46+
* iOS does not currently need this settle window for this flow.
47+
*
48+
* A good example is the message composer edit action: after closing the message
49+
* overlay, we wait for the portal handoff to settle before focusing the input
50+
* and opening the keyboard. Doing this prematurely will result in the keyboard
51+
* being immediately closed.
52+
*
53+
* Another good example would be having a button wrapped in a `PortalWhileClosingView`,
54+
* that possibly renders (or morphs into) something when pressed. Handling `onPress`
55+
* prematurely here may lead to the morphed button rendering into a completely different
56+
* part of the UI hierarchy, causing unknown behaviour. This hook prevents that from
57+
* happening.
58+
*
59+
* @param callback - callback we want to invoke once the portal handoff has settled
60+
* @returns A stable callback gated behind the portal settle window.
61+
*/
62+
export const usePortalSettledCallback = <T extends unknown[]>(callback: (...args: T) => void) => {
63+
const rafIdsRef = useRef<number[]>([]);
64+
// This callback runs from deferred RAF work, so it must stay fresh across rerenders.
65+
const stableCallback = useStableCallback(callback);
66+
67+
const clearScheduledFrames = useStableCallback(() => {
68+
rafIdsRef.current.forEach((rafId) => cancelAnimationFrame(rafId));
69+
rafIdsRef.current = [];
70+
});
71+
72+
useEffect(() => clearScheduledFrames, [clearScheduledFrames]);
73+
74+
return useStableCallback((...args: T) => {
75+
clearScheduledFrames();
76+
scheduleAfterFrames(() => stableCallback(...args), SETTLE_FRAMES, rafIdsRef.current);
77+
});
78+
};

0 commit comments

Comments
 (0)