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

fix: Attachments reopen on multiple 'esc' click and hang on browser's navigation back #31524

Merged
merged 4 commits into from
Nov 24, 2023
Merged

fix: Attachments reopen on multiple 'esc' click and hang on browser's navigation back #31524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
487a794
Use focus with delay on modal close;
paultsimura Nov 19, 2023
b9b0f60
Refocus composer on ReportAttachments modal hide
paultsimura Nov 21, 2023
086e606
Fix composer losing focus on double 'esc' click
paultsimura Nov 21, 2023
0e6d7ed
Merge branch 'main' into fix/27237-attachments-hang
paultsimura Nov 22, 2023
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
4 changes: 1 addition & 3 deletions src/components/Modal/BaseModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function BaseModal(
*/
const hideModal = useCallback(
(callHideCallback = true) => {
Modal.willAlertModalBecomeVisible(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. What does this change provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I just moved Modal.willAlertModalBecomeVisible(false) from 2 useEffect hooks into hideModal.
This removes the race condition when double-clicking the esc button, as the hideModal is eventually called after each of those hooks, therefore Modal.willAlertModalBecomeVisible(false) will be called when the modal gets hidden. Without this change, the composer won't get focused when double-clicking esc.

I asked the original author here - there was no specific reason to put this call inside the hooks instead of the hideModal: #27639 (comment)

if (shouldSetModalVisibility) {
Modal.setModalVisibility(false);
}
Expand All @@ -77,8 +78,6 @@ function BaseModal(
Modal.willAlertModalBecomeVisible(true);
// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
removeOnCloseListener = Modal.setCloseModal(onClose);
} else if (wasVisible && !isVisible) {
Modal.willAlertModalBecomeVisible(false);
}

return () => {
Expand All @@ -96,7 +95,6 @@ function BaseModal(
return;
}
hideModal(true);
Modal.willAlertModalBecomeVisible(false);
},
// eslint-disable-next-line react-hooks/exhaustive-deps
[],
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ function ComposerWithSuggestions({
InputFocus.inputFocusChange(false);
return;
}
focus();
focus(true);
}, [focus, prevIsFocused, editFocused, prevIsModalVisible, isFocused, modal.isVisible, isNextModalWillOpenRef]);
useEffect(() => {
// Scrolls the composer to the bottom and sets the selection to the end, so that longer drafts are easier to edit
Expand Down
7 changes: 6 additions & 1 deletion src/pages/home/report/ReportAttachments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import PropTypes from 'prop-types';
import React, {useCallback} from 'react';
import _ from 'underscore';
import AttachmentModal from '@components/AttachmentModal';
import ComposerFocusManager from '@libs/ComposerFocusManager';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportUtils from '@libs/ReportUtils';
import ROUTES from '@src/ROUTES';
Expand Down Expand Up @@ -38,7 +39,11 @@ function ReportAttachments(props) {
defaultOpen
report={report}
source={source}
onModalHide={() => Navigation.dismissModal()}
onModalHide={() => {
Navigation.dismissModal();
// This enables Composer refocus when the attachments modal is closed by the browser navigation
ComposerFocusManager.setReadyToFocus();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulrahuman5196 I decided to go with my alternative solution for the Composer refocus on navigation because removing the !fullscreen check in hideModal would cause a regression on the Composer refocus after closing the Emoji picker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this change? What does change bring in? @paultsimura

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This focuses the composer when we navigate back by clicking the browser's "Back" button. Otherwise, ComposerFocusManager.setReadyToFocus() is called within BaseModal in 2 places:

App/src/components/Modal/BaseModal.tsx

Lines 115 to 117 in d521dd6

const handleDismissModal = () => {
ComposerFocusManager.setReadyToFocus();
};

App/src/components/Modal/BaseModal.tsx

Lines 66 to 68 in d521dd6

if (!fullscreen) {
ComposerFocusManager.setReadyToFocus();
}

The AttachmentsModal is fullscreen, so the hideModal does not call the ComposerFocusManager.setReadyToFocus().
Also, the Modal's onDismiss is not called when we are navigating – it's the RN limitation. The component just gets destroyed without calling the onDismiss.
Therefore, the ComposerFocusManager.setReadyToFocus() is not called in these two places in the BaseModal, so we need to call it explicitly.

}}
onCarouselAttachmentChange={onCarouselAttachmentChange}
/>
);
Expand Down