Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIGH: (Comment linking: step 2) [23220] WEB maintain visible content position #32098

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
97b0a29
Merge branch 'main' of https://github.com/Expensify/App
perunt Nov 15, 2023
a7360c4
Merge branch 'main' of https://github.com/Expensify/App
perunt Nov 23, 2023
d624dd3
create MVCPFlatList
perunt Nov 28, 2023
a34fed2
use MVCPFlatList
perunt Nov 28, 2023
e642647
Merge branch 'perunt/chat-loader-flickering' of https://github.com/ma…
perunt Nov 28, 2023
7dd2e8f
Merge branch 'perunt/chat-loader-flickering' of https://github.com/ma…
perunt Nov 28, 2023
5d67d02
move scrollToOffset into requestAnimationFrame
perunt Nov 28, 2023
ace8192
Merge branch 'perunt/chat-loader-flickering' of https://github.com/ma…
perunt Nov 29, 2023
9f5084d
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 6, 2023
e9fa5bf
fix scrolling
perunt Dec 11, 2023
731514e
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 11, 2023
5e6d6f6
remove scrollToOffset
perunt Dec 11, 2023
8042282
remove autoscrollToTopThreshold
perunt Dec 19, 2023
a22fabd
automatically scroll to the bottom of the chat when the component mounts
perunt Dec 19, 2023
3259670
fix MVCPFlatList
perunt Dec 19, 2023
cd317c4
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 19, 2023
1ac24bb
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 21, 2023
62ed316
post-merge updates
perunt Dec 21, 2023
24f0fdc
Remove rn web invertion patch
janicduplessis Dec 21, 2023
4f99585
Merge branch 'feat/##23220-web-maintainVisibleContentPosition' of htt…
perunt Dec 21, 2023
760eba2
Inverted fix
janicduplessis Dec 22, 2023
ebfd970
Merge branch 'feat/##23220-web-maintainVisibleContentPosition' of htt…
perunt Dec 28, 2023
e7d3077
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 28, 2023
8118a2d
Merge branch 'perunt/bidirectional-list-loader-mobile-fix' of https:/…
perunt Dec 28, 2023
8787cb1
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 29, 2023
dbdb226
fix after merge
perunt Dec 29, 2023
b68e8bf
use CellRendererComponent
perunt Dec 29, 2023
d173edb
fix reassurePerformanceTests
perunt Dec 29, 2023
0ba8d40
remove node_modules
perunt Dec 29, 2023
7ecd381
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 29, 2023
ee09b8a
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 30, 2023
481bca5
the package name is extracted from the patch file name
perunt Dec 31, 2023
69ee59b
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Dec 31, 2023
cb4e9b7
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Jan 2, 2024
a26402d
cleaning up reassurePerformanceTests
perunt Jan 2, 2024
2babd91
cache clean
perunt Jan 2, 2024
d6f7c55
patch-package
perunt Jan 2, 2024
ff502fa
move reportScrollManager from ReportActionsView to ReportActionsList to
perunt Jan 2, 2024
c9efdc2
do patch-package again
perunt Jan 2, 2024
a05adff
postinstall update
perunt Jan 2, 2024
941ca04
undo postinstall
perunt Jan 2, 2024
454f8a1
Backup current patches
perunt Jan 2, 2024
e5568dc
indentation
perunt Jan 2, 2024
bf5b980
undo reassurePerformanceTests
perunt Jan 2, 2024
ae17f61
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Jan 4, 2024
42ec7a7
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Jan 5, 2024
356adf8
cleanup reassurePerformanceTests
perunt Jan 5, 2024
f23ef43
lint
perunt Jan 5, 2024
1cf27ad
add spaces after linting
perunt Jan 5, 2024
54921a9
Merge branch 'main' of https://github.com/Expensify/App into feat/##2…
perunt Jan 5, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/reassurePerformanceTests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,3 @@ jobs:
with:
DURATION_DEVIATION_PERCENTAGE: 20
COUNT_DEVIATION: 0

