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 18601][$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box #10906

Closed
kbecciv opened this issue Sep 8, 2022 · 68 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. 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

@kbecciv
Copy link

kbecciv commented Sep 8, 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. Go to staging.new.expensify.com
  2. Log in with expensifail account
  3. Go to Setting - Workspace
  4. Manage Members
  5. Tap Invite button
  6. Go to Personal message box and type anything
  7. Use Enter Key on your keyboard to start with new line

Expected Result:

Unable to use Enter key for starting a new line under Personal message box

Actual Result:

Able to use Enter key for starting a new line under Personal message box

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android mWeb/Chrome

Version Number: 1.1.98.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5723405_Screen_Recording_20220908-135730_Chrome.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Interna Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2022

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

@melvin-bot melvin-bot bot added the Overdue label Sep 12, 2022
@neil-marcellini
Copy link
Contributor

Looks like a legitimate problem and would be a good external issue.

@melvin-bot melvin-bot bot removed the Overdue label Sep 12, 2022
@neil-marcellini neil-marcellini removed their assignment Sep 12, 2022
@neil-marcellini neil-marcellini added the External Added to denote the issue can be worked on by a contributor label Sep 12, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2022

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@mallenexpensify
Copy link
Contributor

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2022

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

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

melvin-bot bot commented Sep 12, 2022

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

@melvin-bot melvin-bot bot changed the title mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box [$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box Sep 12, 2022
@0xmiros
Copy link
Contributor

0xmiros commented Sep 13, 2022

Proposal

RCA:
preventDefault() is triggered on Enter key of TextInput because of OptionsSelector (to select members) which overrides keyboard shortcuts of Enter, Arrow up/down keys.
NOTE: On desktop, Shift+Enter can be workaround but still user doesn't know Shift key on this screen. Also, arrow up/down also doesn't move cursor properly on this multiline text input. So we should fix both Enter key and arrow up/down key behaviors on both desktop/mWeb platforms. (no issue on native iOS/Android)

Solution:
When multiline input is focused and press Enter or Arrow up/down keys, we should avoid preventDefault() call and also prevent default behavior of BaseOptionsSelector, ArrowKeyFocusManager components.
To implement this, we need to introduce new props in both components:

  • disabled: to prevent default behavior of keyboard shortcuts
  • keyboardShortcutShouldPreventDefault: not to prevent other components' keyboard behavior (i.e. enter on multiline input)
    You can check full code proposal here

This solution is when focused input should keep focused regardless of keyboard shortcuts triggers. (reference: #10705 (comment))
If focused input should be blurred whenever arrow up/down keys pressed, similar to tab/shit+tab behavior, we need to find another solution. But I don't think this is expected behavior in our app.

before.fix.mov
after.fix.mov

@mollfpr
Copy link
Contributor

mollfpr commented Sep 13, 2022

Proposal

Root cause
We have a keyboard listener for the Enter key and it's also captured on input.

this.unsubscribeEnter = KeyboardShortcut.subscribe(
enterConfig.shortcutKey,
() => {
const focusedOption = this.state.allOptions[this.state.focusedIndex];
if (!focusedOption) {
return;
}
this.selectRow(focusedOption);
},
enterConfig.descriptionKey,
enterConfig.modifiers,
true,
() => !this.state.allOptions[this.state.focusedIndex],
);

On Desktop/Web can use CMD+Enter to create a new line on input but it's not working on native/mWeb.

Solution
Activate the captureOnInputs only on non-touchscreen devices.

diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js 
b/src/components/OptionsSelector/BaseOptionsSelector.js
index cbcb0faa4..dd15a495d 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -18,6 +18,7 @@ import KeyboardShortcut from '../../libs/Keyboard
Shortcut';
 import ONYXKEYS from '../../ONYXKEYS';
 import FullScreenLoadingIndicator from '../FullscreenLoadingIndica
tor';
 import {propTypes as optionsSelectorPropTypes, defaultProps as opt
ionsSelectorDefaultProps} from './optionsSelectorPropTypes';
+import canUseTouchScreen from '../../libs/canUseTouchscreen';
 
 const propTypes = {
     /** Whether we should wait before focusing the TextInput, usef
ul when using transitions on Android */
@@ -62,7 +63,7 @@ class BaseOptionsSelector extends Component {
             },
             enterConfig.descriptionKey,
             enterConfig.modifiers,
-            true,
+            !canUseTouchScreen(),
             () => !this.state.allOptions[this.state.focusedIndex],
         );
 

Result

Screen.Recording.2022-09-13.at.15.20.24.mov
Screen.Recording.2022-09-13.at.15.19.31.mov

@Puneet-here
Copy link
Contributor

Puneet-here commented Sep 13, 2022

Proposal

It's a good opportunity to implement push to page, we should show this input on a different page individually. I have seen some other issues related to this field they can be fixed by moving it to a different field. For example one issue was that user can't see the list completely when keyboard is open because this field is so big and when keyboard opens it takes too much space.
It looks like this -
Screenshot 2022-09-13 at 3 48 27 PM

@rushatgabhane
Copy link
Member

#10906 (comment)

cc @Expensify/design

@MonilBhavsar
Copy link
Contributor

So, If I am understanding it correct, we're implementing a new feature as a part of this issue?

@melvin-bot melvin-bot bot removed the Overdue label Feb 27, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 7, 2023
@Santhosh-Sellavel
Copy link
Collaborator

cc: @JmillsExpensify what's the latest here?

@Santhosh-Sellavel
Copy link
Collaborator

@JmillsExpensify @MonilBhavsar what's next here?

@melvin-bot melvin-bot bot removed the Overdue label Mar 20, 2023
@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Mar 20, 2023

Looks like the workflow is being refactored here. So putting this issue on hold #15083
Respective PR #15672

@MonilBhavsar MonilBhavsar changed the title [$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box [HOLD 15083][$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box Mar 20, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 28, 2023
@MonilBhavsar
Copy link
Contributor

Still on hold

@slafortune
Copy link
Contributor

Changing to monthly

@melvin-bot melvin-bot bot removed the Overdue label Apr 7, 2023
@slafortune slafortune added Monthly KSv2 and removed Weekly KSv2 labels Apr 7, 2023
@melvin-bot melvin-bot bot added the Overdue label May 8, 2023
@MonilBhavsar
Copy link
Contributor

PR to introduce a new invite page was merged.
We're dealing with regressions from it currently. We'll be good to unhold this after follow up PR is merged #18601

@melvin-bot melvin-bot bot removed the Overdue label May 10, 2023
@MonilBhavsar MonilBhavsar changed the title [HOLD 15083][$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box [HOLD 18601][$250] mWeb/Chrome - Workspace - Unable to use Enter key for starting a new line under Personal message box May 10, 2023
@aldo-expensify
Copy link
Contributor

Is this the same issue as #18419 ?

@MonilBhavsar
Copy link
Contributor

@aldo-expensify yes, but we were only able to reproduce this issue on mWeb

@aldo-expensify
Copy link
Contributor

I think this is the same issue, it is the message when you are adding new workspace members, right? The issues is pretty old here so the videos are very outdated. The message is on a different screen now, but the bug is still there.

Do you have any preference on which one we should keep open?

I asked here in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1685138015892469 for opinions on how to deal with this keyboard shortcuts causing the issue.

@sakluger
Copy link
Contributor

sakluger commented Jun 1, 2023

I think we should close this issue since the other one is much newer. But since these are duplicate issues, we shouldn't pay the reporting bonus on the new issue since this one was reported first by Applause.

Gonna close this one out for now, feel free to reopen for discussion if you disagree.

@sakluger sakluger closed this as completed Jun 1, 2023
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. 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