[HOLD for payment 2022-05-20][$4000] Windows - Cursor move to next line if you press Enter

[kbecciv]

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 https://staging.new.expensify.com and log in with any account
2. Go to any conversation
3. Start typing the message and press Enter

Expected Result:

Message should be sent after press Enter

Actual Result:

Cursor move to next line if you press Enter

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web/ Windows

Version Number: 1.1.38.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Enter.1102.mp4

Recording.208.mp4

Upwork job link: https://www.upwork.com/jobs/~01e9d1a86d39175b4d

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

[MelvinBot]

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

[chiragsalian]

Hmm i'm not able to reproduce this on staging or production. Are you still able to consistently reproduce this and are you sure the shift key was not held?

[chiragsalian]

Friendly bump, any update to my question above @kbecciv? 🙂

[kbecciv]

@chiragsalian Yes, I am consistently reproduce this issue, no shift key was held for this action.

[chiragsalian]

Hmm weird, I cannot reproduce. Anyway no worries i'll open it up to contributors that can test and figure out the problem.

[MelvinBot]

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

[michaelhaxhiu]

I also cannot reproduce this bug, and that's typically a mandetory step for me as a contributor manager before exporting assigning this job to upwork.

In order to proceed, I think we need more context on how to reproduce this issue. I followed steps under "Action Performed" and whenever I hit enter it always sends the message exactly as expected. @kbecciv sorry to ping you again but any chance you can try to add more context here?

[mvtglobally]

@michaelhaxhiu @chiragsalian I understand that you may have an issue reproducing the issue. It is not repro on MacOS. We have multiple users who are facing this and it seems to be Device specific. Issue was not happening in the past, and we believe it is a regression from some PR.
E.g every user with ASUS laptop is able to see it.

[parasharrajat]

@mvtglobally In any case Do those laptops have a touch screen?

[mvtglobally]

@parasharrajat i can ask others, but my laptop has a touchscreen where I can repro the issue

[parasharrajat]

Gotcha. Ok, I can see which PR caused it and why?

This is the culprit code

App/src/pages/home/report/ReportActionCompose.js

Line 343 in 6ef2e51

