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

[HOLD App#11795] [$500] Pressing enter takes the focus to Add a personal message field on Android - reported by @thesahindia #10181

Closed
mvtglobally opened this issue Aug 2, 2022 · 99 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Design Engineering 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 Improvement Item broken or needs improvement. Monthly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Aug 2, 2022

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:

  1. Open :expensify: at mobile browser on Android
  2. Go to settings > workspace > manage members > invite
  3. Press enter on keyboard

Alternative Flow

  1. Open :expensify: at mobile browser
  2. Go to settings > workspace > manage members > invite
  3. Tap on Add a personal message field and press enter

Expected Result:

  • Pressing enter should not change the focus to the personal message field
  • Pressing enter a first time should select the first contact
  • Pressing enter another time should select the next contact in the list
Screen.Recording.2022-08-08.at.11.44.43.AM.mov

Actual Result:

On pressing enter the Add a personal message field gets focused

The user can't go to the next line and pressing enter selects the contacts

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Mobile Web (Android)
  • Android

Make sure this works same on iOS too and matches desktop/browser behaviour.

Version Number: 1.1.85-8
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screenrecording_20220710_012126.mp4

potentially related to https://expensify.slack.com/archives/C01GTK53T8Q/p1657397089778049

Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1657396762843809

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 2, 2022
@MitchExpensify
Copy link
Contributor

I am unable to reproduce this on iOS

@thesahindia
Copy link
Member

I am unable to reproduce this on iOS

Only reproducible at android mobile browsers

@MitchExpensify
Copy link
Contributor

Asked in Slack for help with reproduction - https://expensify.slack.com/archives/C01GTK53T8Q/p1659456631390199?thread_ts=1657396762.843809&cid=C01GTK53T8Q

@MitchExpensify
Copy link
Contributor

@rushatgabhane reproduced on v1.1.86-5, Chrome, Android 12

This is odd behavior, labelling Engineering accordingly

@MitchExpensify MitchExpensify added Engineering Improvement Item broken or needs improvement. labels Aug 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

Triggered auto assignment to @Justicea83 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Justicea83
Copy link
Contributor

this can be external

@Justicea83 Justicea83 added the External Added to denote the issue can be worked on by a contributor label Aug 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

@Justicea83 Justicea83 removed their assignment Aug 2, 2022
@MitchExpensify
Copy link
Contributor

Upwork Job

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

Triggered auto assignment to @aldo-expensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Pressing enter at mWeb takes the focus to Add a personal message field - reported by @thesahindia [$250] Pressing enter at mWeb takes the focus to Add a personal message field - reported by @thesahindia Aug 3, 2022
@railway17
Copy link
Contributor

railway17 commented Aug 4, 2022

Proposal

I could replicate the issue in only real devices, not simulators.

Actually, this is an exceptional case in Android devices, which we can reproduce even though we test with 2 input boxes in an empty page.
So, we can resolve this issue by adding returnKeyType props in the OptionsSelector like this.

const textInput = (
            <TextInput
                ref={el => this.textInput = el}
                value={this.props.value}
                label={this.props.textInputLabel}
                onChangeText={(text) => {
                    if (this.props.shouldFocusOnSelectRow) {
                        this.textInput.setNativeProps({selection: null});
                    }
                    this.props.onChangeText(text);
                }}
                placeholder={this.props.placeholderText || this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
                onBlur={(e) => {
                    if (!this.props.shouldFocusOnSelectRow) {
                        return;
                    }
                    this.relatedTarget = e.relatedTarget;
                }}
                selectTextOnFocus
+               returnKeyType="done"
                blurOnSubmit={Boolean(this.state.allOptions.length)}
            />

It will resolve this issue.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 4, 2022
@parasharrajat
Copy link
Member

parasharrajat commented Oct 9, 2022

@railway17's proposal can be improved. it is missing proper explanation and not easy to understand. (Please try to make it easier for a reviewer to understand your proposal). The easier it is to understand, the better odds for you.

Could you please add the following details to it?

  1. What behavior are you proposing?
  2. Explain how you will achieve it in steps and then use code to support your explanation.
  3. There are different scenarios involved on that page. Explain, how is your proposal supporting those?
    for example, if the first option in the list is already selected, what will happen when pressing enter? etc.

What do you mean by selected from ahead one by one?

@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Oct 10, 2022
@mountiny
Copy link
Contributor

@parasharrajat @shawnborton @MitchExpensify @aldo-expensify I am unsure if we should just proceed with this one on its own. Seems like the solution to it is quite involved and we have other similar forms/lists in the App which should have the same behaviour and I dont think we should only focus on this one solely.

The solution/behaviour should be same for all instances of this input form. At the same time, this feels like low ROI at the moment with out attention to better performance of the app.

I think it would be better to put this on HOLD and have some predesign about how this input form should behave across the platforms, clearly define all the scenarios and how they should be handled (similarly as we are doing with the navigation now), otherwise we will have many of similar issues like this where we will be trying to tweak the behaviour slightly depending on who the CME is 😄 Thoughts?

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2022

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@shawnborton
Copy link
Contributor

I like the idea of putting this on HOLD while we design a better solution.

@MitchExpensify
Copy link
Contributor

Would leading that pre-design typically fall on the CM or CME? This is the first time I've run into a case like this as CM

@shawnborton
Copy link
Contributor

I think we'd want to put this back on our internal design team to come up with a better solution, and probably just close this one out altogether?

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Oct 12, 2022

I think we'd want to put this back on our internal design team to come up with a better solution, and probably just close this one out altogether?

Sounds good to me! and totally agree with what @mountiny said here: #10181 (comment)

@MitchExpensify
Copy link
Contributor

I think we'd want to put this back on our internal design team to come up with a better solution, and probably just close this one out altogether?

Cool! I'm tempted to pay out half price ($250) to C+ and the reporter of the issue for their work on this. What do you reckon @michaelhaxhiu ? (CM buddy check)

@mountiny
Copy link
Contributor

Cool! I'm tempted to pay out half price ($250) to C+ and the reporter of the issue for their work on this. What do you reckon @michaelhaxhiu ? (CM buddy check)

I agree with paying that out and I dont think either of us necessarily needs to lead the predesign we should just make sure this does not fall through the cracks and it gets done!

@mountiny
Copy link
Contributor

Created a planning issue for this here #11795 so we dont forget about it. Feel free to subscribe to it or take it on if you want to lead this sooner than later!

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Oct 13, 2022

@MitchExpensify Let's take the pricing question to the internal #contributor-plus channel and discuss there. It would be good to keep track of precedent here in case similar situations arise in the future.

@MitchExpensify
Copy link
Contributor

@MitchExpensify Let's take the pricing question to the internal #contributor-plus channel and discuss there. It would be good to keep track of precedent here in case similar situations arise in the future.

Asked here

@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2022
@mountiny mountiny added Monthly KSv2 and removed Weekly KSv2 labels Oct 24, 2022
@melvin-bot melvin-bot bot removed the Overdue label Oct 24, 2022
@mountiny mountiny changed the title [$500] Pressing enter takes the focus to Add a personal message field on Android - reported by @thesahindia [HOLD] [$500] Pressing enter takes the focus to Add a personal message field on Android - reported by @thesahindia Oct 24, 2022
@mountiny mountiny changed the title [HOLD] [$500] Pressing enter takes the focus to Add a personal message field on Android - reported by @thesahindia [HOLD App#11795] [$500] Pressing enter takes the focus to Add a personal message field on Android - reported by @thesahindia Oct 24, 2022
@mountiny
Copy link
Contributor

Put this on hold as mentioned before

@tjferriss
Copy link
Contributor

Following instructions for the weekly update chore:

@mountiny this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is
  • For any that can't, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #bug-zero

I recognize this issue is on hold so maybe the above is irrelevant, but once it's not on hold do you expect we should take this internal given the issue's age?

@mountiny
Copy link
Contributor

Thanks for the ping, in this particular case I will go ahead and close the issue. My reasons are:

  • this is not strictly bug, but it can be considered as feature request.
  • this is not blocking users from using the app
  • this influences only Android
  • we will fix this and similar issues related to option list selection in this issue [Tracker] Selection List Refactor #11795

@parasharrajat
Copy link
Member

@MitchExpensify Can you please look into the C+ payment #10181 (comment)

@MitchExpensify
Copy link
Contributor

Hey @parasharrajat, apologies for the delay (Thanksgiving break then I was unwell)

Invite sent to new upwork job

@parasharrajat
Copy link
Member

@MitchExpensify Could you please pay this off?

@MitchExpensify
Copy link
Contributor

@parasharrajat Done! Thanks for the bump

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. Design Engineering 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 Improvement Item broken or needs improvement. Monthly KSv2
Projects
None yet
Development

No branches or pull requests