Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.14k stars 2.63k forks source link

[$500] Keyboard focus is on the "close" button when uploading an attachment #43726

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

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: v1.4.83-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1718296177557209

Action Performed:

  1. Go to staging.new.expensify.com
  2. Open any report and add attachment
  3. Press "enter" key on keyboard

    Expected Result:

    Keyboard focus on "Send" button and image sent

    Actual Result:

    Keyboard focus on cancel button and pressing "enter" on keyboard cancels the upload

    Workaround:

    Unknown

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [X] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence image (8)

https://github.com/Expensify/App/assets/38435837/3f301e26-1397-4d01-b8fc-bc05f255478f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ab436ac7382c9793
  • Upwork Job ID: 1802770011901307524
  • Last Price Increase: 2024-07-08
  • Automatic offers:
    • rojiphil | Reviewer | 103095843
    • suneox | Contributor | 103095845
Issue OwnerCurrent Issue Owner: @grgia
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @CortneyOfstad (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.

tsa321 commented 1 month ago

Cannot reproduce in production release. offending PR: https://github.com/Expensify/App/pull/39520

tsa321 commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Keyboard focus is on the "close" button when uploading an attachment

What is the root cause of that problem?

We introduce focusTrap in https://github.com/Expensify/App/pull/39520 But in react-native-modal we already have focusTrap:

https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/Modal/ModalFocusTrap.js

What changes do you think we should make in order to solve the problem?

We can remove the new focusTrap and use react native web focusTrap in baseModal which is already there. So we only need to remove FocusTrapForModal:

https://github.com/Expensify/App/blob/e306af7b363b499384fdad74e4049e05ddaf17fb/src/components/Modal/BaseModal.tsx#L256

Another alternative solution is to include the attachment screen in:

include the coresponding attachment modal screen in SCREENS_WITH_AUTOFOCUS. https://github.com/Expensify/App/blob/2a641d98760dfdc1a2ebe9b958787c9cfacb02c2/src/components/FocusTrap/SCREENS_WITH_AUTOFOCUS.ts#L4

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01ab436ac7382c9793

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External)

CortneyOfstad commented 1 month ago

I was able to recreate so going to get some eyes on this. @rojiphil we have a proposal here for review when you have a chance!

melvin-bot[bot] commented 1 month ago

@CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

CortneyOfstad commented 4 weeks ago

@rojiphil can you provide feedback on the proposal here by EOD please? Thanks!

melvin-bot[bot] commented 3 weeks ago

@rojiphil, @CortneyOfstad Whoops! This issue is 2 days overdue. Let's get this updated quick!

rojiphil commented 3 weeks ago

Based on the OP test steps, I am unable to launch the Attachment modal on press of Enter key. Maybe I am missing something here. @tsa321 Are you able to reproduce the problem on the latest main? If so, please share a video as well.

tsa321 commented 3 weeks ago

@rojiphil I cannot reproduce it anymore, but I found another bug because of the new focusTrap in baseModal:

Send an attachment -> edit a report action (keep the focus in compose edit) -> open the attachment -> press back browser back button. Notice the edit composer isn't highlighted.

https://github.com/Expensify/App/assets/114975543/cd59e94f-8c5a-4d1c-804f-49dd51a65be6

Maybe there are another bugs too. So my suggestion is, since baseModal (react-native-modal) already has focusTrap in it:

https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/Modal/ModalFocusTrap.js

We could remove the new focusTrap in baseModal.

rojiphil commented 3 weeks ago

I cannot reproduce it anymore, but I found another bug because of the new focusTrap in baseModal:

@tsa321 As the raised issue is completely different, it is best to raise it separately and go through the normal assignment process. You can raise this in the expensify-open-source channel.

rojiphil commented 3 weeks ago

@CortneyOfstad I think we can close this issue as it is not reproduceable

CortneyOfstad commented 3 weeks ago

Sounds good β€” thanks @rojiphil! I was not able to reproduce again either!

mallenexpensify commented 2 weeks ago

