Allow keyboard shortcuts to work on native devices

[azimgd]

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/6883 PROPOSAL: https://github.com/Expensify/App/issues/6883

Tests

• [x] Verify that no errors appear in the JS console

Offline tests

Same as QA Steps.

QA Steps

Note: you need to connect hardware keyboard to test mobile keyboard shortcuts on iOS and Android.

1. Press CMD + K to open "search" sidebar
2. Press CMD + SHIFT + K to open "newGroup" sidebar
3. Press CMD + I to open "shortcut" dialog
4. Press ESC to close any sidebar or dialog
5. Press ENTER to submit attachment dialog

https://github.com/Expensify/App/blob/ef6937c38197da7124341faefd2218260da7effe/src/CONST.js#L231-L282 All of the shortcuts here should work on Web.

List of known issues on Android and iOS: https://github.com/Expensify/App/pull/14767#issuecomment-1494158230 which are not counted as regressions.

• [x] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android / native
  • [x] Android / Chrome
  • [x] iOS / native
  • [x] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

https://user-images.githubusercontent.com/4882133/217061211-7b10e95f-ca0c-46b1-bec7-64d143e0df37.mov

Mobile Web - Chrome

https://user-images.githubusercontent.com/4882133/228418338-0c398127-98ba-43c6-a8f8-217a0ede50a6.mov

Mobile Web - Safari

https://user-images.githubusercontent.com/4882133/228415502-02486d6c-e8a6-4e30-92bb-2ebd9260064d.mp4

Desktop

https://user-images.githubusercontent.com/4882133/217061108-29148272-46de-4c5d-ae54-c5107f1d8927.mov

iOS

https://user-images.githubusercontent.com/4882133/228414893-628d999f-0e2c-4de5-82ba-2b30dbb5734c.mp4

Android

https://user-images.githubusercontent.com/4882133/216602144-6bc2ee7f-80f3-4578-bd92-cda60d071d29.MOV

[parasharrajat]

Built a production app and faced the same issue https://github.com/Expensify/App/pull/14767#issuecomment-1424833574 https://github.com/Expensify/App/pull/14767#issuecomment-1427092940. I suspect it might be related to OS.

@luacmartins Can you please help with the following?

1. Testing the above-mentioned bug on Android.
2. Testing shortcuts on iOS App on a real device. I can't run it on the simulator.

[parasharrajat]

Asking for help on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1677264003502069.

[luacmartins]

Sorry, I've been focused on a roadmap project and the ECX presentation so I haven't been able to prioritize this. I'll try to get back to it next week!

[parasharrajat]

No problem. Thanks.

[alexxxwork]

2. Testing shortcuts on iOS App on a real device. I can't run it on the simulator.

@parasharrajat Do you still need help?

[parasharrajat]

@alexxxwork Yeah, that is still remaining.

[alexxxwork]

Tested this PR on a hardware device with bluetooth keyboard attached. Key combos work (ctrl + I, ctrl + K, ctrl + shift + K ).

• ESC doesn't work reliably - sometimes it's just closing the app and brings you to home screen. There's an error in logcat on such cases:

  2023-02-28 08:27:03.774 25142-25142 unknown:ReactNative     com.expensify.chat                   E  Tried to remove non-existent frame callback

• Every time you see a popup you should press TAB to make a selection with keyboard arrow keys
• Can't manage to test Enter shortcut. It just closes popup on submit attachment window, you should select submit button with TAB.
• Arrow keys work strange on a report page - up key scrolls down through chat messages and down key scroll up.

I don't see such a behaviour on search modal https://github.com/Expensify/App/pull/14767#issuecomment-1427092940 But there's another strange thing:

1. Press Ctrl + K
2. You see a searh modal and the focus is on search textbox. Arrow keys doesn't work
3. Press Tab
4. Press down keys several times
5. In the end of the searh list selection behaves in a strange way - two elements are highlighted. This happens only for the first time. Next round of scrolling is ok.

Screenshots

 

[parasharrajat]

Thanks,@alexxxwork. You have similar results like mine. This will really help.

Cc @azimgd.

[azimgd]

Hey @alexxxwork, thank you for a quick feedback.

ESC doesn't work reliably - sometimes it's just closing the app and brings you to home screen.

I believe this happens on a home page, right? Pressing ESC on a home page will minimize the app per Android OS.

Can't manage to test Enter shortcut. It just closes popup on submit attachment window, you should select submit button with TAB.

Could you confirm if pressing ENTER doesn't send an attachment in the popup before closing it? We don't support pressing Tab to focus on elements.

Arrow keys work strange on a report page - up key scrolls down through chat messages and down key scroll up.

While I haven't noticed this behavior, Flatlist on a (home) report page is not wrapped with a keyboard subscription library and operates fully on a native side.

I don't see such a behaviour on search modal https://github.com/Expensify/App/pull/14767#issuecomment-1427092940

