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.

Sign up for GitHub

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

Jump to bottom

Fix cannot open search page by shortcut if delete receipt confirm modal is visible #33203

Merged
merged 11 commits into from
Jan 25, 2024
Merged

Fix cannot open search page by shortcut if delete receipt confirm modal is visible #33203

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e5de1c7
Fix cannot open search page by shortcut if delete receipt confirm mod…
tsa321 Dec 16, 2023
d8d31f9
Remove isNavigating parameter
tsa321 Dec 17, 2023
8055f3c
Merge branch 'main' into seashortc-drconfirm
tsa321 Jan 16, 2024
cc393f3
Merge branch 'main' into seashortc-drconfirm
tsa321 Jan 18, 2024
c2547ef
Preserve the isNavigating parameter
tsa321 Jan 22, 2024
cf7c98d
add Reset isNavigate value
tsa321 Jan 22, 2024
e15c964
revert resetting isNavigate
tsa321 Jan 22, 2024
76c565a
Merge branch 'main' into seashortc-drconfirm
tsa321 Jan 23, 2024
d0f1991
add reset for isNavigate
tsa321 Jan 23, 2024
bf23434
Merge 'main' and resolve conflict
tsa321 Jan 25, 2024
dd7a6f1
Fix typecheck
tsa321 Jan 25, 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
2 changes: 1 addition & 1 deletion src/components/Modal/BaseModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ function BaseModal(
onBackdropPress={handleBackdropPress}
// Note: Escape key on web/desktop will trigger onBackButtonPress callback
// eslint-disable-next-line react/jsx-props-no-multi-spaces
onBackButtonPress={onClose}
onBackButtonPress={Modal.closeTop}
onModalShow={handleShowModal}
propagateSwipe={propagateSwipe}
onModalHide={hideModal}
Expand Down
1 change: 1 addition & 0 deletions src/components/ThreeDotsMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, me
hidePopoverMenu();
return;
}
buttonRef.current.blur();
Copy link
Contributor Author

@tsa321 tsa321 Dec 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is needed to prevent react native modal to show blue border box (because of focused state) of threedots button on modal hide when user close modal by pressing esc:

Video:
blue-border-d.mp4

This is because of trap focus of react native modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not able to reproduce. Btw, if you reproduced, can you confirm also reproducible on staging/production?
If so, no need to fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aimane-chnaif Because I am using onBackButtonPress={Modal.closeTop} in BaseModal.tsx to fix an issue about: When user press three dots menu on attachment modal then press esc the attachment modal is closed. The current behavior in staging:

Pressing esc on three dots menu in attachment modal:
esc-on-staging-d.mp4

After using onBackButtonPress={Modal.closeTop} :

blue border appears:
blue-border-current-branch-d.mp4

showPopoverMenu();
if (onIconPress) {
onIconPress();
Expand Down
24 changes: 18 additions & 6 deletions src/libs/actions/Modal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';

const closeModals: Array<(isNavigating: boolean) => void> = [];
const closeModals: Array<() => void> = [];

let onModalClose: null | (() => void);

Expand All @@ -21,24 +21,36 @@ function setCloseModal(onClose: () => void) {
};
}

/**
* Close topmost modal
*/
function closeTop() {
if (closeModals.length === 0) {
return;
}
closeModals[closeModals.length - 1]();
}

/**
* Close modal in other parts of the app
*/
function close(onModalCloseCallback: () => void, isNavigating = true) {
function close(onModalCloseCallback: () => void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isNavigating is still needed.

As you see, composer is briefly focused

Screen.Recording.2024-01-21.at.8.13.56.PM.mov

this logic was added here

used in

App/src/components/EmojiPicker/EmojiPicker.js

Lines 85 to 88 in 52adf77

const hideEmojiPicker = (isNavigating) => {
if (isNavigating) {
onModalHide.current = () => {};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aimane-chnaif done preserving the isNavigating parameter

if (closeModals.length === 0) {
onModalCloseCallback();
return;
}
onModalClose = onModalCloseCallback;
[...closeModals].reverse().forEach((onClose) => {
onClose(isNavigating);
});
closeTop();
}

function onModalDidClose() {
if (!onModalClose) {
return;
}
if (closeModals.length) {
closeTop();
return;
}
onModalClose();
onModalClose = null;
}
Expand All @@ -58,4 +70,4 @@ function willAlertModalBecomeVisible(isVisible: boolean) {
Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible});
}

export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible};
export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible, closeTop};
Loading