[$1000] [HOLD for payment 2023-05-16] Typing a new line with "SHIFT + ENTER" in the composer doesn't work (neither the arrow keys to move the cursor up or down)

[aldo-expensify]

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

---

Action Performed:

1. Have a chat with 2 users (User A, User B)
2. Type something in the composer
3. Press "SHIFT + ENTER"

Related bug?

1. Paste a text that contain multiple lines in the composer
2. Try to move the cursor with up and down keyboard arrows (doesn't work)

Expected Result:

A new line is added

Actual Result:

It isn’t creating new line

Workaround:

Type the message somewhere else and copy/paste it into the composer

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.10-2
Reproducible in staging?: y
Reproducible in production?: n
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
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1683219007822509

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ead9a9eb95ee636d
• Upwork Job ID: 1658647113534902272
• Last Price Increase: 2023-05-17

[MelvinBot]

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

[MelvinBot]

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

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[MelvinBot]

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

[Pujan92]

Regression from this PR #18273

[aldo-expensify]

Regression from this PR #18273

Thanks!

[aldo-expensify]

@azimgd do you see and easy fix to avoid the bug the #18273 ?

otherwise, I think we should revert

[Pujan92]

Need to pass excludeNodes TextArea for this subscribe event method(Reason - not to call this configured callback and function normally for the text input enter).

App/src/components/KeyboardShortcutsModal.js

Lines 52 to 56 in ac16487

	const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	this.unsubscribeCloseEnterModal = KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => {
	ModalActions.close();
	KeyboardShortcutsActions.hideKeyboardShortcutModal();
	}, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true, () => !this.props.isShortcutsModalOpen);

Extra last 3 parameters have to pass I think.
, 0, true, ['TEXTAREA']

[aldo-expensify]

@Pujan92 that does seem to fix the problem, can you elaborate a bit about the root cause and why this fixes it? I'm pretty unfamiliarized with the KeyboardShortcut.subscribe function.

[aldo-expensify]

This is not fixing the up and down arrows keys not working, I'm guessing we have to do something similar for the other shortcuts?

Update: applying the same solution to the other shortcuts (up/down) seems to fix that problem too.

[aldo-expensify]

Putting up a PR with what @Pujan92 proposed: #18422

[Pujan92]

Basically, we are configuring the callback function for these keyboard shortcuts and I think with this we are restricting the normal behavior of those key shortcuts. So for textInput we don't want to use this callback and let the OS do the normal function. Considering that we are providing the nodes which we want to exclude from this behavior where I am suggesting to pass TEXTAREA target nodes.

[aldo-expensify]

A bit more on this after debugging:

The shortcuts (ENTER / UP ARROW / DOWN ARROW) have shouldPreventDefault = true, which makes us call event.preventDefault(); here:

App/src/libs/KeyboardShortcut/bindHandlerToKeydownEvent/index.js

Lines 46 to 48 in 2437e37

	if (callback.shouldPreventDefault) {
	event.preventDefault();
	}

That prevent default is the thing that stops the textarea from behaving normally.

This proposal or similar of captureOnInputs = false when we call KeyboardShortcut.subscribe here:

App/src/components/KeyboardShortcutsModal.js

Lines 52 to 65 in 2437e37

	const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	this.unsubscribeCloseEnterModal = KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => {
	ModalActions.close();
	KeyboardShortcutsActions.hideKeyboardShortcutModal();
	}, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true, () => !this.props.isShortcutsModalOpen, 0, true, ['TEXTAREA']);
	// Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open
	const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP;
	this.unsubscribeArrowUpKey = KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => {
	}, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true, () => !this.props.isShortcutsModalOpen, 0, true, ['TEXTAREA']);
	const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN;
	this.unsubscribeArrowDownKey = KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => {
	}, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true, () => !this.props.isShortcutsModalOpen, 0, true, ['TEXTAREA']);

makes us avoid calling event.preventDefault(); because we exit early here:

App/src/libs/KeyboardShortcut/bindHandlerToKeydownEvent/index.js

Lines 24 to 35 in 2437e37

	if (_.contains(callback.excludedNodes, event.target.nodeName)) {
	return true;
	}
	// If configured to do so, prevent input text control to trigger this event
	if (!callback.captureOnInputs && (
	event.target.nodeName === 'INPUT'
	|| event.target.nodeName === 'TEXTAREA'
	|| event.target.contentEditable === 'true'
	)) {
	return true;
	}

[aldo-expensify]

@s77rt proposed here another path that makes sense to me: https://expensify.slack.com/archives/C02NK2DQWUX/p1683222520307999?thread_ts=1683221275.330439&cid=C02NK2DQWUX

if the modal is not open, don't even capture the keys, no matter if if textarea or not

[aldo-expensify]

I think the options are:

1. Don't subscribe the shortcut if the modal is not open: best IMO, but a bit harder to implement, it probably would rely on componentDidUpdate and checking prop's current and previous values.
2. Don't prevent default if the modal is not open: The parameter shouldPreventDefault for subscribe would need to accept a boolean or function, just like shouldBubble
3. Exclude inputs passing captureOnInputs = false to subscribe

I'm not decided on which way to go yet.

[aldo-expensify]

Implemented Option 1 here #18425, which to me feels less like a workaround. I think this fix is a bit better fix than #18422 because I think #18422 could still unexpectedly block events that are not targeted for inputs.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Current assignee @aldo-expensify is eligible for the External assigner, not assigning anyone new.

[arielgreen]

Just added External to get the Upwork job created, unassigning @mananjadhav

[arielgreen]

@s77rt and @Pujan92 can you please accept the offers in Upwork, and I will remit payment. Thanks.

[s77rt]

@arielgreen Accepted, but let's hold payment till we get a clarification on regressions.

@aldo-expensify Did the linked PR cause any regression? I'm not sure as it"s linked in some issues.

[aldo-expensify]

@aldo-expensify Did the linked PR cause any regression? I'm not sure as it"s linked in some issues.

Which linked PR? this one? #18273

I think that caused the regression.

[melvin-bot]

📣 @s77rt You have been assigned to this job by @aldo-expensify!
Please apply to this job in Upwork 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 📖

[aldo-expensify]

@sr77rt adding you as the C+ of the issue

[aldo-expensify]

Started slack discussion about how to prevent this in the future: https://expensify.slack.com/archives/C049HHMV9SM/p1684528122955109

[s77rt]

@aldo-expensify I meant the other way around. Did this PR cause other regressions?

[aldo-expensify]

@aldo-expensify I meant the other way around. Did this PR cause other regressions?

Ahh I see. Not that I remember. You mentioned an issue linking this, can you copy the link here?

[s77rt]

#18599

[aldo-expensify]

That looks unrelated.

[s77rt]

Ah okay. Just wanted to be sure.

@arielgreen I think we are good to issue the payment.

[aldo-expensify]

Ah okay. Just wanted to be sure.

All good, thanks for checking !

[arielgreen]

paid