feat: arrow navigation in context menu #27877

samh-nl · 2023-09-20T18:07:58Z

Details

Navigating the report actions context menu with arrows was not previously possible.

Fixed Issues

$ #23280
PROPOSAL: #23280 (comment)

Tests

Web/desktop

Go to any report that has a message
Right-click the message
Verify the context menu opens
Press the down arrow once
Verify the first menu item ('Reply in thread') is selected
Verify that you can navigate through the entire menu by pressing the up and down arrows
Use the arrows to go to the first menu item
Press Enter
Verify the page navigates to a thread and the context menu closes

Mobile

Go to any report that has a message
Long-press the message
Verify that the context menu opens

Verify that no errors appear in the JS console

Offline tests

Web/desktop

Go to any report that has a message
Right-click the message
Verify the context menu opens
Press the down arrow once
Verify the first menu item ('Reply in thread') is selected
Verify that you can navigate through the entire menu by pressing the up and down arrows
Use the arrows to go to the first menu item
Press Enter
Verify the page navigates to a thread and the context menu closes

Mobile

Go to any report that has a message
Long-press the message
Verify that the context menu opens

QA Steps

Web/desktop

Go to any report that has a message
Right-click the message
Verify the context menu opens
Press the down arrow once
Verify the first menu item ('Reply in thread') is selected
Verify that you can navigate through the entire menu by pressing the up and down arrows
Use the arrows to go to the first menu item
Press Enter
Verify the page navigates to a thread and the context menu closes

Mobile

Go to any report that has a message
Long-press the message
Verify that the context menu opens

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Web.Screen.Recording.2023-09-20.at.18.26.03.mov

Mobile Web - Chrome

mWeb.Chrome.webm

Mobile Web - Safari

mWeb.Safari.screencap-2023-09-20T165102.236Z.mp4

Desktop

Desktop.Screen.Recording.2023-09-20.at.18.27.06.mov

iOS

Native.iOS.screencap-2023-09-20T165350.509Z.mp4

Android

Native.Android.webm

melvin-bot · 2023-09-20T18:08:04Z

@situchan Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

samh-nl · 2023-09-20T18:12:13Z

For mobile, where there's no arrows, I have added to the test steps to verify that the context menu opens as expected when long-pressing. Let me know if you think I should change it.

src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

Co-authored-by: Situ Chandra Shil <108292595+situchan@users.noreply.github.com>

samh-nl · 2023-09-21T13:07:31Z

Thank you, I have accepted both suggestions.

situchan · 2023-09-21T13:08:03Z

Also, please pull main

situchan · 2023-09-21T13:19:33Z

unit test failing

Also, found the case of arrows not working on LHN context menu while focusing on composer

Screen.Recording.2023-09-21.at.7.17.56.PM.mov

samh-nl · 2023-09-21T14:11:17Z

Also, found the case of arrows not working on LHN context menu while focusing on composer

We prevent losing composer focus when clicking on an LHN item (here).
The intention behind it relates to left-clicks, not right-clicks that cause a context menu, but we don't check for that.

I think we need to have a similar handling as in EmojiReactionBubble (here) and check if e.button === 2, to ensure that the composer loses focus when the context menu opens. That solves the problem and makes it consistent with opening context menu elsewhere (report action context menu, FAB, etc all cause composer to lose focus).

situchan · 2023-09-21T14:14:17Z

I was about to say it's out of scope. There's already GH to blur composer when any popover is open - #25485

situchan · 2023-09-21T14:15:29Z

Please investigate why unit test is failing

samh-nl · 2023-09-21T14:26:31Z

Good find, perhaps then it is better to leave it for that issue as they are taking a broader approach which would make my solution redundant.

samh-nl · 2023-09-21T14:26:48Z

recheck

situchan

triggering actions

situchan · 2023-09-21T14:28:26Z

@samh-nl please revert full code and see it still happens

samh-nl · 2023-09-21T14:32:25Z

I see this particular unit test is also failing in another recent PR: https://github.com/Expensify/App/actions/runs/6261808769/job/17002648235?pr=27941

samh-nl · 2023-09-21T14:49:55Z

Could revert, but if you check the accepted suggestions: one of them caused the error, the other one didn't, so it's a bit random (it measures execution time). If the error doesn't occur after reverting, it might just be luck.

situchan · 2023-09-21T14:51:15Z

yes, let's try. Hope that "luck" won't happen

…extMenu.js" This reverts commit e8d9442.

…extMenu.js" This reverts commit 5869c6c.

This reverts commit fa0b8e7.

This reverts commit cd7278f.