This is still happening on staging and production.

Expected behavior - image uploads

Actual behaviour - nothing happens, you have to navigate to the send button and click.

@CortneyOfstad , if you're unable to reproduce, please swap assignments with me. Thx

melvin-bot[bot] commented 2 weeks 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 2 weeks ago

@rojiphil, @CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 2 weeks ago

@tsa321 @rojiphil can you both try to reproduce again? If you're not able to reproduce, can you share your steps? I'm able to consistently. Thx

CortneyOfstad commented 2 weeks ago

@tsa321 @rojiphil bump on the above ^^^ thank you!

melvin-bot[bot] commented 2 weeks ago

@rojiphil, @CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

tsa321 commented 2 weeks ago

@mallenexpensify @CortneyOfstad yes, I can reproduce the issue mentioned here

mallenexpensify commented 2 weeks ago

@rojiphil , do you think @tsa321 's proposal above will fix the bug?

rojiphil commented 2 weeks ago

We introduce focusTrap in https://github.com/Expensify/App/pull/39520

We can remove the new focusTrap and use react native web focusTrap in baseModal which is already there. So we only need to remove FocusTrapForModal

@tsa321 FocusTrapForModal was introduced intentionally in the mentioned PR. So, removal is not an option. And your alternative solution is not clear to me. Can you provide more details of the code changes if you think that would work? Or if you have any other ideas, please share.

melvin-bot[bot] commented 1 week 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 week ago

Upwork job price has been updated to $500

suneox commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Trigger focus to modal element when the modal is opened

What is the root cause of that problem?

We have disabled initialFocus at FocusTrapForModal so when paste file to modal the focus element still on the input

https://github.com/Expensify/App/assets/11959869/7aa358ed-b18e-4b02-9564-5a6a40610ea9

What changes do you think we should make in order to solve the problem?

While opening the modal, we should trigger focus to expected element in that case at AttachmentModal we can update hook useAutoFocusInput allow button then apply to trigger focus send button

+   const { focusRef: submitRef } = useAutoFocusInput();

    return (
        <>
            <Modal
                type={modalType}
                ....
                        <SafeAreaConsumer>
                            {({safeAreaPaddingBottomStyle}) => (
                                <Animated.View style={[StyleUtils.fade(confirmButtonFadeAnimation), safeAreaPaddingBottomStyle]}>
                                    <Button
+                                       ref={submitRef}
                                        success
                                        large

or only focus the modal container due to submit button has listen event pressOnEnter

What alternative solutions did you explore? (Optional)

We can allow pass props initialFocus into FocusTrapForModal from AttachmentModal we will get submit button ref then pass it into initialFocus

rojiphil commented 1 week ago

@suneox Thanks for your proposal. Your alternative solution LGTM i.e setting initialFocus with the element that needs to be focused. This way we are also extending the feature to bring the initial focus to a custom element. πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

mvtglobally commented 1 week ago

Issue not reproducible during KI retests. (First week)

suneox commented 1 week ago

Issue not reproducible during KI retests. (First week)

@mvtglobally This issue still happen on latest staging & production, can not send by "enter" key and text input bellow modal still focus after paste image

https://github.com/Expensify/App/assets/11959869/2ed2baa9-12ee-4da1-a16a-4b15595b29e7

melvin-bot[bot] commented 1 week ago

@rojiphil @CortneyOfstad @grgia this issue is now 4 weeks old, please consider:

Thanks!

mallenexpensify commented 1 week ago

@grgia πŸ‘€ on the proposal above plz

melvin-bot[bot] commented 6 days ago

πŸ“£ @rojiphil πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 6 days ago

πŸ“£ @suneox πŸŽ‰ 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 πŸ“–

mallenexpensify commented 6 days ago

@rojiphil @suneox , if possible, let's try to expedite this fix. It's a smaller bug but it's affecting quite a few folks. Thx

suneox commented 6 days ago

I'll create PR in few hours

suneox commented 6 days ago

Hi @rojiphil the PR is ready for review