872 changes: 617 additions & 255 deletions patches/react-native-web+0.19.9+001+initial.patch

Large diffs are not rendered by default.

687 changes: 0 additions & 687 deletions patches/react-native-web+0.19.9+002+fix-mvcp.patch

This file was deleted.

207 changes: 207 additions & 0 deletions src/components/FlatList/MVCPFlatList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
/* eslint-disable es/no-optional-chaining, es/no-nullish-coalescing-operators, react/prop-types */
Copy link
Contributor

@roryabraham roryabraham Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for posterity: these were disabled to keep this code as similar as possible to the upstream PR it's based on: necolas/react-native-web#2588

Hopefully this component only lives in our codebase relatively temporarily. We probably will want to switch to FlashList soon too for the main chat list.

import PropTypes from 'prop-types';
import React from 'react';
import {FlatList} from 'react-native';

function mergeRefs(...args) {
return function forwardRef(node) {
args.forEach((ref) => {
if (ref == null) {
return;
}
if (typeof ref === 'function') {
ref(node);
return;
}
if (typeof ref === 'object') {
// eslint-disable-next-line no-param-reassign
ref.current = node;
return;
}
console.error(`mergeRefs cannot handle Refs of type boolean, number or string, received ref ${String(ref)}`);
});
};
}

function useMergeRefs(...args) {
return React.useMemo(
() => mergeRefs(...args),
// eslint-disable-next-line
[...args],
);
}

