fix: messages content overlap when bottom sheet is shown #54764
Conversation
…own"" This reverts commit ba4f078.
|
@kirillzyusko is the next step here to fix the blockers before we can put this in review again? |
Yes, it's the plan 🙂 |
… you long-press last message be sure there is no empty gap between composer and the message
|
@kirillzyusko is there a rough ETA to get this PR ready for review again? |
|
@luacmartins there is one deploy blocker to be resolved. I was planning to discuss how to tackle that one tomorrow with @hannojg 👀 I'll get back to you tomorrow with the answer, but I hope this/next week is the target 😊 |
|
@kirillzyusko did you manage to discuss the fix with Hanno? Looking forward to getting this one in review again |
|
Sorry for providing no update earlier, Kiryl was blocked with some other tasks and I believe @perunt wanted to fix the remaining issues? |
|
Np, all good. Let me know if I should assign @perunt to the issue as well |
|
@luacmartins yes, please. You can assign it to me Also, after some testing, I found two bugs that I am going to address as well:
RPReplay_Final1736867201.MP4
RPReplay_Final1736940087.MP4 |
…om-sheet-message-overlap
|
@rojiphil, may I ask you to review this in the next round? |
…om-sheet-message-overlap
|
@luacmartins, the new ESLint rules introduce a lot of changes. I'm also not sure if it's a good idea to address them in this PR. Let me explain why: |
|
@perunt I agree that we can address those in a follow up, so we don't distract from the current changes here, avoid conflicts and make things easier to debug later on if reverts are needed. Let's go ahead without addressing those for now. |
…om-sheet-message-overlap
I see this on the main as well. I suggest raising an issue/feature request separately |
|
@perunt Yes |
|
@roryabraham did you wanna review again or are we good to merge? |
| import useWindowDimensions from '@hooks/useWindowDimensions'; | ||
| import {Actions, ActionSheetAwareScrollViewContext, States} from './ActionSheetAwareScrollViewContext'; | ||
|
|
||
| const KeyboardState = { |
There was a problem hiding this comment.
NAB:
diff --git a/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx b/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
index cf96a0823b0..c773c74c30b 100644
--- a/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
+++ b/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
@@ -3,6 +3,7 @@ import type {ViewProps} from 'react-native';
import {useKeyboardHandler} from 'react-native-keyboard-controller';
import Reanimated, {useAnimatedReaction, useAnimatedStyle, useDerivedValue, useSharedValue, withSequence, withSpring, withTiming} from 'react-native-reanimated';
import type {SharedValue} from 'react-native-reanimated';
+import type {ValueOf} from 'type-fest';
import useSafeAreaPaddings from '@hooks/useSafeAreaPaddings';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
@@ -14,16 +15,16 @@ const KeyboardState = {
OPEN: 2,
CLOSING: 3,
CLOSED: 4,
-};
+} as const;
const SPRING_CONFIG = {
mass: 3,
stiffness: 1000,
damping: 500,
-};
+} as const;
const useAnimatedKeyboard = () => {
- const state = useSharedValue(KeyboardState.UNKNOWN);
+ const state = useSharedValue<ValueOf<typeof KeyboardState>>(KeyboardState.UNKNOWN);
const height = useSharedValue(0);
const lastHeight = useSharedValue(0);
const heightWhenOpened = useSharedValue(0);
@@ -74,7 +75,7 @@ function ActionSheetKeyboardSpace(props: ActionSheetKeyboardSpaceProps) {
const {position} = props;
// Similar to using `global` in worklet but it's just a local object
- const syncLocalWorkletState = useSharedValue(KeyboardState.UNKNOWN);
+ const syncLocalWorkletState = useSharedValue<ValueOf<typeof KeyboardState>>(KeyboardState.UNKNOWN);
const {windowHeight} = useWindowDimensions();
const {currentActionSheetState, transitionActionSheetStateWorklet: transition, resetStateMachine} = useContext(ActionSheetAwareScrollViewContext);| const {windowHeight} = useWindowDimensions(); | ||
| const {currentActionSheetState, transitionActionSheetStateWorklet: transition, resetStateMachine} = useContext(ActionSheetAwareScrollViewContext); | ||
|
|
||
| // Reset state machine when component unmounts |
There was a problem hiding this comment.
NAB:
diff --git a/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx b/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
index cf96a0823b0..76c9bb3182b 100644
--- a/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
+++ b/src/components/ActionSheetAwareScrollView/ActionSheetKeyboardSpace.tsx
@@ -79,10 +79,7 @@ function ActionSheetKeyboardSpace(props: ActionSheetKeyboardSpaceProps) {
const {currentActionSheetState, transitionActionSheetStateWorklet: transition, resetStateMachine} = useContext(ActionSheetAwareScrollViewContext);
// Reset state machine when component unmounts
- // eslint-disable-next-line arrow-body-style
- useEffect(() => {
- return () => resetStateMachine();
- }, [resetStateMachine]);
+ useEffect(() => resetStateMachine, [resetStateMachine]);
useAnimatedReaction(
() => keyboard.state.get(),
|
@perunt let's resolve conflicts here too |
|
I think main changed such that there were no conflicts anymore. So I went ahead and merged this |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| } | ||
|
|
||
| // save previous payload or merge the new payload with the previous payload | ||
| const nextPayload = typeof action.payload === 'undefined' ? state.current.payload : fastMerge(state.current.payload, action.payload); |
There was a problem hiding this comment.
We created an app error due to this. We should convert fastMerge to worklet before using it here.
Should I create a PR for this?
There was a problem hiding this comment.
Yea, it seems like you already did 😄
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.1.53-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.1.53-0 🚀
|
|
@kirillzyusko This PR is failing because of issue #62955 |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.1.53-7 🚀
|
11 participants
To Do
paddingTopinuseAnimatedStyles. It triggersonLayoutevents when we perform animation. In some corner case sometimesonLayoutonCellItemskips some last events (when bottom sheet gets hidden we expect, that it will return to its initial position andonLayoutwill produce the same event as when the view was just mounted. But instead we start scrolling when view is far away about ~80-100px till its expected position). The simplest solution is to postpone a call toscrollTomethod, but in this case it produces significant delay).Details
A successor of #42143
The first step of react-native-keyboard-controller migration plan
Important
This PR relies on
react-native-keyboard-controller(useKeyboardHandler) hook andKeyboardAvoidingViewcomponent).How it works 🤔
This PR is iOS only for now. Other platforms are not affected. Also, in this explanation, we are talking about iOS only since on Android, iOS web and Android Web keyboard handling works differently.
Note
We're talking about default platform behavior. If we start to use
react-native-keyboard-controlleron Android - it'll disable all default keyboard handling by OS and will delegate the keyboard handling to us - so we will have identical behavior across both iOS/Android platforms.But it may introduce additional issues (because we need to enter edge-to-edge mode - that would be good to do on entire app level, but it may bring some issues with
StatusBarpaddings, so everything has to be verified very carefully). That's why I think it would be better to handle in separate PR.When we long press on a message, we want to be able to see it. It's easy when we have the keyboard closed. But gets tricky when the keyboard is open.
But first of all let's focus on the case when the keyboard is closed. User long press on the message, we render the bottom sheet (action sheet) with some options. If the message is higher than the height of bottom sheet - the message is visible. When the message is the last one (first from the bottom), it can be fully or partially covered by the bottom sheet.
Press to see videos
300481023-9dc8be61-799d-470b-997f-ee5c5995fe19.mp4
300481277-0d0570cd-a85f-4cf5-8a91-6a3d5ad72c58.mp4
Also everything is tricky when we have keyboard open 😓
Press to see videos with keyboard
300482884-cbe3b610-fa85-4071-a3dd-bbc1a9d56e05.mp4
300483006-2e5ba833-c0fa-4fad-b383-b3854e5fa112.mp4
To achieve this goal we had to:
FlatListofReportListView(it uses reanimated to move the view by the offset)KeyboardAvoidingso we're in sync with keyboard-avoiding style updatesActionSheetAwareScrollViewContextto pass the transition function, so we can transition from one state to another by action triggered from different parts of the applicationBelow you can find more use cases that involves keyboard and other UI elements interactions:
Interactions with other UI elements
300483536-04991f52-a41b-4d3e-abaf-12712b94f1aa.mp4
300483723-91b076a9-4c7a-4fbb-bc50-1e8194763e9d.mp4
Note
Call popover, bottom sheet and emoji picker are implemented as Modal windows and on iOS keyboard instantly gets hidden when modal window is shown. But "Attachment" popup is implemented not as modal, so for this type of transition we are using
progress-based animations. When we press on plus -> popup is already shown under keyboard and we dismiss the keyboard. Overall the mechanism is similar to what we've used in the past, except the moment where we substitutetranslationYvalue - in case when keyboard disappear/appears we interpolate it byprogressvariable to achieve smooth transitions.Technical details ⚒️
So in order to calculate the offset for the message, we:
ref.current.measureInWindow, so we can haveheightandycoordinate of the imageonLayoutcallbackDimensionssafeArea.topsinceycoordinate doesn't get into account safe areaelementOffset = (y + safeArea.top + itemHeight) - (windowHeight - popoverHeight)In order to animate item when they keyboard was open, we need to also know its height which is provided by useAnimatedKeyboard hook from react-native-reanimated, but we should always subtract safeArea.bottom from it since it's not part of the calculation for the offset, but only keyboardHeight.
Overall, the keyboard is the tricky part. On iOS, the keyboard doesn't resize the viewport. So for the content to not be covered by the keyboard,
KeyboardAvoidingViewis used. In React Native,KeyboardAvoidingViewis implemented as a view that changes its height or resizes the children's view by the keyboard height. It's done using RN's Layout Animations. This results in the animation for the keyboard being applied using the native event emitter callback which uses the bridge. So open/close events of the keyboard will always be delayed that resulting in the delayed keyboard avoiding animation and, which is more important, scheduling layout animations.In order to be consistent 100% of the time with animation and timings, we had to:
KeyboardAvoidingViewfromreact-native-keyboard-controllerfor iOS. It reacts on keyboard height/state change on UI thread in the same frame, so we can schedule an update for styles in the next frameuseDerivedValuefor all the logic, since introducinguseAnimatedReactionor having multiple shared values will delay updates for 1 frame, but we want to be 100% in sync with keyboard updatespaddingTop: translateY(becauseScrollViewis inverted).Another important aspect of the implementation is the usage of the state machine for the animation. State machine allows us to specify the chain of events and how to handle them, and more importantly what actions to ignore, for example:
KEYBOARD_OPENactionEMOJI_PICKER_OPENactionEMOJI_PICKER_CLOSEaction which again does nothingKEYBOARD_OPENaction. idle can react to this action by transitioning into keyboardOpen stateKEYBOARD_CLOSE,EMOJI_PICKER_OPENactionsEMOJI_PICKER_OPENaction which transitions our state intoemojiPickerOpenstate. now we react only toEMOJI_PICKER_CLOSEactionKEYBOARD_CLOSEaction. but we ignore it since ouremojiPickerOpenstate can only handleEMOJI_PICKER_CLOSEaction. so we write the logic for handling 9. hiding the keyboard but maintaining the offset based on the keyboard state shared valueEMOJI_PICKER_CLOSEaction which transitions us back intokeyboardOpenstate.Fixed Issues
$ #10632
PROPOSAL: N/A
Tests
These steps were performed on all the platforms. The new behavior is iOS only for now.
Before each next we make sure the keyboard is open:
Offline tests
Should not affect the offline state.
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
telegram-cloud-document-2-5460627828225625066.mp4
Android: mWeb Chrome
telegram-cloud-document-2-5460627828225625138.mp4
iOS: Native
1104ff6c-6eba-4295-a905-6caaaf3edf28.mp4
iOS: mWeb Safari
6eff007f-3c61-4f19-a60e-43f46d2f0e61.mp4
MacOS: Chrome / Safari
e5e28bd8-529f-4da3-8ae7-bff03bd14499.mp4
MacOS: Desktop
2c813bc9-6988-4fbc-8f36-3282c9cecd82.mp4