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] Android - Rooms can be created when the user presses the Return key while typing in the description #36624

Closed
1 of 6 tasks
kbecciv opened this issue Feb 15, 2024 · 35 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Feb 15, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.42.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: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Launch the app
  2. Log in with your account
  3. Tap on Plus green button
  4. Select Start Chat - Room
  5. Enter room name
  6. On description, use any text and hit Return key on Samsung device

Expected Result:

Room should not be created

Actual Result:

Rooms can be created when the user presses the Return key while typing in the description

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

Screen_Recording_20240215_131957_New.Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fe10c08e956825df
  • Upwork Job ID: 1758204974898151424
  • Last Price Increase: 2024-03-14
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 15, 2024
@melvin-bot melvin-bot bot changed the title Android - Rooms can be created when the user presses the Return key while typing in the description [$500] Android - Rooms can be created when the user presses the Return key while typing in the description Feb 15, 2024
Copy link

melvin-bot bot commented Feb 15, 2024

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

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

melvin-bot bot commented Feb 15, 2024

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

Copy link

melvin-bot bot commented Feb 15, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Feb 15, 2024

We think that this bug might be related to #vip-vsb
CC @quinthar

@neonbhai
Copy link
Contributor

neonbhai commented Feb 15, 2024

Proposal

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

Rooms can be created when the user presses the Return key while typing in the description

What is the root cause of that problem?

We don't disable pressing the submit button on enter here

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

We should change disable press on enter to true

Alternatively

We can remove the prop

@eucool
Copy link
Contributor

eucool commented Feb 15, 2024

Proposal

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

Rooms can be created when the user presses the Return key while typing in the description

What is the root cause of that problem?

We do not define the type of functionality for enter button for the description InputWrapper:

<InputWrapper
InputComponent={TextInput}
inputID={INPUT_IDS.REPORT_DESCRIPTION}
label={translate('reportDescriptionPage.roomDescriptionOptional')}
accessibilityLabel={translate('reportDescriptionPage.roomDescriptionOptional')}
role={CONST.ACCESSIBILITY_ROLE.TEXT}
autoGrowHeight
maxLength={CONST.REPORT_DESCRIPTION.MAX_LENGTH}
autoCapitalize="none"
containerStyles={[styles.autoGrowHeightMultilineInput]}
/>

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

pass a default prop (React documentation) returnKeyType and set it to done:

<InputWrapper 
     InputComponent={TextInput} 
     inputID={INPUT_IDS.REPORT_DESCRIPTION} 
     returnKeyType='done'

What alternative solutions did you explore? (Optional)

N/A

@Krishna2323
Copy link
Contributor

Proposal

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

Rooms can be created when the user presses the Return key while typing in the description

What is the root cause of that problem?

The issue seems device specific, not reproducible on emulator, but I believe it can be solved if we pass the props below conditionally.

disablePressOnEnter={false}

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

We should pass false if DeviceCapabilities.canUseTouchScreen() && isMediumScreenWidth is true.

disablePressOnEnter={DeviceCapabilities.canUseTouchScreen() && isMediumScreenWidth}

Additionally we can also check for platform !== 'android'.

Result

@agent3bood
Copy link
Contributor

agent3bood commented Feb 18, 2024

Proposal

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

Android - Rooms can be created when the user presses the Return key while typing in the description

What is the root cause of that problem?

The component tha listens for Enter keyboard shortcut is not able to detect the description field as multiline.
Look here
while the web implementation for validateSubmitShortcut is skipping this event when `eventTarget.nodeName === 'TEXTAREA'
the native implementation does not have any information of the event or what type of element is focused.

To know which input is focus we can use FormContext to store that info.

<InputWrapper
  InputComponent={TextInput}
  inputID={INPUT_IDS.REPORT_DESCRIPTION}
  onTouched={} // here
  onBlur={} // here

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

Update KeyboardShortcutComponent and add new arg isMultiline
and use that in the validateSubmitShortcut function.

Because the keyboardShortcutComponent is only used locally this change will not break other parts of the app, and it will be the expected behaviour to not submit the form when the user presses Enter while typing in a multiline field.

What alternative solutions did you explore? (Optional)

  • Check if bindHandlerToKeydownEvent emit any useful information when the enter key is pressed on native, the event was just undefined.
  • Tyr to get the focused node and figure out if it is a multiline field, but native do not support focus.

@melvin-bot melvin-bot bot added the Overdue label Feb 18, 2024
Copy link

melvin-bot bot commented Feb 19, 2024

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

@mananjadhav
Copy link
Collaborator

mananjadhav commented Feb 19, 2024

@kbecciv I can see only Android native is marked as the platform. Can you please confirm about other platforms? If not, then none of the proposals highlight why it is only reproducible in Android.

@melvin-bot melvin-bot bot removed the Overdue label Feb 19, 2024
@eucool
Copy link
Contributor

eucool commented Feb 20, 2024

I can see only Android native is marked as the platform. Can you please confirm about other platforms? If not, then none of the proposals highlight why it is only reproducible in Android.

@mananjadhav If we do not pass the prop returnKeyType, then

For Android, the default value of returnKeyType is "done".
For iOS, the default value of returnKeyType is "default".

And hence on iOS we don't see the submit behavior but we see it submits on android devices

@kbecciv
Copy link
Author

kbecciv commented Feb 22, 2024

@mananjadhav Let me double check with reproducible platforms.

@melvin-bot melvin-bot bot added the Overdue label Feb 22, 2024
Copy link

melvin-bot bot commented Feb 22, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Feb 22, 2024

@mananjadhav Issue is reproducible only on Android app

Copy link

melvin-bot bot commented Feb 23, 2024

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

@eucool
Copy link
Contributor

eucool commented Feb 26, 2024

bump @mananjadhav for review :)

Copy link

melvin-bot bot commented Feb 27, 2024

@mananjadhav, @kevinksullivan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@mananjadhav
Copy link
Collaborator

Will review this by tomorrow.

@mananjadhav
Copy link
Collaborator

I have asked for a clarifying question here. Overall I am inclined to use disablePressOnEnter.

@mananjadhav
Copy link
Collaborator

@kevinksullivan I am a little lost here on the expected behavior. Based on @ishpaul777's comment it was done intentionally? Are we looking to rollback the change or only fix for Android?

Copy link

melvin-bot bot commented Feb 29, 2024

@mananjadhav @kevinksullivan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Feb 29, 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 Mar 4, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

Copy link

melvin-bot bot commented Mar 5, 2024

@mananjadhav, @kevinksullivan Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Mar 7, 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 Mar 7, 2024

@mananjadhav @kevinksullivan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Copy link

melvin-bot bot commented Mar 7, 2024

@mananjadhav, @kevinksullivan Still overdue 6 days?! Let's take care of this!

@mananjadhav
Copy link
Collaborator

@kevinksullivan I am a little lost here on the expected behavior. Based on @ishpaul777's comment it was done intentionally? Are we looking to rollback the change or only fix for Android?

@kevinksullivan Can you please confirm the expected behavior?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 10, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

Copy link

melvin-bot bot commented Mar 14, 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 Mar 14, 2024

@mananjadhav @kevinksullivan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, 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 #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed 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 labels Mar 14, 2024
Copy link

melvin-bot bot commented Mar 14, 2024

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Mar 15, 2024

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

@kevinksullivan
Copy link
Contributor

Huh, closing out if this was by design. @kbecciv was there a particular test you ran where it suggested this was a bug?

@kbecciv
Copy link
Author

kbecciv commented Mar 19, 2024

@kevinksullivan No particular test, exploratory issue!

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 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

7 participants