if (!e || VirtualKeyboard.shouldAssumeIsOpen()) {

.

Although, the last commit is mine 😸. But this change was originally added #7355 .

In Short, if a device has a touch screen Enter to submit shortcut is disabled. But I see that it can be irritating for touch screen laptop users who are not really using the touchscreen.

[chiragsalian]

@michaelhaxhiu, did you post this job in upwork? If not please do. Looks like it's specific to touchscreen devices.

@parasharrajat, thanks for the find. I'm not sure if the last statement is a solution. Either way, can you explain where you'll be adding the code and what would it be for clarity. Additionally, I was curious if this is something you can test or not? Worst case we can still push out the fix and ask QA if it's been resolved.

[parasharrajat]

I haven't proposed anything yet #7715 (comment) is an explanation of the root cause.

I don't see any solution that allows us to detect that the user is using an external keyboard so we will have to detect that the user is on desktop.
Possible options.
2. dimensions check.
3. UserAgent check.
4. feature detection.

I will try to find a bulletproof solution for it.

[chiragsalian]

Cool, thanks for clarifying. I'll bump this down to weekly since i don't feel like its a daily. If anyone disagrees feel free to bump it up.

[michaelhaxhiu]

@michaelhaxhiu, did you post this job in upwork? If not please do. Looks like it's specific to touchscreen devices.

done

https://www.upwork.com/jobs/~01e9d1a86d39175b4d

[parasharrajat]

I meant feature detection for detecting if a device is a desktop with an external keyboard.

Dimensions check

This was/is a testing method on a few CSS frameworks like bootstrap. But I feel like, it is going to be buggy.

At last, until we break anything for users that are using isOpen api, I am good with userAgent check. Could you please tell me the places where this is used and how will your solution work without breaking those?

[mateusbra]

I meant feature detection for detecting if a device is a desktop with an external keyboard.
Dimensions check
This was/is a testing method on a few CSS frameworks like bootstrap. But I feel like, it is going to be buggy.

As far as I could find, people count time and make assumptions based on time between KeyDown and KeyUp to know if the keyboard is either real or virtual:
https://stackoverflow.com/questions/18880236/how-do-i-detect-hardware-keyboard-presence-with-javascript
I think it's hacky and more a workaround than achieving the desired solution and won't work everytime.
I still strongly think we should go with userAgent check.

So in my updated proposal:

PROPOSAL

in my solution we are going to create new folder/files within https://github.com/Expensify/App/tree/main/src/libs/
isMobile

with the content:
index.js:

/**
 * Allows us to identify whether the platform is mobile.
 *
 * https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent
 *
 * @returns {Boolean}
 */
 function isMobile(){
        return /Android|webOS|iPhone|iPad|iPod|BlackBerry|BB|PlayBook|IEMobile|Windows Phone|Silk|Opera Mini/i.test(navigator.userAgent);
}


export default isMobile;

index.native.js:

function isMobile() {
    return true;
}

export default isMobile;

And use it here instead of use canUseTouchScreen()

App/src/libs/VirtualKeyboard/index.js

Lines 16 to 26 in fd0f83a

	/**
	* As of January 2022, the VirtualKeyboard web API is not available in all browsers yet
	* If it is unavailable, we default to assuming that the virtual keyboard is open on touch-enabled devices.
	* See https://github.com/Expensify/App/issues/6767 for additional context.
	*
	* @returns {Boolean}
	*/
	function shouldAssumeIsOpen() {
	const isOpened = isOpen();
	return _.isNull(isOpened) ? canUseTouchScreen() : isOpened;
	}

After changes:

/**
 * As of January 2022, the VirtualKeyboard web API is not available in all browsers yet
 * If it is unavailable, we default to assuming that the virtual keyboard is open on mobile devices.
 * See https://github.com/Expensify/App/issues/7715 for additional context.
 *
 * @returns {Boolean}
 */
function shouldAssumeIsOpen() {
    const isOpened = isOpen();
    return _.isNull(isOpened) ? isMobile() : isOpened;
}

[parasharrajat]

At last, until we break anything for users that are using isOpen api, I am good with userAgent check. Could you please tell me the places where this is used and how will your solution work without breaking those?

What do you think about this?

[mateusbra]

What do you think about this?

Sorry I didn't get what you want...

If you mean where we will use it:
We will use it here:

App/src/pages/home/report/ReportActionCompose.js

Lines 342 to 344 in 6ef2e51

	triggerHotkeyActions(e) {
	if (!e || VirtualKeyboard.shouldAssumeIsOpen()) {
	return;

Instead of use canUseTouchScreen() and assume when canUseTouchScreen = true the device is mobile we will use the userAgent check to tell us when we are on mobile devices(only when we don't have the API available)

[parasharrajat]

ok. I think this mobile userAgent check is best suited for shouldAssumeIsOpen instead of canUseTouchScreen. IMO, isMobile should be moved to getBrowser lib.

Rest, I am fine with @mateusbra's approach to this problem #7715 (comment). I was waiting on a proposal with feature detection but there is no such API atm. And @mateusbra's reason for dimensions check seems valid.

cc: @chiragsalian

🎀 👀 🎀 C+ reviewed

[chiragsalian]

Proposal LGTM as well. Feel free to create the PR @mateusbra.

[melvin-bot]

📣 @mateusbra You have been assigned to this job by @chiragsalian!
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 📖

[mateusbra]

I submitted a proposal on Upwork, gonna rise a PR soon!
cc: @michaelhaxhiu

[michaelhaxhiu]

Hired - let's get this party started.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[melvin-bot]

This issue has not been updated in over 15 days. @michaelhaxhiu, @chiragsalian, @parasharrajat, @mateusbra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[parasharrajat]

PR deployed on PROD 4 days back.

[michaelhaxhiu]

Just a quick bud check -- so does that mean payment is due on Friday for @mateusbra, assuming no regressions between now & then.

Weird the GH title didn't update to add that 🤔

[parasharrajat]

Yeah, Please update the title. It must be due to the wrong link format used on PR. Please update that as well if possible.

[michaelhaxhiu]

@parasharrajat can you apply for this job - https://www.upwork.com/jobs/~0112030d1e6d50fa17 for the C+ work

[michaelhaxhiu]

@mateusbra has been paid.

[mvtglobally]

Issue not reproducible during KI retests. (Third week)

[parasharrajat]

Ping for

@michaelhaxhiu

[michaelhaxhiu]

Waiting for you to accept job offer @parasharrajat 👍

[michaelhaxhiu]

Paid.