situchan · 2023-09-21T15:12:17Z

pass. now re-revert

…tionContextMenu.js"" This reverts commit 777ca93.

…tionContextMenu.js"" This reverts commit 18a9e81.

situchan · 2023-09-21T15:24:30Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- 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)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- 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).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- 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.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- 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.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
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
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- 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)
- 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)
- All JSX used for rendering exists in the render method
- 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
If any new file was added I verified that:
- 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
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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)
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.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

situchan · 2023-09-21T15:25:52Z

Another weird behavior:

Screen.Recording.2023-09-21.at.9.25.06.PM.mov

Enter key triggers 2 actions at the same time

samh-nl · 2023-09-21T15:34:45Z

Also occurs when right-clicking an avatar. Focus is put on the element. Let me check how to solve this, shouldPreventDefault is already default true.

samh-nl · 2023-09-21T19:25:22Z

Here's my solution: The shouldPreventDefault is not enough, what it needs is event.stopPropagation();
For this, we could add a new config called shouldStopPropagation (default false, so it doesn't change any of the existing behavior), it would be placed here.

And then in our usage of useKeyboardShortcut, we add shouldStopPropagation: true to the configuration.

samh-nl · 2023-09-21T19:33:59Z

I see in the callback we pass the event, so instead of adding a new config, what also works is to add it like this:

useKeyboardShortcut(
        CONST.KEYBOARD_SHORTCUTS.ENTER,
-        () => {
+        (event) => {
+            // Ensures the event does not cause side-effects beyond the context menu, e.g. when an outside element is focused
+            event.stopPropagation();

situchan · 2023-09-21T21:14:49Z

ok, that looks good. Thanks for the research

samh-nl · 2023-09-21T21:55:51Z

I have pushed the change

situchan

@francoisl all yours

francoisl

LGTM, minor small change request

src/components/ContextMenuItem.js

situchan · 2023-09-22T00:43:17Z

src/components/ContextMenuItem.js

@@ -89,6 +99,7 @@ function ContextMenuItem({onPress, successIcon, successText, icon, text, isMini,
            descriptionTextStyle={styles.breakAll}
            style={getContextMenuItemStyles(windowWidth)}
            isAnonymousAction={isAnonymousAction}
+            focused={focused}


I also thought isFocused name is better but didn't request change because focused prop name already exists here 😄

I have adjusted the prop name in the added code.
If you'd like me to refactor the existing prop in MenuItem and change it there + it's usages also, let me know.

I think that's fine as we didn't touch MenuItem component here.
If anyone is adding/changing props on that component in the future, they might be responsible.
cc: @francoisl

Co-authored-by: Francois Laithier <francois@expensify.com>

francoisl

👍

OSBotify · 2023-09-22T08:46:22Z

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

situchan · 2023-09-22T08:54:02Z

There's 2 hrs delay from bonus timeline but it would be much appreciated if it's still eligible unless any regression found.
The delay was just from variable name change and @samh-nl was sleeping at that moment.
cc: @francoisl

OSBotify · 2023-09-22T13:58:41Z

🚀 Deployed to staging by https://github.com/francoisl in version: 1.3.73-0 🚀

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

OSBotify · 2023-09-25T09:33:32Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.73-1 🚀

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

OSBotify · 2023-09-25T10:05:30Z

🚀 Deployed to staging by https://github.com/francoisl in version: 1.3.74-0 🚀

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

OSBotify · 2023-09-28T18:47:51Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.74-3 🚀

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

samh-nl added 3 commits September 20, 2023 17:06

feat: arrow navigation in context menu

cd7278f

Merge branch 'main' into fix/issue-23280

977d4f1

fix: use internal press handler on Enter

fa0b8e7

samh-nl requested a review from a team as a code owner September 20, 2023 18:07

melvin-bot bot requested review from situchan and removed request for a team September 20, 2023 18:08

situchan reviewed Sep 21, 2023

View reviewed changes

src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js Outdated Show resolved Hide resolved

src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js Outdated Show resolved Hide resolved

samh-nl and others added 2 commits September 21, 2023 14:39

Update src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

5869c6c

Co-authored-by: Situ Chandra Shil <108292595+situchan@users.noreply.github.com>

Update src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js

e8d9442

Co-authored-by: Situ Chandra Shil <108292595+situchan@users.noreply.github.com>

Merge branch 'main' into fix/issue-23280

20a5415

situchan reviewed Sep 21, 2023

View reviewed changes

samh-nl added 4 commits September 21, 2023 16:57

Revert "Update src/pages/home/report/ContextMenu/BaseReportActionCont…

18a9e81

…extMenu.js" This reverts commit e8d9442.

Revert "Update src/pages/home/report/ContextMenu/BaseReportActionCont…

777ca93

…extMenu.js" This reverts commit 5869c6c.

Revert "fix: use internal press handler on Enter"

0867e44

This reverts commit fa0b8e7.

Revert "feat: arrow navigation in context menu"

fe46818

This reverts commit cd7278f.

samh-nl added 2 commits September 21, 2023 17:13

Revert "Revert "Update src/pages/home/report/ContextMenu/BaseReportAc…

56d0a92

…tionContextMenu.js"" This reverts commit 777ca93.

Revert "Revert "Update src/pages/home/report/ContextMenu/BaseReportAc…

c70a374

…tionContextMenu.js"" This reverts commit 18a9e81.

fix: prevent propagation of enter

10bb500

samh-nl added 2 commits September 22, 2023 00:00

fix: stopPropagation if item is selected

6428b28

fix: check event is set

4d4fff3

situchan approved these changes Sep 22, 2023

View reviewed changes

melvin-bot bot requested a review from francoisl September 22, 2023 00:16

francoisl requested changes Sep 22, 2023

View reviewed changes

src/components/ContextMenuItem.js Outdated Show resolved Hide resolved

situchan reviewed Sep 22, 2023

View reviewed changes

samh-nl and others added 2 commits September 22, 2023 08:48

Update src/components/ContextMenuItem.js

82ed8fc

Co-authored-by: Francois Laithier <francois@expensify.com>

style: isFocused

ea487db

francoisl approved these changes Sep 22, 2023

View reviewed changes

francoisl merged commit 33b3c88 into Expensify:main Sep 22, 2023
15 checks passed

OSBotify mentioned this pull request Sep 22, 2023

Deploy Checklist: New Expensify 2023-09-22 #28013

Closed

96 tasks

francoisl mentioned this pull request Sep 25, 2023

[HOLD for payment 2023-10-02] [$1000] Web- Action menu - Unable to navigate options in Action menu using Arrow keys #23280

Closed

6 tasks

This was referenced Sep 25, 2023

Deploy Checklist: New Expensify DUPE 2023-09-25 #28118

Closed

Deploy Checklist: New Expensify 2023-09-25 #28119

Closed

feat: arrow navigation in context menu #27877

feat: arrow navigation in context menu #27877

Conversation

samh-nl commented Sep 20, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 20, 2023

samh-nl commented Sep 20, 2023 • edited Loading

samh-nl commented Sep 21, 2023

situchan commented Sep 21, 2023

situchan commented Sep 21, 2023

samh-nl commented Sep 21, 2023 • edited Loading

situchan commented Sep 21, 2023

situchan commented Sep 21, 2023

samh-nl commented Sep 21, 2023

samh-nl commented Sep 21, 2023

situchan left a comment

Choose a reason for hiding this comment

situchan commented Sep 21, 2023

samh-nl commented Sep 21, 2023

samh-nl commented Sep 21, 2023

situchan commented Sep 21, 2023

situchan commented Sep 21, 2023

situchan commented Sep 21, 2023

Reviewer Checklist

Screenshots/Videos

situchan commented Sep 21, 2023 • edited Loading

samh-nl commented Sep 21, 2023 • edited Loading

samh-nl commented Sep 21, 2023

samh-nl commented Sep 21, 2023 • edited Loading

situchan commented Sep 21, 2023

samh-nl commented Sep 21, 2023

situchan left a comment • edited Loading

Choose a reason for hiding this comment

francoisl left a comment • edited Loading

Choose a reason for hiding this comment

situchan Sep 22, 2023

Choose a reason for hiding this comment

samh-nl Sep 22, 2023

Choose a reason for hiding this comment

situchan Sep 22, 2023

Choose a reason for hiding this comment

francoisl left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 22, 2023

situchan commented Sep 22, 2023 • edited Loading

OSBotify commented Sep 22, 2023

OSBotify commented Sep 25, 2023

OSBotify commented Sep 25, 2023

OSBotify commented Sep 28, 2023

samh-nl commented Sep 20, 2023 •

edited

Loading

samh-nl commented Sep 20, 2023 •

edited

Loading

samh-nl commented Sep 21, 2023 •

edited

Loading

situchan commented Sep 21, 2023 •

edited

Loading

samh-nl commented Sep 21, 2023 •

edited

Loading

samh-nl commented Sep 21, 2023 •

edited

Loading

situchan left a comment •

edited

Loading

francoisl left a comment •

edited

Loading

situchan commented Sep 22, 2023 •

edited

Loading