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

35380 virtual viewport on report screen #38755

Merged
merged 10 commits into from
Mar 26, 2024
2 changes: 1 addition & 1 deletion src/components/EmojiPicker/EmojiPicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ const EmojiPicker = forwardRef((props, ref) => {
onModalShow={focusEmojiSearchInput}
onModalHide={onModalHide.current}
hideModalContentWhileAnimating
shouldSetModalVisibility={false}
shouldSetModalVisibility={Browser.isMobile()}
Copy link
Contributor

Choose a reason for hiding this comment

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

This also affects mChrome. Is it safe?
It would be good to add comment on platform specific changes like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it must be specific for mSafari, i have update and add comment

animationInTiming={1}
animationOutTiming={1}
anchorPosition={{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const useEmojiPickerMenu = () => {
const isListFiltered = allEmojis.length !== filteredEmojis.length;
const {preferredLocale} = useLocalize();
const [preferredSkinTone] = usePreferredEmojiSkinTone();
const {windowHeight} = useWindowDimensions();
const {windowHeight} = useWindowDimensions(true);
const StyleUtils = useStyleUtils();
const listStyle = StyleUtils.getEmojiPickerListHeight(isListFiltered, windowHeight);

Expand Down
7 changes: 7 additions & 0 deletions src/hooks/useViewportOffsetTop/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/**
* Native doesn't support DOM so default value is 0
*/

export default function useViewportOffsetTop(): number {
return 0;
}
51 changes: 51 additions & 0 deletions src/hooks/useViewportOffsetTop/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import {useEffect, useRef, useState} from 'react';
import addViewportResizeListener from '@libs/VisualViewport';

/**
* A hook that returns the offset of the top edge of the visual viewport
*/
export default function useViewportOffsetTop(shouldAdjustScrollView = false): number {
const [viewportOffsetTop, setViewportOffsetTop] = useState(0);
const initialHeight = useRef(window.visualViewport?.height ?? window.innerHeight).current;
const cachedDefaultOffsetTop = useRef<number>(0);
useEffect(() => {
const updateOffsetTop = (event?: Event) => {
let targetOffsetTop = window.visualViewport?.offsetTop || 0;

Check failure on line 13 in src/hooks/useViewportOffsetTop/index.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator
if (event?.target instanceof VisualViewport) {
targetOffsetTop = event.target.offsetTop;
}

if (shouldAdjustScrollView && window.visualViewport) {
const adjustScrollY = Math.round(initialHeight - window.visualViewport.height);
if (cachedDefaultOffsetTop.current === 0) {
cachedDefaultOffsetTop.current = targetOffsetTop;
}

if (adjustScrollY > targetOffsetTop) {
setViewportOffsetTop(adjustScrollY);
} else if (targetOffsetTop !== 0 && adjustScrollY === targetOffsetTop) {
setViewportOffsetTop(cachedDefaultOffsetTop.current);
} else {
setViewportOffsetTop(targetOffsetTop);
}
} else {
setViewportOffsetTop(targetOffsetTop);
}
};
updateOffsetTop();
const removeViewportResizeListener = addViewportResizeListener(updateOffsetTop);

return () => {
removeViewportResizeListener();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not needed right?

Suggested change
return () => {
removeViewportResizeListener();
};
return removeViewportResizeListener;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I have cleanup this function

}, [initialHeight, shouldAdjustScrollView]);

useEffect(() => {
if (!shouldAdjustScrollView) {
return;
}
window.scrollTo({top: viewportOffsetTop});
}, [shouldAdjustScrollView, viewportOffsetTop]);

return viewportOffsetTop;
}
1 change: 1 addition & 0 deletions src/libs/actions/Modal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
onModalClose();
onModalClose = null;
isNavigate = undefined;
setModalVisibility(false);

Check failure on line 63 in src/libs/actions/Modal.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

'setModalVisibility' was used before it was defined
Copy link
Contributor

@situchan situchan Mar 24, 2024

Choose a reason for hiding this comment

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

Please make sure that this change doesn't cause any regressions.
Especially when PopoverWithoutOverlay is opened and closed inside Modal on web platform.

Actually, which bug does this change fix?

Copy link
Contributor Author

@suneox suneox Mar 25, 2024

Choose a reason for hiding this comment

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

I'll check condition only setModalVisibility when current browser isMobileSafari

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @situchan i have updated the condition to avoid regression and we don't need to change setModalVisibility, so I have reverted code change related to setModalVisibility

}

/**
Expand Down
160 changes: 84 additions & 76 deletions src/pages/home/ReportScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
import TaskHeaderActionButton from '@components/TaskHeaderActionButton';
import withCurrentReportID from '@components/withCurrentReportID';
import type {CurrentReportIDContextValue} from '@components/withCurrentReportID';
import withViewportOffsetTop from '@components/withViewportOffsetTop';
import type {ViewportOffsetTopProps} from '@components/withViewportOffsetTop';
import useAppFocusEvent from '@hooks/useAppFocusEvent';
import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import useViewportOffsetTop from '@hooks/useViewportOffsetTop';
import useWindowDimensions from '@hooks/useWindowDimensions';
import Timing from '@libs/actions/Timing';
import * as Browser from '@libs/Browser';
import Navigation from '@libs/Navigation/Navigation';
import clearReportNotifications from '@libs/Notification/clearReportNotifications';
import reportWithoutHasDraftSelector from '@libs/OnyxSelectors/reportWithoutHasDraftSelector';
Expand All @@ -46,7 +46,8 @@
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';

Check failure on line 49 in src/pages/home/ReportScreen.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

'@src/types/onyx' imported multiple times
import type {Modal} from '@src/types/onyx';

Check failure on line 50 in src/pages/home/ReportScreen.tsx

View workflow job for this annotation

GitHub Actions / Run ESLint

'@src/types/onyx' imported multiple times
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import HeaderView from './HeaderView';
import ReportActionsView from './report/ReportActionsView';
Expand All @@ -55,6 +56,8 @@
import type {ActionListContextType, ReactionListRef, ScrollPosition} from './ReportScreenContext';

type ReportScreenOnyxProps = {
/** Indicates if there is a modal currently visible or not */
modal: OnyxEntry<Modal>;
/** Tells us if the sidebar has rendered */
isSidebarLoaded: OnyxEntry<boolean>;

Expand Down Expand Up @@ -93,7 +96,7 @@

type ReportScreenNavigationProps = StackScreenProps<CentralPaneNavigatorParamList, typeof SCREENS.REPORT>;

type ReportScreenProps = OnyxHOCProps & ViewportOffsetTopProps & CurrentReportIDContextValue & ReportScreenOnyxProps & ReportScreenNavigationProps;
type ReportScreenProps = OnyxHOCProps & CurrentReportIDContextValue & ReportScreenOnyxProps & ReportScreenNavigationProps;

/** Get the currently viewed report ID as number */
function getReportID(route: ReportScreenNavigationProps['route']): string {
Expand Down Expand Up @@ -131,7 +134,7 @@
markReadyForHydration,
policies = {},
isSidebarLoaded = false,
viewportOffsetTop,
modal = {isVisible: false},
Copy link
Contributor

Choose a reason for hiding this comment

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

default value here is redundant

isComposerFullSize = false,
userLeavingStatus = false,
currentReportID = '',
Expand Down Expand Up @@ -257,6 +260,8 @@
Timing.start(CONST.TIMING.CHAT_RENDER);
Performance.markStart(CONST.TIMING.CHAT_RENDER);
}
const [isComposerFocus, setIsComposerFocus] = useState(false);
const viewportOffsetTop = useViewportOffsetTop(Browser.isMobileSafari() && isComposerFocus && !modal?.isVisible);

const {reportPendingAction, reportErrors} = ReportUtils.getReportOfflinePendingActionAndErrors(report);
const screenWrapperStyle: ViewStyle[] = [styles.appContent, styles.flex1, {marginTop: viewportOffsetTop}];
Expand Down Expand Up @@ -638,6 +643,8 @@

{isReportReadyForDisplay ? (
<ReportFooter
onComposerFocus={() => setIsComposerFocus(true)}
onComposerBlur={() => setIsComposerFocus(false)}
report={report}
pendingAction={reportPendingAction}
isComposerFullSize={!!isComposerFullSize}
Expand All @@ -657,81 +664,82 @@

ReportScreen.displayName = 'ReportScreen';

export default withViewportOffsetTop(
withCurrentReportID(
withOnyx<ReportScreenProps, ReportScreenOnyxProps>(
{
isSidebarLoaded: {
key: ONYXKEYS.IS_SIDEBAR_LOADED,
},
sortedAllReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`,
canEvict: false,
selector: (allReportActions: OnyxEntry<OnyxTypes.ReportActions>) => ReportActionsUtils.getSortedReportActionsForDisplay(allReportActions, true),
},
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
allowStaleData: true,
selector: reportWithoutHasDraftSelector,
},
reportMetadata: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${getReportID(route)}`,
initialValue: {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
isLoadingNewerReportActions: false,
},
},
isComposerFullSize: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`,
initialValue: false,
},
betas: {
key: ONYXKEYS.BETAS,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
allowStaleData: true,
},
accountManagerReportID: {
key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID,
initialValue: null,
},
userLeavingStatus: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`,
initialValue: false,
export default withCurrentReportID(
withOnyx<ReportScreenProps, ReportScreenOnyxProps>(
{
modal: {
key: ONYXKEYS.MODAL,
},
isSidebarLoaded: {
key: ONYXKEYS.IS_SIDEBAR_LOADED,
},
sortedAllReportActions: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getReportID(route)}`,
canEvict: false,
selector: (allReportActions: OnyxEntry<OnyxTypes.ReportActions>) => ReportActionsUtils.getSortedReportActionsForDisplay(allReportActions, true),
},
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
allowStaleData: true,
selector: reportWithoutHasDraftSelector,
},
reportMetadata: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_METADATA}${getReportID(route)}`,
initialValue: {
isLoadingInitialReportActions: true,
isLoadingOlderReportActions: false,
isLoadingNewerReportActions: false,
},
parentReportAction: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
selector: (parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, props: WithOnyxInstanceState<ReportScreenOnyxProps>): OnyxEntry<OnyxTypes.ReportAction> => {
const parentReportActionID = props?.report?.parentReportActionID;
if (!parentReportActionID) {
return null;
}
return parentReportActions?.[parentReportActionID] ?? null;
},
canEvict: false,
},
isComposerFullSize: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_IS_COMPOSER_FULL_SIZE}${getReportID(route)}`,
initialValue: false,
},
betas: {
key: ONYXKEYS.BETAS,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
allowStaleData: true,
},
accountManagerReportID: {
key: ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID,
initialValue: null,
},
userLeavingStatus: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_LEAVING_ROOM}${getReportID(route)}`,
initialValue: false,
},
parentReportAction: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : 0}`,
selector: (parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, props: WithOnyxInstanceState<ReportScreenOnyxProps>): OnyxEntry<OnyxTypes.ReportAction> => {
const parentReportActionID = props?.report?.parentReportActionID;
if (!parentReportActionID) {
return null;
}
return parentReportActions?.[parentReportActionID] ?? null;
},
canEvict: false,
},
true,
)(
memo(
ReportScreen,
(prevProps, nextProps) =>
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
lodashIsEqual(prevProps.reportMetadata, nextProps.reportMetadata) &&
prevProps.isComposerFullSize === nextProps.isComposerFullSize &&
lodashIsEqual(prevProps.betas, nextProps.betas) &&
lodashIsEqual(prevProps.policies, nextProps.policies) &&
prevProps.accountManagerReportID === nextProps.accountManagerReportID &&
prevProps.userLeavingStatus === nextProps.userLeavingStatus &&
prevProps.currentReportID === nextProps.currentReportID &&
prevProps.viewportOffsetTop === nextProps.viewportOffsetTop &&
lodashIsEqual(prevProps.parentReportAction, nextProps.parentReportAction) &&
lodashIsEqual(prevProps.route, nextProps.route) &&
lodashIsEqual(prevProps.report, nextProps.report),
),
},
true,
)(
memo(
ReportScreen,
(prevProps, nextProps) =>
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
lodashIsEqual(prevProps.reportMetadata, nextProps.reportMetadata) &&
prevProps.isComposerFullSize === nextProps.isComposerFullSize &&
lodashIsEqual(prevProps.betas, nextProps.betas) &&
lodashIsEqual(prevProps.policies, nextProps.policies) &&
prevProps.accountManagerReportID === nextProps.accountManagerReportID &&
prevProps.userLeavingStatus === nextProps.userLeavingStatus &&
prevProps.currentReportID === nextProps.currentReportID &&
lodashIsEqual(prevProps.modal, nextProps.modal) &&
lodashIsEqual(prevProps.parentReportAction, nextProps.parentReportAction) &&
lodashIsEqual(prevProps.route, nextProps.route) &&
lodashIsEqual(prevProps.report, nextProps.report),
),
),
);
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ type SuggestionsRef = {
updateShouldShowSuggestionMenuToFalse: (shouldShowSuggestionMenu?: boolean) => void;
setShouldBlockSuggestionCalc: (shouldBlock: boolean) => void;
getSuggestions: () => Mention[] | Emoji[];
onComposerFocus: () => void;
onComposerBlur: () => void;
};

type ReportActionComposeOnyxProps = {
Expand All @@ -85,6 +87,9 @@ type ReportActionComposeProps = ReportActionComposeOnyxProps &

/** Whether the report is ready for display */
isReportReadyForDisplay?: boolean;
} & {
onComposerFocus: () => {};
onComposerBlur: () => {};
};

// We want consistent auto focus behavior on input between native and mWeb so we have some auto focus management code that will
Expand All @@ -107,6 +112,8 @@ function ReportActionCompose({
isReportReadyForDisplay = true,
isEmptyChat,
lastReportAction,
onComposerFocus,
onComposerBlur,
}: ReportActionComposeProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
Expand Down Expand Up @@ -296,6 +303,7 @@ function ReportActionCompose({
const onBlur = useCallback((event: NativeSyntheticEvent<TextInputFocusEventData>) => {
const webEvent = event as unknown as FocusEvent;
setIsFocused(false);
onComposerBlur();
if (suggestionsRef.current) {
suggestionsRef.current.resetSuggestions();
}
Expand All @@ -304,8 +312,14 @@ function ReportActionCompose({
}
}, []);

const onFocus = useCallback(() => {
const handleFocus = useCallback(() => {
setIsFocused(true);
onComposerFocus();
}, []);

const handleBlur = useCallback(() => {
setIsFocused(false);
onComposerBlur();
}, []);

// resets the composer to normal size when
Expand Down Expand Up @@ -436,8 +450,8 @@ function ReportActionCompose({
setIsCommentEmpty={setIsCommentEmpty}
handleSendMessage={handleSendMessage}
shouldShowComposeInput={shouldShowComposeInput}
onFocus={onFocus}
onBlur={onBlur}
onFocus={handleFocus}
onBlur={handleBlur}
measureParentContainer={measureContainer}
listHeight={listHeight}
onValueChange={(value) => {
Expand Down
Loading
Loading