const MVCPFlatList = React.forwardRef(({maintainVisibleContentPosition, horizontal, onScroll, ...props}, forwardedRef) => {
const {minIndexForVisible: mvcpMinIndexForVisible, autoscrollToTopThreshold: mvcpAutoscrollToTopThreshold} = maintainVisibleContentPosition ?? {};
const scrollRef = React.useRef(null);
const prevFirstVisibleOffsetRef = React.useRef(null);
const firstVisibleViewRef = React.useRef(null);
const mutationObserverRef = React.useRef(null);
const lastScrollOffsetRef = React.useRef(0);

const getScrollOffset = React.useCallback(() => {
if (scrollRef.current == null) {
return 0;
}
return horizontal ? scrollRef.current.getScrollableNode().scrollLeft : scrollRef.current.getScrollableNode().scrollTop;
}, [horizontal]);

const getContentView = React.useCallback(() => scrollRef.current?.getScrollableNode().childNodes[0], []);

const scrollToOffset = React.useCallback(
(offset, animated) => {
const behavior = animated ? 'smooth' : 'instant';
scrollRef.current?.getScrollableNode().scroll(horizontal ? {left: offset, behavior} : {top: offset, behavior});
},
[horizontal],
);

const prepareForMaintainVisibleContentPosition = React.useCallback(() => {
if (mvcpMinIndexForVisible == null) {
return;
}

const contentView = getContentView();
if (contentView == null) {
return;
}

const scrollOffset = getScrollOffset();

const contentViewLength = contentView.childNodes.length;
for (let i = mvcpMinIndexForVisible; i < contentViewLength; i++) {
const subview = contentView.childNodes[i];
const subviewOffset = horizontal ? subview.offsetLeft : subview.offsetTop;
if (subviewOffset > scrollOffset || i === contentViewLength - 1) {
prevFirstVisibleOffsetRef.current = subviewOffset;
firstVisibleViewRef.current = subview;
break;
}
}
}, [getContentView, getScrollOffset, mvcpMinIndexForVisible, horizontal]);

const adjustForMaintainVisibleContentPosition = React.useCallback(() => {
if (mvcpMinIndexForVisible == null) {
return;
}

const firstVisibleView = firstVisibleViewRef.current;
const prevFirstVisibleOffset = prevFirstVisibleOffsetRef.current;
if (firstVisibleView == null || prevFirstVisibleOffset == null) {
return;
}

const firstVisibleViewOffset = horizontal ? firstVisibleView.offsetLeft : firstVisibleView.offsetTop;
const delta = firstVisibleViewOffset - prevFirstVisibleOffset;
if (Math.abs(delta) > 0.5) {
const scrollOffset = getScrollOffset();
prevFirstVisibleOffsetRef.current = firstVisibleViewOffset;
scrollToOffset(scrollOffset + delta, false);
if (mvcpAutoscrollToTopThreshold != null && scrollOffset <= mvcpAutoscrollToTopThreshold) {
scrollToOffset(0, true);
}
}
}, [getScrollOffset, scrollToOffset, mvcpMinIndexForVisible, mvcpAutoscrollToTopThreshold, horizontal]);
Copy link
Contributor

@s77rt s77rt Jul 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having mvcpAutoscrollToTopThreshold as a dependency made this function gets re-evaluated on value change. Which also made the setupMutationObserver function re-evaluated and executed (due to useEffect). That later function will disconnect the old observer and create a new one. This constant observer recreation is found to disrupt the new message detection.

(Coming from #43600)


const setupMutationObserver = React.useCallback(() => {
const contentView = getContentView();
if (contentView == null) {
return;
}

mutationObserverRef.current?.disconnect();

const mutationObserver = new MutationObserver(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When list is hidden and reappears, it scrolls down to the bottom. But it should keep its last scroll offset. That is why ignore this callback when list is hidden. Coming from #45434

// This needs to execute after scroll events are dispatched, but
// in the same tick to avoid flickering. rAF provides the right timing.
requestAnimationFrame(() => {
// Chrome adjusts scroll position when elements are added at the top of the
// view. We want to have the same behavior as react-native / Safari so we
// reset the scroll position to the last value we got from an event.
const lastScrollOffset = lastScrollOffsetRef.current;
const scrollOffset = getScrollOffset();
if (lastScrollOffset !== scrollOffset) {
scrollToOffset(lastScrollOffset, false);
}

adjustForMaintainVisibleContentPosition();
});
});
mutationObserver.observe(contentView, {
attributes: true,
childList: true,
subtree: true,
});

mutationObserverRef.current = mutationObserver;
}, [adjustForMaintainVisibleContentPosition, getContentView, getScrollOffset, scrollToOffset]);

React.useEffect(() => {
requestAnimationFrame(() => {
prepareForMaintainVisibleContentPosition();
setupMutationObserver();
});
}, [prepareForMaintainVisibleContentPosition, setupMutationObserver]);

const setMergedRef = useMergeRefs(scrollRef, forwardedRef);

const onRef = React.useCallback(
(newRef) => {
// Make sure to only call refs and re-attach listeners if the node changed.
if (newRef == null || newRef === scrollRef.current) {
return;
}

setMergedRef(newRef);
prepareForMaintainVisibleContentPosition();
setupMutationObserver();
},
[prepareForMaintainVisibleContentPosition, setMergedRef, setupMutationObserver],
);

React.useEffect(() => {
const mutationObserver = mutationObserverRef.current;
return () => {
mutationObserver?.disconnect();
};
}, []);

const onScrollInternal = React.useCallback(
(ev) => {
lastScrollOffsetRef.current = getScrollOffset();

prepareForMaintainVisibleContentPosition();

onScroll?.(ev);
},
[getScrollOffset, prepareForMaintainVisibleContentPosition, onScroll],
);

return (
<FlatList
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
maintainVisibleContentPosition={maintainVisibleContentPosition}
horizontal={horizontal}
onScroll={onScrollInternal}
scrollEventThrottle={1}
ref={onRef}
/>
);
});

