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.

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

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

Open
1 of 6 tasks
m-natarajan opened this issue Jun 13, 2024 · 37 comments
Open
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jun 13, 2024

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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
image (8)

Recording.211.mp4

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
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

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
Copy link
Contributor

tsa321 commented Jun 13, 2024

Cannot reproduce in production release.
offending PR: #39520

@tsa321
Copy link
Contributor

tsa321 commented Jun 14, 2024

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 #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:

<FocusTrapForModal active={isVisible}>

Another alternative solution is to include the attachment screen in:

include the coresponding attachment modal screen in SCREENS_WITH_AUTOFOCUS.

const SCREENS_WITH_AUTOFOCUS: string[] = [

@melvin-bot melvin-bot bot added the Overdue label Jun 17, 2024
@CortneyOfstad CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Jun 17, 2024
@melvin-bot melvin-bot bot changed the title Keyboard focus is on the "close" button when uploading an attachment [$250] Keyboard focus is on the "close" button when uploading an attachment Jun 17, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 17, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@CortneyOfstad
Copy link
Contributor

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!

Copy link

melvin-bot bot commented Jun 17, 2024

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

@CortneyOfstad
Copy link
Contributor

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

Copy link

melvin-bot bot commented Jun 21, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jun 21, 2024
@rojiphil
Copy link
Contributor

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.

@melvin-bot melvin-bot bot removed the Overdue label Jun 21, 2024
@tsa321
Copy link
Contributor

tsa321 commented Jun 21, 2024

@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.

macos-web-d.mp4

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
Copy link
Contributor

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
Copy link
Contributor

rojiphil commented Jun 24, 2024

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

@CortneyOfstad
Copy link
Contributor

Sounds good — thanks @rojiphil! I was not able to reproduce again either!

@mallenexpensify
Copy link
Contributor

This is still happening on staging and production.

  • open chat
  • paste in an image
  • tap enter/return.

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

Copy link

melvin-bot bot commented Jun 29, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Jul 1, 2024

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

@tsa321
Copy link
Contributor

tsa321 commented Jul 3, 2024

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

@mallenexpensify
Copy link
Contributor

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

@rojiphil
Copy link
Contributor

rojiphil commented Jul 4, 2024

We introduce focusTrap in #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 melvin-bot bot removed the Overdue label Jul 4, 2024
Copy link

melvin-bot bot commented Jul 6, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Jul 6, 2024
@mallenexpensify mallenexpensify changed the title [$250] Keyboard focus is on the "close" button when uploading an attachment [$500] Keyboard focus is on the "close" button when uploading an attachment Jul 8, 2024
Copy link

melvin-bot bot commented Jul 8, 2024

Upwork job price has been updated to $500

@suneox
Copy link
Contributor

suneox commented Jul 9, 2024

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

Screen.Recording.2024-07-09.at.10.19.14.mov

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
Copy link
Contributor

rojiphil commented Jul 9, 2024

@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 melvin-bot bot removed the Overdue label Jul 9, 2024
Copy link

melvin-bot bot commented Jul 9, 2024

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

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@suneox
Copy link
Contributor

suneox commented Jul 11, 2024

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

Screen.Recording.2024-07-11.at.12.05.09.mov

@melvin-bot melvin-bot bot added the Overdue label Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@mallenexpensify
Copy link
Contributor

@grgia 👀 on the proposal above plz

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 12, 2024
Copy link

melvin-bot bot commented Jul 12, 2024

📣 @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

Copy link

melvin-bot bot commented Jul 12, 2024

📣 @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 📖

@melvin-bot melvin-bot bot removed the Overdue label Jul 12, 2024
@mallenexpensify
Copy link
Contributor

@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
Copy link
Contributor

suneox commented Jul 13, 2024

I'll create PR in few hours

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 13, 2024
@suneox
Copy link
Contributor

suneox commented Jul 13, 2024

Hi @rojiphil the PR is ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

8 participants