Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/components/EmojiPicker/EmojiPicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import withViewportOffsetTop from '@components/withViewportOffsetTop';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as Browser from '@libs/Browser';
import calculateAnchorPosition from '@libs/calculateAnchorPosition';
import CONST from '@src/CONST';
import EmojiPickerMenu from './EmojiPickerMenu';
Expand Down Expand Up @@ -169,6 +170,7 @@ const EmojiPicker = forwardRef((props, ref) => {
// emojis. The best alternative is to set it to 1ms so it just "pops" in and out
return (
<PopoverWithMeasuredContent
shouldHandleNavigationBack={Browser.isMobileChrome()}
isVisible={isEmojiPickerVisible}
onClose={hideEmojiPicker}
onModalShow={focusEmojiSearchInput}
Expand Down
22 changes: 20 additions & 2 deletions src/components/Modal/index.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import React, {useState} from 'react';
import React, {useEffect, useRef, useState} from 'react';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import StatusBar from '@libs/StatusBar';
import CONST from '@src/CONST';
import BaseModal from './BaseModal';
import type BaseModalProps from './types';

function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = () => {}, children, ...rest}: BaseModalProps) {
function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = () => {}, children, shouldHandleNavigationBack, ...rest}: BaseModalProps) {
const theme = useTheme();
const StyleUtils = useStyleUtils();
const [previousStatusBarColor, setPreviousStatusBarColor] = useState<string>();
Expand All @@ -22,8 +22,15 @@ function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = (
const hideModal = () => {
setStatusBarColor(previousStatusBarColor);
onModalHide();
if (window.history.state.shouldGoBack) {
window.history.back();
}
};

const handlePopStateRef = useRef(() => {
rest.onClose();
});

const showModal = () => {
const statusBarColor = StatusBar.getBackgroundColor() ?? theme.appBG;

Expand All @@ -35,9 +42,20 @@ function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = (
setStatusBarColor(isFullScreenModal ? theme.appBG : StyleUtils.getThemeBackgroundColor(statusBarColor));
}

if (shouldHandleNavigationBack) {
window.history.pushState({shouldGoBack: true}, '', null);
window.addEventListener('popstate', handlePopStateRef.current);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are using a ref for the popstate, and in some cases, the 'popstate' event uses a stale onClose function, which caused the issue described in Expensify/App#69781. We have applied the fix here:

#70820
#72200

}
Comment on lines +45 to +48

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This addition contributed to creating the following issue:

Explanation: (from this proposal RCA)
This popstate listener which will close the modal when triggered if shouldHandleNavigationBack is true (it's only true for emoji picker).

if (shouldHandleNavigationBack) {
window.history.pushState({shouldGoBack: true}, '', null);
window.addEventListener('popstate', handlePopStateRef.current);
}

It's triggered when we call back here when the modal is closed if the state contains shouldGoBack property that we push above.

const hideModal = () => {
setStatusBarColor(previousStatusBarColor);
onModalHide();
if ((window.history.state as WindowState)?.shouldGoBack) {
window.history.back();
}
};

However, the back is triggered even though we don't close the emoji picker. That's because it's triggered from the context menu hide. When we show the emoji picker, the context menu is closed which triggers the back. shouldGoBack exists in the history state because the emoji picker is shown first which already pushes the new state that sets shouldGoBack to true.

Long-press comment -> context menu shown -> press add emoji reaction -> push state, shouldGoBack: true -> context menu hides -> history.back -> triggers popstate -> emoji picker closes.

onModalShow?.();
};

useEffect(
() => () => {
window.removeEventListener('popstate', handlePopStateRef.current);
},
[],
);

return (
<BaseModal
// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
3 changes: 3 additions & 0 deletions src/components/Modal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ type BaseModalProps = Partial<ModalProps> & {
* */
hideModalContentWhileAnimating?: boolean;

/** Whether handle navigation back when modal show. */
shouldHandleNavigationBack?: boolean;

/** Should we use a custom backdrop for the modal? (This prevents focus issues on desktop) */
shouldUseCustomBackdrop?: boolean;
};
Expand Down
1 change: 1 addition & 0 deletions src/components/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ function Popover(props: PopoverProps) {
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
onClose={onCloseWithPopoverContext}
shouldHandleNavigationBack={props.shouldHandleNavigationBack}
type={isSmallScreenWidth ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.POPOVER}
popoverAnchorPosition={isSmallScreenWidth ? undefined : anchorPosition}
fullscreen={isSmallScreenWidth ? true : fullscreen}
Expand Down
3 changes: 3 additions & 0 deletions src/components/Popover/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ type PopoverProps = BaseModalProps &

/** Whether we want to show the popover on the right side of the screen */
fromSidebarMediumScreen?: boolean;

/** Whether handle navigation back when modal show. */
shouldHandleNavigationBack?: boolean;
};

type PopoverWithWindowDimensionsProps = PopoverProps & WindowDimensionsProps;
Expand Down
5 changes: 5 additions & 0 deletions src/components/PopoverWithMeasuredContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import type {WindowDimensionsProps} from './withWindowDimensions/types';
type PopoverWithMeasuredContentProps = Omit<PopoverProps, 'anchorPosition' | keyof WindowDimensionsProps> & {
/** The horizontal and vertical anchors points for the popover */
anchorPosition: AnchorPosition;

/** Whether handle navigation back when modal show. */
shouldHandleNavigationBack?: boolean;
};

/**
Expand Down Expand Up @@ -42,6 +45,7 @@ function PopoverWithMeasuredContent({
statusBarTranslucent = true,
avoidKeyboard = false,
hideModalContentWhileAnimating = false,
shouldHandleNavigationBack = false,
...props
}: PopoverWithMeasuredContentProps) {
const styles = useThemeStyles();
Expand Down Expand Up @@ -117,6 +121,7 @@ function PopoverWithMeasuredContent({
};
return isContentMeasured ? (
<Popover
shouldHandleNavigationBack={shouldHandleNavigationBack}
popoverDimensions={popoverDimensions}
anchorAlignment={anchorAlignment}
isVisible={isVisible}
Expand Down