Open lanitochka17 opened 2 weeks ago
Triggered auto assignment to @jliexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Job added to Upwork: https://www.upwork.com/jobs/~013929a859819a7038
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External
)
Can repro, adding External
and probably fits into VSB
best?
Browser back button open other images when user opens first attachment image in attachment view (and won't close attachment view) and press the back button.
In here:
When back button is pressed, the beforeRemove
listener is called immediately even though the modal is not completely hidden yet. The code above will set modal visibility to false too early and will make:
shouldAutoFocus
to be true. This will make RNMarkdownTextInput:
autofocus to be true. This will make composer immediately focused even though the modal is not completelly hidden(because there is some animation). The trap focuse in react native modal will detect if focus is outside modal and will make focus back to element inside the modal, and here will eventually make the image changes to other/ next images. More context is in here:
https://github.com/Expensify/App/issues/27237#issuecomment-1751632831
So basically we prevent set modal visibility too early, in here we should add a condition:
the code could be:
if (Modal.areAllModalsHidden()) {
Modal.setModalVisibility(false);
Modal.willAlertModalBecomeVisible(false);
}
This will check whether all modals is hidden completely.
Then the set of setModalVisibility
and willAlertModalBecomeVisible
are already exist in BaseModal - hideModal:
The modal already use it and so the if check above is sufficient to fix this issue.
To solve the closed issue that reappears above, in baseModal, we should call hide modal only in here:
so we add hideModal inside handleDismissModal
then remove onModalHide and onDismiss property of ReactNativeModal. This is because many types of modal (whether the modal support onModalHide or not) used in the app and put the hideModal call in here will make sure the hideModal is executed once.
When an image attachment is opened inside a chat, pressing the browser back button will result in another image displaying, rather than being navigated back to the chat.
Inside the AttachmentCarousel component, the updatePage function is unexpectedly being called when the browser back button is pressed (due to how onViewableItemsChanged
works on FlatList). This causes onNavigate to be called with the first attachment in the viewable items, which then navigates the user to a different attachment.
We should detect when the back button is pressed (potentially using a ref) and then return early inside the updatePage function. This will ensure that we're not unnecessarily fetching a different image and navigating the user to it. It will allow the user to go back to the chat
Compose box - Browser's back arrow do not focus composer if first image preview open
This issue is reproducible for video attachments as well.
In FlatList
component of AttachmentCarousel
, onViewableItemsChanged
is called when pressing browser back button without user interection.
We should add waitForInteraction: true
in viewabilityConfig
π£ @CleverWolf1220! π£ 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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
@jliexpensify, I won't be able to review this soon. Please reassign.
I would love to take over this issue as C+
π£ @DylanDylann π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
All yours @DylanDylann!
EDIT: Still considering proposals!
@tsa321 Could you detail this point? Which logic does that?
The trap focuse in react native modal will detect if focus is outside modal and will make focus back to element inside the modal, and here will eventually make the image changes to other/ next images.
@arjun-dureja Could you deep dive into this point and explain why the updatePage is called? Is there any reference, document that explain why onViewableItemsChanged is triggered?
due to how onViewableItemsChanged works on FlatList
cc @CleverWolf1220
@CleverWolf1220 Why waitForInteraction will resolve this problem?
@DylanDylann Focus event catch by react native modal ModalFocusTrap
is in here:
The function of this file based on the comment is to trap focus so the focus won't leave modal, if the focus is outside of modal, the trap focus will focus last element inside modal.
The code flow is: Composer focused (from autofocus of RNMarkdownTextInput)-> focus event triggered -> catch by trapFocus (which will try to focus child or last descendant element of the modal -> trap focus looping through descendant element -> until last element of the initial rendered image (initialNumToRender is 3 in AttachmentCarousel) list reached :
The trapFocus try to focus the element of the attachment list will cause onViewableItemsChanged
will be executed because the focus will make the displayed image slide(changes), hence will execute handler in:
which is navigate back to the attachment modal.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@jliexpensify, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!
Hi Melv, thanks for tagging me. I will complete my review today
@DylanDylann
@CleverWolf1220 Why waitForInteraction will resolve this problem?
Sorry for late response. I found that my proposal solves this issue but causes other problems.
@tsa321's proposal looks good to me. Let's go with them
π π π C+ reviewed
Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Ooh, that's complicated. I think this solution makes sense to me, but we should make sure to do a lot of testing on closing other modals, etc. to make sure this doesn't cause any regressions there.
Assigining @tsa321
π£ @tsa321 π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.74-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
Precondition: upload multiple images in chat
Expected Result:
Composer gets focused
Actual Result:
Last uploaded image preview opened
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/fb43f2cf-5fee-4545-8f25-4066cce2a725
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @thesahindia