Fix: mWeb- Unable to move to another line by clicking "return" button

[ahmdshrif]

Details

Fixed Issues

$ #7678

Tests

• Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

• I made sure to pull main before submitting my PR for review
• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I clearly indicated the environment tests should be run in (Staging vs Production)
• I wrote testing steps that cover success & fail scenarios (if applicable)
• I ran the tests & they passed on all platforms
• I included screenshots or videos for tests on all platforms
• I verified there are no console errors related to changes in this PR
• I followed proper code patterns (see Reviewing the code)
  • I added comments when the code was not self explanatory
  • I put all copy / text shown in the product in all src/languages/* files (if applicable)
  • I followed proper naming convention for platform-specific files (if applicable)
  • I followed style guidelines (in Styling.md) for all style edits I made
• I followed the guidelines as stated in the Review Guidelines

PR Reviewer Checklist

• I verified the Author pulled main before submitting the PR
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the testing environment is mentioned in the test steps
• I verified testing steps cover success & fail scenarios (if applicable)
• I verified tests pass on all platforms & I tested again on all platforms
• I checked that screenshots or videos are included for tests on all platforms
• I verified there are no console errors related to changes in this PR
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified comments were added when the code was not self explanatory
  • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
  • I verified proper naming convention for platform-specific files was followed (if applicable)
  • I verified style guidelines were followed
• I verified that this PR follows the guidelines as stated in the Review Guidelines

QA Steps

1- Log with any account
2- Navigate to Settings > Workspace > Manage members > Invite
3- Start typing in the custom message field
4- Hit "return" button
5- make sure enter make new line

• Verify that no errors appear in the JS console

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Screen.Recording.2022-02-25.at.8.32.29.PM.mov

Mobile Web

Screen.Recording.2022-02-25.at.8.29.42.PM.mov

Desktop

Screen.Recording.2022-02-25.at.8.45.36.PM.mov

iOS

Screen.Recording.2022-02-25.at.8.31.26.PM.mov

Android

Screen.Recording.2022-02-25.at.8.40.10.PM.mov

[parasharrajat]

HOLD for #7702

[luacmartins]

Changes look good! But I'll have to test once the hold is removed.

[parasharrajat]

#7702 is broken into multiple PRs and now we need to hold for #8059 which is ready for merge.

[rushatgabhane]

@parasharrajat @ahmdshrif event bubbling is now live

[ahmdshrif]

thanks, rushatgabhane will work on this now.

[parasharrajat]

Awesome @rushatgabhane. Thanks for the heads up.
@ahmdshrif Please merge main before doing changes. Now TextArea comparison logic should work.

[ahmdshrif]

@parasharrajat this does not work after merge main also

if (this.props.isDisabled || this.props.isLoading || !e || e.target.nodeName === 'textarea' ) {
               return;
           }

I do some debug to see where this issue come form

App/src/libs/KeyboardShortcut/index.js

Lines 83 to 86 in c26e415

	if (_.isFunction(callback.callback)) {
	callback.callback(event);
	}
	event.preventDefault();

here we call the callback if it exists and it returns but this function will continue and will call event.preventDefault();
which stop the default behavior of the key on textinput (to make new line)

so changing on callback will not effect

if the event.preventDefault(); not needed we can delete it otherwise we need to add new param like activeOnTextArea
and return early before preventDefault called. the same we do with pressOnEnter but new param only for textArea

[parasharrajat]

Oh, shoot. Let's raise the same point on the related PR. I will try to see why are we preventing default for no reason. If this is needed, I would say to add a new param to subscription shouldPreventDefault which will skip event.preventDefault call when false.

[parasharrajat]

@ahmdshrif Ok, I did some research and I think preventDefault is necessary for some cases for Lib to work.

This is the action plan now.

1. Add a new param shouldPreventDefault:Boolean defaults to true to the KeyboardShortcut.subscribe.
2. Save this property on the eventHandlers object.
3. in bindHandlerToKeydownEvent. Add an if check around the event.preventDefault call that if shouldPreventDefault is true run it.
4. Add the logic same as Fix: mWeb- Unable to move to another line by clicking "return" button  #7920 (comment) which I suggested.
5. In the Button.js where we are subscribing set shouldPreventDefault=false.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[Ahmed]

Wrong ahmed

…

On Fri, Mar 11, 2022, 7:46 AM github-actions[bot] ***@***.***> wrote: *CLA Assistant Lite bot:* Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement <https://github.com/Expensify/App/blob/main/CLA.md> before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format. ------------------------------ *I have read the CLA Document and I hereby sign the CLA* ------------------------------ *1* out of *2* committers have signed the CLA. ✅ @ahmdshrif <https://github.com/ahmdshrif> ❌ @Ahmed <https://github.com/Ahmed> Sherif *Ahmed Sherif* seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account <https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user> . You can retrigger this bot by commenting *recheck* in this Pull Request — Reply to this email directly, view it on GitHub <#7920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDHCZLIVH5VWMK66GPDMDU7M6C7ANCNFSM5PK6QRXA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[ahmdshrif]

I wite the comment before seeing your comment 😅

ok, shoudpreventDefault is working fine I push it.

also, I think about it we need this option for example on the report screen we need to preventDefault enter key to not make a new line and send at the same time.

but there is another way to make shoudpreventDefault an option. the callback already has access to event object a so we can remove it and if he wants to preventDefault he can call it on the callback.

[parasharrajat]

I saw what you mean but it is possible that there are many event listeners attached and manipulating the native property is a bad practice.

[parasharrajat]

Found a critical issue https://expensify.slack.com/archives/C01GTK53T8Q/p1647009526394119 which prevents testing the PR as it is. Although, I am sure that it works. But I am leaving this to Carlos.

[luacmartins]

I think we should fix that issue and fully test this PR before merging it!

[ahmdshrif]

I think the context issue is affected disabled and loading button which has not changed in this pr.
otherwise, the changes solve the return button issue. and we can test it. since it's now working fine on all cases.

[parasharrajat]

LGTM. That was not a real bug. Debugger going nuts.

cc: @luacmartins

PR Reviewer Checklist

• I verified the PR has a small number of commits behind main
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the testing environment is mentioned in the test steps
• I verified testing steps cover success & fail scenarios (if applicable)
• I checked that screenshots or videos are included for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors related to changes in this PR
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified comments were added when the code was not self explanatory
  • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
  • I verified proper naming convention for platform-specific files was followed (if applicable)
  • I verified style guidelines were followed
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified the UI performance was not affected (the performance is the same than main branch)
• If a new component is created I verified that a similar component doesn't exist in the codebase

🎀 👀 🎀 C+ reviewed

[luacmartins]

LGTM and tested well! Thanks for the changes!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by @luacmartins in version: 1.1.43-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.43-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