But search modal is wrapped.

In the end of the searh list selection behaves in a strange way - two elements are highlighted. This happens only for the first time. Next round of scrolling is ok.

Same with this one, keyboard subscription is interfering with a FlatList.

[azimgd]

@alexxxwork https://expensify.slack.com/archives/C049HHMV9SM/p1676401683622009

[alexxxwork]

I believe this happens on a home page, right? Pressing ESC on a home page will minimize the app per Android OS.

Not only homepage, but any popup in App. Seems like an app crash. It looks like an error calling back.

Could you confirm if pressing ENTER doesn't send an attachment in the popup before closing it?

Yes it doesn't

But search modal is wrapped.

I mean that I don't see problems, that @parasharrajat reported here - search elements scroll with arrow keys works just ok.

While I haven't noticed this behavior, Flatlist on a (home) report page is not wrapped with a keyboard subscription library and operates fully on a native side.

I think this should be fixed maybe in another issue

[luacmartins]

Thanks for helping with tests @alexxxwork!

[parasharrajat]

Every time you see a popup you should press TAB to make a selection with keyboard arrow keys

It happened to me on New Chat or New Group Page.

Arrow keys work strange on a report page - up key scrolls down through chat messages and down key scroll up.

Scrolls the whole chat list for me.

---

I think that for us native keyboard handling is interfering with custom shortcuts. For example, on the web, arrow keys normally will scroll the list and there are other shortcuts as well that are native to OS. I think the same thing is happening when a Hardware keyboard is attached to the device. OS detects the keyboard and enables native shortcuts like scrollable list scrolls with arrow keys, and tab keys.

---

It might not be happening if we are using a Bluetooth keyboard. I believe that the code is correct but there might be edge cases based on OS. Mine is MiUI.

Can we utilize BrowserStack to test this? Need to check.

[alexxxwork]

For example, on the web, arrow keys normally will scroll the list

@parasharrajat You're right, arrow keys works on web just the same - up/down reversed. I think that ESC and Enter issues should be fixed and also "keyboard focus" in popups. Arrows are another issue or maybe feature not bug 😄

[azimgd]

Hey @alexxxwork would you mind checking this again and see if the latest update fixed "Enter" and "Escape" bug you had.

[alexxxwork]

@azimgd Still there's problem with popups (you need to press Tab and then make a selection) Enter shortcut works now in upload attachment modal 👍 ESC bug is still here. Pressing Ctrl-K then ESC brings me to phone home screen. Ctrl + I + ESC works ok. The error in Android Studio's logcat is the same Tried to remove non-existent frame callback

[azimgd]

Hey @alexxxwork, let's try to clarify some points from your feedback.

@azimgd Still there's problem with popups (you need to press Tab and then make a selection)

I'm not going to count this as a bug unless Carlos or Rajat has a very strong argumentation. It's a feature request since initial implementation was done for web only.

ESC bug is still here. Pressing Ctrl-K then ESC brings me to phone home screen. Ctrl + I + ESC works ok.

Would you mind checking this on main branch please? I think what you have there is an interference with native OS functionality. Shortcut popup (ctrl + i) is a modal while Search popup (ctrl + k) is a page.

Hope I addressed all outstanding comments.

[alexxxwork]

I'm not going to count this as a bug unless Carlos or Rajat has a very strong argumentation. It's a feature request since initial implementation was done for web only.

@azimgd I've just took a look at the original issue. 32k for this issue that's huge 👍 I think @parasharrajat should clarify the amount of work to be done on this issue.

Would you mind checking this on main branch please?

Tested on release from google play. Obviously Ctrl+K don't work. But ESC from pages (settings, group chat, etc.) works correctly. So I suppose this is a bug in your implementation.

Do you need help solving this ESC problem?

[azimgd]

@azimgd I've just took a look at the original issue. 32k for this issue that's huge 👍

Not sure I understand this part.

Tested on release from google play. Obviously Ctrl+K don't work. But ESC from pages (settings, group chat, etc.) works correctly. So I suppose this is a bug in your implementation.

Looking at this.

[parasharrajat]

Thanks @alexxxwork for continued feedback. I won't suggest investing time into this anymore if you are expecting compensation. Because I can't guarantee that.

---

Obviously, we will chase all the bugs or inconsistencies that are present on this PR.

But the main focus is to land the first working draft. Every shortcut should at least work. Edge cases can be tackled later.

[alexxxwork]

Not sure I understand this part.

@azimgd congratulations on winning an issue worth of $32k.

@parasharrajat I understand this. Anyway I can test this PR a few times more, not a problem.

[azimgd]

As I mentioned above ESC is interfering with native OS based implementation. There are couple of ways solving this but not directly related to the work being done in this PR.

1. utilizing: https://reactnative.dev/docs/backhandler (Android only)
2. disable ESC propagation on a application level:

