melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kbecciv closed 11 months ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01ebd6c095f0ad277c
melvin-bot[bot]
commented
1 year ago Platforms in OP are ✅)melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)
ishpaul777
commented
1 year ago Pressing ESC Attachement preview glitches the preview.
The root cause is when keyboard shortcut is triggered we call Modal.close(() => callback), here modal.close is dependent on the onClose prop which we pass in Modal component. But in this case we have 2 Modals in stack of AttachmentsModal and other is ConfirmModal used for invalid file type.
Conditionally rendering the ConfirmModal based on the whether there is a validation error or not, and Modifying onModalHide prop passed to AttachementModal will solve this issue.
// AttachmentsModal.js
{
isAttachmentInvalid && (
<ConfirmModal
title={attachmentInvalidReasonTitle ? translate(attachmentInvalidReasonTitle) : ''}
onConfirm={closeConfirmModal}
onCancel={closeConfirmModal}
isVisible={isAttachmentInvalid}
prompt={attachmentInvalidReason ? translate(attachmentInvalidReason) : ''}
confirmText={translate('common.close')}
shouldShowCancelButton={false}
/>
)
}// ReportAttachments.js
<AttachmentModal
onModalHide={() => {
// if active route is not the attachment route, user maybe navigated to search using KeyboardShortcut Cmd+K/Cmd+Shift+K below can be moved to dismissModal function also
const activeRoute = Navigation.getActiveRoute();
if (activeRoute.includes('/attachment')) {
Navigation.dismissModal(reportID)
}
}}
/// other props
/>As this problem can be solved by conditionally rendering the ConfirmModal. There is a downside to the approach we are following as explained in root cause, we can only have one modal in Stack at a time. So where there is multiple modals in stack, we have to conditionally render them. A better approach would be to use a Array to store the onClose, and setOnClose function will push the onClose function to the array along with a ModalID. When unmounting the modal will remove the onClose function from the array. This way we can have multiple modals in stack and onClose will be called in the order of modals in stack. Pseudo Code
zukilover
commented
1 year ago App glitches on closing preview on ESC click
AttachmentModal is using unnecessary useCallback for closeModal function. This causes React to not throw away the cached function.
Remove the useCallback wrapper for closeModal, e.g.:
const closeModal = () => {
setIsModalOpen(false);
};This is the safest and optimal way since the modal visibility is controlled by a local state.
N/A
ishpaul777
commented
1 year ago While my solution is solving the issue I think the root cause is correctly identified by @zukilover, the solution proposed by me is solving the issue of Navigation issue when CMD+K or CMD+SHIFT+K is used for navigation (reported by a contributor on slack already) I am marking my off-topic (unless we decide to solve this issue along with this).
Christinadobrzyn
commented
1 year ago Not sure why @adelekennedy and I are both assigned but I did the testing and can reproduce this only in Chrome. Looks like External is already added for some proposals.
Ah this is why we were both added - https://expensify.slack.com/archives/C01SKUP7QR0/p1694216537643229
astrohunter62
commented
1 year ago Pressing or double pressing ESC on the attachment preview glitches the preview
This issue is caused by the onViewableItemsChanged props.
The onViewableItemsChanged is still being invoked even after navigating to a different screen in the React Native FlatList.
We can add and change the focused boolean value with navigation eventListeners 'blur' and 'focus'.
For example: src/components/Attachments/AttachmentCarousel/index.js
const [focused, setFocused] = useState();
const navigation = useNavigation();
useEffect(() => {
const subscribeFocusEvent = navigation.addListener('focus', () => {
setFocused(true);
});
const subscribeBlurEvent = navigation.addListener('blur', () => {
setFocused(false);
});
return () => {
subscribeFocusEvent();
subscribeBlurEvent();
};
}, [focused, navigation]);
.....
{containerWidth > 0 && focused && (
<FlatList
....const renderItem = useCallback(
({item}) =>
focused ? (
<CarouselItem
item={item}
isFocused={activeSource === item.source}
onPress={canUseTouchScreen ? () => setShouldShowArrows(!shouldShowArrows) : undefined}
/>
) : (
<></>
),
[activeSource, focused, canUseTouchScreen, setShouldShowArrows, shouldShowArrows],
);https://github.com/Expensify/App/assets/136574831/5821d38d-5afe-4e1f-8355-f6627ce98e15
https://github.com/Expensify/App/assets/136574831/ac4a25cf-3a3c-478f-a583-6139942346d3
melvin-bot[bot]
commented
1 year ago 📣 @astrohunter62! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>✅ Contributor details stored successfully. Thank you for contributing to Expensify!
Christinadobrzyn
commented
1 year ago I think we're reviewing proposals!
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Christinadobrzyn
commented
1 year ago hi @abdulrahuman5196! Can you let me know what you think of the current proposals and if you'd like to see more?
Christinadobrzyn
commented
1 year ago Hey @abdulrahuman5196 would you like to see more proposals?
Christinadobrzyn
commented
1 year ago reached out to @abdulrahuman5196 to see if we can get a review of these
melvin-bot[bot]
commented
1 year ago @abdulrahuman5196 @Christinadobrzyn this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
mvtglobally
commented
1 year ago Issue not reproducible during KI retests. (First week)
abdulrahuman5196
commented
1 year ago Reviewing now
abdulrahuman5196
commented
1 year ago @Christinadobrzyn I am not able to repro the issue in the latest main. Could you kindly check again if reproducible?
Christinadobrzyn
commented
1 year ago Yep! This is no longer happening. I'll ask QA to test just to make sure.
https://expensify.slack.com/archives/C9YU7BX5M/p1695945411896269
kavimuru
commented
1 year ago @Christinadobrzyn I am not able to reproduce
https://github.com/Expensify/App/assets/43996225/f897a7d4-410c-433d-913c-f332fa8c137e
astrohunter62
commented
1 year ago Hi, @Christinadobrzyn, did you press ECS twice on the first attachment preview?
Christinadobrzyn
commented
1 year ago Oh no I didn't let me try that @astrohunter62
Christinadobrzyn
commented
1 year ago Ah, it is still happening when you press Esc multiple times... I'll update the OP. Thanks for catching that @astrohunter62!
keeping this open. @abdulrahuman5196 could you try again to reproduce based on the steps in the OP?
astrohunter62
commented
1 year ago I updated my proposal https://github.com/Expensify/App/issues/27237#issuecomment-1719794579
abdulrahuman5196
commented
1 year ago Rechecking this again.
abdulrahuman5196
commented
1 year ago Got it. able to repro the issue if Esc is pressed multiple times
abdulrahuman5196
commented
1 year ago On @zukilover 's proposal here https://github.com/Expensify/App/issues/27237#issuecomment-1715649797 I am not sure of the root cause, but i tried the solution. It is not solving the issue.
On @astrohunter62 's proposal here https://github.com/Expensify/App/issues/27237#issuecomment-1719794579. I not sure if the root cause is correct. But the solution suggestion like a workaround to not show FlatList on non focused state to avoid the issue. So I am not inclined to the solution.
@Christinadobrzyn Currently there is no proposal to approve, can we increase the bounty given that the issue is couple of weeks old already.
astrohunter62
commented
1 year ago @abdulrahuman5196 I believe the root cause is valid. When I press the 'ESC' key, it triggers the 'onClose' function in the modal. If I click 'ESC' before the 'onModalHide' event is triggered, the 'AttachmentCarousel' is re-rendered with the last item as the entry item. Consequently, the modal experiences a glitch.
melvin-bot[bot]
commented
1 year ago @abdulrahuman5196 @Christinadobrzyn this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
melvin-bot[bot]
commented
1 year ago Upwork job price has been updated to $1000
Christinadobrzyn
commented
1 year ago Increased the bounty on this one to get more proposals.
ilkeruyanik
commented
1 year ago When the Attachment Carousel is open, pressing the "ESC" key multiple times causes the modal to close and then reopen.
The SidebarLinks component has an event listener for the "ESC" key, but it doesn't check whether the modal is currently visible or not. When the "ESC" key is pressed, it triggers the Navigation.dismissModal() function with check modal.willAlertModalBecomeVisible.
Additionally, the AttachmentModal also calls Navigation.dismissModal() when it hides modal.
BaseModal component close modal with onBackButtonPress prop, and AttachmentModal call Navigation.dismissModal().SidebarLinks component's event listener trigger Navigation.dismissModal().This leads to two consecutive calls to Navigation.dismissModal(), causing the issue.
Furthermore, the BaseModal component only closes the modal with the "ESC" key using its onBackButtonPress prop, but it doesn't update the modal's visibility status using Modal.setModalVisibility(false). This action write Onyx to modal's visibility.
To prevent multiple calls to Navigation.dismissModal(), we will check modal visibility in SidebarLinks component with
if(modal.isVisible){
Navigation.dismissModal();
}Also, we will add Modal.setModalVisibility(false) to BaseModal onBackButtonPress prop.
onBackButtonPress={() => {
Modal.setModalVisibility(false);
onClose();
}}By implementing these two changes, we will prevent multiple calls to Navigation.dismissModal(). The onBackButtonPress action will set the modal's visibility to false when the modal is closed with the "ESC" key, ensuring that it won't be called again in the SidebarLinks component.
N/A
tsa321
commented
1 year ago Pressing ESC on attachment preview multiple times will cause glitches on the preview modal
In the SidebarLink.js there is Esc key subscription that if Modal.willAlertModalBecomeVisible is true (moda is available), the Navigation.dismissModal(); won't get executed.
But when we press esc on attachment preview, there is animation going on to close the modal and at the start of the animation we already set Modal.willAlertModalBecomeVisible to false, without waiting the modal to really disappear.
So it will cause the esc key caught by SidebarLink will pass that check and execute Navigation.dismissModal and will cause weird following behavior.
For more explanation of the root cause I have commented in the comment below. Here I am including the root cause:
After checking some modals and RHN panel (opening RHN will set Modal.isVisible to true), I think the best way to fix this problem is to set Modal.willAlertModalBecomeVisible to false when modal really disappear. So, we could remove the set here:
https://github.com/Expensify/App/blob/299e054aba3324fee224e2bd5ad144507e6763f2/src/components/Modal/BaseModal.js#L89
and move it inside hideModal:
This is needed to prevent the hide modal animation to end abruptly while it is still running when user pressing esc for the second time. I have explained it in the root cause, if we increase animationOut of the basemodal this will be really visible.
For the issue of the app hangs when user press back button, This can be fixed by setting the parameter of focus to true in here:
The parameter true is to wait for ComposerFocusManageer.isReadyToFocus() :
https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/libs/focusWithDelay.ts#L25
then focus the textinput.
and set ComposerFocusManager.resetReadyToFocus(); in isvisible clause:
also set the ComposerFocusManager.setReadyToFocus(); inside hideModal or remove the check full screen condition of the ComposerFocusManager.setReadyToFocus();
(For the first bug) Or we could add willAlertModalBecomeInvisible field in Onyx modal in case moving willAlertModalBecomeVisible inside hideModal causing a regression. But I have tested it and I haven't found any regression yet when moving set willAlertModalBecomeVisible(false) into hideModal.
shubham1206agra
commented
1 year ago @bernhardoj Can you confirm https://github.com/Expensify/App/issues/23959 will fix this too?
bernhardoj
commented
1 year ago @shubham1206agra I'm pretty sure it's unrelated and won't fix this issue.
shubham1206agra
commented
1 year ago @bernhardoj For me, it is pretty related cause when we move the modalHide logic to onDismiss, we will not have race condition for Navigation.dismissModal() in SidebarLinks and ReportAttachment
See @ilkeruyanik proposal
bernhardoj
commented
1 year ago @shubham1206agra got it, I will test it and let you know the result!
bernhardoj
commented
1 year ago @shubham1206agra sorry, but I have no idea to test the react-native-web fix. I tried to update the code directly from node_modules with no success. I read @ilkeruyanik proposal and I can confirm double Navigation.dismissModal is causing the issue, but I still don't know why double Navigation.dismissModal reopens the attachment carousel. I think it's better to focus on that issue.
shubham1206agra
commented
1 year ago I have already asked the team which is bumping the version of RNWeb. Lets test again after the version bump
abdulrahuman5196
commented
1 year ago Rechecking again
abdulrahuman5196
commented
1 year ago Thank you @bernhardoj for
I read @ilkeruyanik proposal and I can confirm double Navigation.dismissModal is causing the issue, but I still don't know why double Navigation.dismissModal reopens the attachment carousel. I think it's better to focus on that issue.
I agree on the same.
abdulrahuman5196
commented
1 year ago @ilkeruyanik and @tsa321 both proposals mentions the issue happens on Navigation.dismissModal() being called twice and providing solutions to make sure only one call is made. But the root cause would need more information on why Navigation.dismissModal() causes the attachment viewer to show the next image and close. Without knowing this, if we only put a solution to make sure only one call is made, we would be putting a workaround solution.
So I would kindly request the contributors to recheck the root cause and add more information on the same.
tsa321
commented
1 year ago @abdulrahuman5196
I am doing test by posting using 5 attachment images or more, and increase the timing of animationOutTiminig of BaseModal into 5000 :
The flow if second esc key is pressed: Navigation.dismissModal of sidebarlink executed -> navigate to report page -> focus on composer executed in this file:
focus event handled by react native modal ModalFocusTrap (because the modal is not completely hidden yet, so it is still exist):
The function of this file based on the comment is to trap focus of modal child components so it won't leave the modal.
Continue: composer focused -> focus event triggered -> handled by trapFocus (which will try to focus child or last descendant element of the modal -> looping through descendant -> until last element of flat list reached -> try to to focus the element of the attachment flat list -> then will cause onViewableItemsChanged will be fired because of the focus of the trapFocus attempt and make viewable image change to last redered (initialNumToRender is 3 so the third attachment)-> will execute the handler in:
Then navigate back to the modal attachment.
If animationOutTiminig value is increased with many attachments then pressing esc when the image almost disappear, the result will be something like this:
So I think we should not change focus (in this case by navigate of dismiss modal in the siderbar link then to the report screen then focus on composer box) when the modal is still available, we must wait for the modal to completely hidden (animation is finished) and we can navigate to report screen.
and another note: when user press esc for the first time, the Navigation.dismissModal is not immediately executed, because it is set onModalHide:
It will be executed when modal hide animation is finished.
So the Navigation.dismissModal of sidebarlink is the first to be executed. If we pass on dismissModal of sidebarlink while the hide animation is running, the result will be janky hide animation (can be confirmed by increasing the animationOutTiminig). The animation will immediately stop.
And another bug: If we press back button in attachment preview, the page kind of stuck and won't back.
This can be fixed by setting the parameter of focus to true in here: https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js#L481
The parameter true is to wait for ComposerFocusManageer.isReadyToFocus() :
https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/libs/focusWithDelay.ts#L25
then focus the textinput.
and set ComposerFocusManager.resetReadyToFocus(); in isvisible clause:
ilkeruyanik
commented
1 year ago @abdulrahuman5196
I investigated further this issue, and I found;
When user press ESC two times, the SidebarLinks component's event listener trigger Navigation.dismissModal() as I mentioned in proposal.
However, at that moment, the modal doesn't actually disappear. It cause trigger onViewableItemsChanged in AttachmentCarousel.
Because we've set the Navigate.route in the onCarouselAttachmentChange prop in the ReportAttachments component, the AttachmentCarousel component's onViewableItemsChanged event also triggers the Navigate.route
Note that in this case viewableItems parameters are changed, up to rendered items. You can test it with set props initialNumToRender={1} windowSize={1} maxToRenderPerBatch={1} on FlatList in AttachmentCarousel. It explains why this bug is not happen when carousel has only one image. Because it has only one rendered element.
On the other hand, onModalHide is triggered in AttachmentModal component by onBackButtonPress props in the BaseModal component. It call Navigation.dismissModal() second time and return report page.
In summary, the root cause is Sidebarlinks components event listener trigger Navigation.dismissModal() while modal is not disappear. It cause triggers onViewableItemsChanged. It call onCarouselAttachmentChange, finally calls navigate.route.
When we do implication of my proposal, event listener in Sidebarlinks will not call Navigation.dismissModal(). It will solve this issue.
pradeepmdk
commented
1 year ago when pressing esc key double time attachement preview glitches
https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/pages/home/sidebar/SidebarLinks.js#L81
when pressing esc key SidebarLinks will called first and react native modal onBackButtonPress in baseModal is called second.
so at the time willAlertModalBecomeVisible set false so second time pressing esc key SidebarLinks Navigation.dismissModal(); will triggered.
but in the mean time in AttachmentModal -> AttachmentCarousel we are using flatlist on that we are using onViewableItemsChanged it will triggered based on the viewabilityConfig
whenever onViewableItemsChanged we are navigating REPORT_ATTACHMENTS again we are navigating.
so when pressing double esc before completing the modal close and open it again with changed item.
in basemodal when onModalHide={hideModal} called only modal will removed completely.
https://www.npmjs.com/package/react-native-modal
so that glitches happened.
we to prevent the onViewableItemsChanged
options 1)
https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/components/Attachments/AttachmentCarousel/index.js#L24
here we need to add
waitForInteraction: true,
https://reactnative.dev/docs/flatlist#viewabilityconfig
https://reactnative.dev/docs/flatlist#waitforinteraction
options2)
{isModalOpen && <View style={styles.imageModalImageCenterContainer}>
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
multiple times, observe that preview closes, reopens with different image and closesExpected Result:
App should close preview on ESC click (note you need to press Esc multiple times)
Actual Result:
App glitches on closing preview on ESC click
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.61-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/93399543/1c6168d6-ff13-4892-892f-4a98015c026a
https://github.com/Expensify/App/assets/93399543/9e5b35a2-db33-474a-957e-6fe952084b2e
Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693558112570809
View all open jobs on GitHub
Upwork Automation - Do Not Edit