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

[$250] Can't go back in "Contact methods" if triggering "Back" button using Enter #29132

Closed
2 of 6 tasks
m-natarajan opened this issue Oct 9, 2023 · 12 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@m-natarajan
Copy link

m-natarajan commented Oct 9, 2023

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.3.79-3
Reproducible in staging?: Yes
Reproducible in production?: Yes
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: @kosmydel
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696849230849869

Action Performed:

  1. Go to Settings -> Profile -> Contact method
  2. Select the "Back" button using Tab.
  3. Press Enter

Expected Result:

Should go back to the previous screen

Actual Result:

Opens the "New contact method" screen and immediately closes it.

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

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
bug1.mov
Tab.contact.mp4
MacOS: Desktop

.

Recording.2550.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014d24f78857b6fbc3
  • Upwork Job ID: 1711509906241732608
  • Last Price Increase: 2023-10-10
@m-natarajan m-natarajan added the External Added to denote the issue can be worked on by a contributor label Oct 9, 2023
@melvin-bot melvin-bot bot changed the title Can't go back in "Contact methods" if triggering "Back" button using Enter [$500] Can't go back in "Contact methods" if triggering "Back" button using Enter Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014d24f78857b6fbc3

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 9, 2023
@saranshbalyan-1234
Copy link
Contributor

Proposal

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

Can't go back in "Contact methods" if triggering "Back" button using Enter

What is the root cause of that problem?

onPress={() => Navigation.navigate(ROUTES.SETTINGS_NEW_CONTACT_METHOD)}
pressOnEnter
/>
</FixedFooter>
</ScrollView>
</ScreenWrapper>

The issue is with pressOnEnter prop here so on pressing enter, New Contact Method button is triggered.

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

As the contact methods page does not contain any input elements (like text of selection) we dont need pressOnEnter functionality here.
We can just remove the prop and it will work fine.

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot melvin-bot bot added the Daily KSv2 label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@Krishna2323
Copy link
Contributor

I believe it comes under accessibility and we closed multiple issue that are related to accessibility as we are not focusing on it right now.

cc: @thesahindia

@cead22 cead22 changed the title [$500] Can't go back in "Contact methods" if triggering "Back" button using Enter [$250] Can't go back in "Contact methods" if triggering "Back" button using Enter Oct 10, 2023
@cead22
Copy link
Contributor

cead22 commented Oct 10, 2023

This looks like a simple bug, and I imagine the solution will be simple as well, so I decreased the bounty

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

Upwork job price has been updated to $250

@Victor-Nyagudi
Copy link
Contributor

A pull request affecting the Enter key's behavior got merged and recently deployed to production.

It could provide some insight on why this issue occurred.

@parasharrajat
Copy link
Member

We shouldn't be focusing on this issue ATM. We are migrating to RN-web v0.19 which will allow us to use button components instead of simple View thus this issue won't happen. This change is being tracked on some other issue.

Also, this is something that might be facing on almost all pages of the app and we have closed these in the past. I would do the same until we are ready to focus on such issues.

@FitseTLT
Copy link
Contributor

Proposal

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

On Contact Methods page after focusing on the back button with the Tab key and pressing enter; it doesn't navigate back

What is the root cause of that problem?

Because there is the new contact methods button with pressOnEnter enabled, in addition to the navigate back code the create new contact methods' code is also executed. So that's why the page will navigate to the new contact page and it will get back to the contact methods page(instead of navigating back to the profiles page).

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

This problem must exist everywhere there is a Button component with pressOnEnter enabled. So the fix should be applied to our Button component.
Changes to be applied:
Here,

(e) => {
if (!validateSubmitShortcut(this.props.isFocused, this.props.isDisabled, this.props.isLoading, e)) {
return;
}
this.props.onPress();

we need additional logic to check if the focus is not on another element (it should be on the button element or on its parent) so it should be changed to
if(document.activeElement.contains(this.ref.current)) this.props.onPress();
POC:
https://github.com/Expensify/App/assets/38649957/2a9e55d2-01be-4cdd-b2a0-faaf5000f46d

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@m-natarajan m-natarajan added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@cead22
Copy link
Contributor

cead22 commented Oct 10, 2023

Closing per #29132 (comment)

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

9 participants