MVCPFlatList.displayName = 'MVCPFlatList';
MVCPFlatList.propTypes = {
maintainVisibleContentPosition: PropTypes.shape({
minIndexForVisible: PropTypes.number.isRequired,
autoscrollToTopThreshold: PropTypes.number,
}),
horizontal: PropTypes.bool,
};

MVCPFlatList.defaultProps = {
maintainVisibleContentPosition: null,
horizontal: false,
};

export default MVCPFlatList;
3 changes: 3 additions & 0 deletions src/components/FlatList/index.web.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import MVCPFlatList from './MVCPFlatList';

export default MVCPFlatList;
2 changes: 0 additions & 2 deletions src/components/InvertedFlatList/BaseInvertedFlatList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import React, {forwardRef} from 'react';
import type {FlatListProps} from 'react-native';
import FlatList from '@components/FlatList';

const AUTOSCROLL_TO_TOP_THRESHOLD = 128;
const WINDOW_SIZE = 15;

function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>) {
Expand All @@ -15,7 +14,6 @@ function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<Flat
windowSize={WINDOW_SIZE}
maintainVisibleContentPosition={{
minIndexForVisible: 0,
autoscrollToTopThreshold: AUTOSCROLL_TO_TOP_THRESHOLD,
}}
inverted
/>
Expand Down
2 changes: 2 additions & 0 deletions src/components/InvertedFlatList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {FlatList, FlatListProps, NativeScrollEvent, NativeSyntheticEvent} f
import {DeviceEventEmitter} from 'react-native';
import CONST from '@src/CONST';
import BaseInvertedFlatList from './BaseInvertedFlatList';
import CellRendererComponent from './CellRendererComponent';

// This is adapted from https://codesandbox.io/s/react-native-dsyse
// It's a HACK alert since FlatList has inverted scrolling on web
Expand Down Expand Up @@ -87,6 +88,7 @@ function InvertedFlatList<T>({onScroll: onScrollProp = () => {}, ...props}: Flat
{...props}
ref={ref}
onScroll={handleScroll}
CellRendererComponent={CellRendererComponent}
/>
);
}
Expand Down
7 changes: 0 additions & 7 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,6 @@ function ReportScreen({
const onSubmitComment = useCallback(
(text) => {
Report.addComment(getReportID(route), text);

// We need to scroll to the bottom of the list after the comment is added
const refID = setTimeout(() => {
flatListRef.current.scrollToOffset({animated: false, offset: 0});
}, 10);

return () => clearTimeout(refID);
},
[route],
);
Expand Down
13 changes: 10 additions & 3 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {useRoute} from '@react-navigation/native';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {DeviceEventEmitter} from 'react-native';
import {DeviceEventEmitter, InteractionManager} from 'react-native';
import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated';
import _ from 'underscore';
import InvertedFlatList from '@components/InvertedFlatList';
Expand Down Expand Up @@ -138,7 +138,6 @@ function ReportActionsList({
isComposerFullSize,
}) {
const styles = useThemeStyles();
const reportScrollManager = useReportScrollManager();
const {translate} = useLocalize();
const {isOffline} = useNetwork();
const route = useRoute();
Expand All @@ -151,6 +150,7 @@ function ReportActionsList({
}
return cacheUnreadMarkers.get(report.reportID);
};
const reportScrollManager = useReportScrollManager();
const [currentUnreadMarker, setCurrentUnreadMarker] = useState(markerInit);
const scrollingVerticalOffset = useRef(0);
const readActionSkipped = useRef(false);
Expand Down Expand Up @@ -252,6 +252,13 @@ function ReportActionsList({
});
}, [report.reportID]);

useEffect(() => {
InteractionManager.runAfterInteractions(() => {
reportScrollManager.scrollToBottom();
});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
// Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function?
// Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted,
Expand All @@ -273,7 +280,7 @@ function ReportActionsList({
if (!isFromCurrentUser) {
return;
}
reportScrollManager.scrollToBottom();
InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
});

const cleanup = () => {
Expand Down
Loading