@Override
public boolean onKeyDown(int keyCode, KeyEvent event) {
  // disabling hardware ESCAPE support which acts as a button
  if (keyCode == 111) { return false; }

  KeyCommandModule.getInstance().onKeyDownEvent(keyCode, event);

  return super.onKeyDown(keyCode, event);
}

Skipping this as it's not related to: "Allow keyboard shortcuts to work on native devices"

[alexxxwork]

@azimgd @parasharrajat if you need my opinion here, I think that QA test steps for this PR should work without errors, but they don't for now:

QA Steps

Note: you need to connect hardware keyboard to test mobile keyboard shortcuts.

1. Press CMD + K to open "search" sidebar
2. Press CMD + SHIFT + K to open "newGroup" sidebar
3. Press CMD + I to open "shortcut" dialog
4. Press ESC to close any sidebar or dialog There's an error in logcat at this step and the key doesn't work as intended
5. Press ENTER to submit attachment dialog This step works

[azimgd]

@alexxxwork you were absolutely right about the ESC button, which was broken after "ENTER" support fix. This should be fixed now. Thank you.

Do we have any unresolved points left from your feedback ?

[alexxxwork]

@azimgd Actually it was broken before ENTER fix, the first time I tested it.

Enter and ESC works ok now 👍

tested all these QA steps.

1. Press CMD + K to open "search" sidebar
2. Press CMD + SHIFT + K to open "newGroup" sidebar
3. Press CMD + I to open "shortcut" dialog
4. Press ESC to close any sidebar or dialog
5. Press ENTER to submit attachment dialog

There's also a minor edge case. When you press Enter in a "search" sidebar it closes the page, but I think it shouldn't. Maybe this is a side-effect of the previous Enter fixing commit https://github.com/Expensify/App/pull/14767/commits/10a080eab1c68bdebdfe0d8bc3b777b892cb21c2

@parasharrajat if you need my opinion for native keyboard navigation in general - it's hard to use it whithout right assigned tabindexes. When you use it in a dialog it's hard to switch to text input using keyboard only because you got to scroll ALL messages.

[luacmartins]

When you press Enter in a "search" sidebar it closes the page, but I think it shouldn't. Maybe this is a side-effect of the previous Enter fixing commit

This seems to be the behavior on main too, so I think we can tackle it as a separate PR! Thanks for all the testing @alexxxwork!

[luacmartins]

@azimgd would you mind fixing the conflicts so we can do a final review?

[alexxxwork]

This seems to be the behavior on main too, so I think we can tackle it as a separate PR!

@luacmartins Should I first report this as a bug?

[luacmartins]

Go for it @alexxxwork

[parasharrajat]

@luacmartins Can you please test this on ioS? I guess it needs to build an app on iOS. Maybe add the label ready to build to help us test it out.

[luacmartins]

I triggered the testing builds. We still have conflicts though

[parasharrajat]

@azimgd Please merge main.

[azimgd]

@luacmartins I get "This app cannot be installed because its integrity could not be verified."

note: "developer mode" is enabled on my device.

[luacmartins]

I'm building a new version for testing since we merged main and resolved conflicts. I posted in Slack about the iOS integrity issue.

[azimgd]

Same error. Are you able to run it on your end?

[luacmartins]

No, I get the same error

[azimgd]

Could you try building it manually from your machine?

[luacmartins]

@azimgd gonna look into this now. Meanwhile, some jest tests are failing on this branch, you might wanna take a look at that.

[azimgd]

Tests are failing on main.

[azimgd]

I will be mostly unavailable starting this Thursday, for about a month. I will appreciate if we can get this merged within next 2 days.

[luacmartins]

@azimgd tests on main seem to be passing for me. I also see recent PRs, e.g. this one where tests are passing.

[luacmartins]

@azimgd we tested on android and iOS but had a few issues:

Android

1. CMD + SHIFT + K doesn't seem to work
2. CMD + I opens the shortcut modal, but immediately closes it
3. All other shortcuts seem to work great!

iOS

1. None of the shortcuts seem to work for me and another contributor also had issues. Maybe we are holding something wrong?

[azimgd]

That should be ctrl, not cmd right?

[luacmartins]

Correct!

[azimgd]

@alexxxwork ctrl + shift + k was working for your right ?

[luacmartins]

@azimgd an update. ctrl + shift + k seems to be working for me now, not sure why it didn't before.

I'm still having issues with ctrl + i though. It seems like when I release the keys before the animation finishes, it closes the modal right away. However, if I keep the keys pressed until after the animation is over, the modal stays open.

[luacmartins]

@azimgd Never mind, ctrl + i seems to work fine now. Maybe it's an issue with my device + keyboard 🤔

[luacmartins]

So only the issue with iOS remains. I'm testing that again now.

[alexxxwork]

@alexxxwork ctrl + shift + k was working for your right ?

yeah, all worked ok.

[azimgd]

Could you please check if the commit above fixes iOS ?