[HOLD for payment 2023-03-09] [$4000] Opening search using the keyboard shortcut (ctrl+k) while options for adding payment method are visible, options overlap with search. #14889
Comments
kavimuru
added
Daily
|
Triggered auto assignment to @puneetlath ( |
Bug0 Triage Checklist (Main S/O)
|
puneetlath
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~0106757eb17dcdb76c |
|
Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
melvin-bot
bot
added
the
Help Wanted
|
Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new. |
ProposalRCAWhen a stack navigator route is opened, it does not unmount the previous stack navigator route. Instead, the previous route remains open. In this case, the Popover component of the previous route has a z-Index of SolutionsThere can be multiple solutions here depending on the testing effort we want to put into this.
diff --git a/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js b/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
index 9a480454ea..36a65d23a0 100644
--- a/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
+++ b/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
@@ -34,6 +34,7 @@ import OfflineWithFeedback from '../../../../components/OfflineWithFeedback';
import ConfirmContent from '../../../../components/ConfirmContent';
import Button from '../../../../components/Button';
import themeColors from '../../../../styles/themes/default';
+import KeyboardShortcut from '../../../../libs/KeyboardShortcut';
class BasePaymentsPage extends React.Component {
constructor(props) {
@@ -73,6 +74,18 @@ class BasePaymentsPage extends React.Component {
componentDidMount() {
this.fetchData();
+ const searchShortcutConfig = CONST.KEYBOARD_SHORTCUTS.SEARCH;
+ const groupShortcutConfig = CONST.KEYBOARD_SHORTCUTS.NEW_GROUP;
+
+ // Listen for the key K being pressed so that focus can be given to
+ // the chat switcher, or new group chat
+ // based on the key modifiers pressed and the operating system
+ this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(searchShortcutConfig.shortcutKey, () => {
+ this.hideAddPaymentMenu();
+ }, searchShortcutConfig.descriptionKey, searchShortcutConfig.modifiers, true, true, undefined, false);
+ this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => {
+ this.hideAddPaymentMenu();
+ }, groupShortcutConfig.descriptionKey, groupShortcutConfig.modifiers, true, true, undefined, false);
}
componentDidUpdate(prevProps) {
@@ -95,6 +108,15 @@ class BasePaymentsPage extends React.Component {
this.fetchData();
}
+ componentWillUnmount() {
+ if (this.unsubscribeSearchShortcut) {
+ this.unsubscribeSearchShortcut();
+ }
+ if (this.unsubscribeGroupShortcut) {
+ this.unsubscribeGroupShortcut();
+ }
+ }
+
setShouldShowLoadingSpinner() {
// In order to prevent a loop, only update state of the spinner if there is a change
const shouldShowLoadingSpinner = this.props.isLoadingPaymentMethods || false;
ResultScreen.Recording.2023-02-07.at.5.44.36.AM.mov |
ProposalProblemThis issue occurs for all the modals i.e. all modals stay open when any keyboard shortcuts are triggered. SolutionWe should close all the modals/popovers before opening any new modals or navigating to new routes by calling diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js
index c45ced82f..1f4c39b42 100644
--- a/src/components/KeyboardShortcutsModal.js
+++ b/src/components/KeyboardShortcutsModal.js
@@ -15,6 +15,7 @@ import compose from '../libs/compose';
import KeyboardShortcut from '../libs/KeyboardShortcut';
import * as KeyboardShortcutsActions from '../libs/actions/KeyboardShortcuts';
import ONYXKEYS from '../ONYXKEYS';
+import Navigation from '../libs/Navigation/Navigation';
const propTypes = {
/** prop to set shortcuts modal visibility */
@@ -35,6 +36,7 @@ class KeyboardShortcutsModal extends React.Component {
componentDidMount() {
const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL;
this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, () => {
+ Navigation.dismissModal();
KeyboardShortcutsActions.showKeyboardShortcutModal();
}, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true);
}
diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js
index 58fae5f8b..f32694d08 100644
--- a/src/libs/Navigation/AppNavigator/AuthScreens.js
+++ b/src/libs/Navigation/AppNavigator/AuthScreens.js
@@ -113,9 +113,11 @@ class AuthScreens extends React.Component {
// the chat switcher, or new group chat
// based on the key modifiers pressed and the operating system
this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(searchShortcutConfig.shortcutKey, () => {
+ Navigation.dismissModal();
Navigation.navigate(ROUTES.SEARCH);
}, searchShortcutConfig.descriptionKey, searchShortcutConfig.modifiers, true);
this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => {
+ Navigation.dismissModal();
Navigation.navigate(ROUTES.NEW_GROUP);
}, groupShortcutConfig.descriptionKey, groupShortcutConfig.modifiers, true);
}
|
|
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
ProposalYes, it is indeed happening to all modal, except for FAB. We already have a fix only for FAB and we can use that by moving the part of the logic to the modal component instead. The logic that we need move to the modal component is when the screen focus is changed, we then close the modal. We can't move the drawer state logic from FAB because not all modal is inside a drawer navigator, so that logic is still there on the FAB component. diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 1b58579f93..c3bf01e25e 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -10,9 +10,13 @@ import {propTypes as modalPropTypes, defaultProps as modalDefaultProps} from './
import * as Modal from '../../libs/actions/Modal';
import getModalStyles from '../../styles/getModalStyles';
import variables from '../../styles/variables';
+import compose from '../../libs/compose';
+import withNavigationFocus, {withNavigationFocusPropTypes} from '../withNavigationFocus';
+import withNavigationFallback from '../withNavigationFallback';
const propTypes = {
...modalPropTypes,
+ ...withNavigationFocusPropTypes,
/** The ref to the modal container */
forwardedRef: PropTypes.func,
@@ -31,6 +35,13 @@ class BaseModal extends PureComponent {
}
componentDidUpdate(prevProps) {
+ if (this.didScreenBecomeInactive(prevProps)) {
+ // Hide popover manually when other pages are opened using shortcut key
+ if (this.props.isVisible) {
+ this.props.onClose();
+ }
+ }
+
if (prevProps.isVisible === this.props.isVisible) {
return;
}
@@ -43,6 +54,22 @@ class BaseModal extends PureComponent {
this.hideModal(this.props.isVisible);
}
+ /**
+ * Check if page status changed from active to inactive.
+ * Used to close already opened popover when open any other pages (i.e. Press Command + K on web).
+ *
+ * @param {Object} prevProps
+ * @return {Boolean}
+ */
+ didScreenBecomeInactive(prevProps) {
+ // When any other page is opened over the page
+ if (!this.props.isFocused && prevProps.isFocused) {
+ return true;
+ }
+
+ return false;
+ }
+
/**
* Hides modal
* @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback
@@ -166,7 +193,10 @@ class BaseModal extends PureComponent {
BaseModal.propTypes = propTypes;
BaseModal.defaultProps = defaultProps;
-export default React.forwardRef((props, ref) => (
+export default compose(
+ withNavigationFallback,
+ withNavigationFocus,
+)(React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
<BaseModal {...props} forwardedRef={ref} />
-));
+)));
diff --git a/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js b/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
index 6fe0447ef4..c999e68275 100644
--- a/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
+++ b/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
@@ -99,11 +99,6 @@ class FloatingActionButtonAndPopover extends React.Component {
return true;
}
- // When any other page is opened over LHN
- if (!this.props.isFocused && prevProps.isFocused) {
- return true;
- }
-
return false;
}
However, it does not cover the case for keyboard shortcut modal. When open the keyboard shortcut from Settings > About and we press ctrl+k for example, the keyboard shortcut modal will not be closed because the screen focus where the keyboard shortcut modal resides does not change. That is because When we open Settings > About, the sidebar screen App/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js Lines 50 to 69 in 26fdace diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js
index 58fae5f8b4..bf44befe49 100644
--- a/src/libs/Navigation/AppNavigator/AuthScreens.js
+++ b/src/libs/Navigation/AppNavigator/AuthScreens.js
@@ -30,6 +30,7 @@ import defaultScreenOptions from './defaultScreenOptions';
import * as App from '../../actions/App';
import * as Download from '../../actions/Download';
import * as Session from '../../actions/Session';
+import * as KeyboardShortcutsActions from '../../actions/KeyboardShortcuts';
let currentUserEmail;
Onyx.connect({
@@ -114,9 +115,11 @@ class AuthScreens extends React.Component {
// based on the key modifiers pressed and the operating system
this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(searchShortcutConfig.shortcutKey, () => {
Navigation.navigate(ROUTES.SEARCH);
+ KeyboardShortcutsActions.hideKeyboardShortcutModal();
}, searchShortcutConfig.descriptionKey, searchShortcutConfig.modifiers, true);
this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => {
Navigation.navigate(ROUTES.NEW_GROUP);
+ KeyboardShortcutsActions.hideKeyboardShortcutModal();
}, groupShortcutConfig.descriptionKey, groupShortcutConfig.modifiers, true);
}
Result Screen.Recording.2023-02-06.at.19.31.36.mov |
ProposalProblemWhen a popover menus are opened, triggering the search with keyboard shortcuts keep them opened. SolutionAttach Keyboard listeners to close the popover menu once the keyboard is triggered.
Then a method to remove the listeners: ScreenScreen.Recording.2023-02-07.at.05.25.07.1.mov |
ProposalProblem: Root Cause: In the AuthScreens.js file, the code is missing to dismiss the previously opened modal in the keyboard shortcut listeners.
Solution: Navigation.dismissModal() needs to add on below place in AuthScreens.js file, in order to close old modal when new modal arise.
|
|
reviewing proposals today |
ProposalPlease re-state the problem that we are trying to solve in this issue.Press Ctrl+K on keyboard when any popover modal shows. Search page opens but it's behind popover and not active until popover is closed manually. What is the root cause of that problem?There are 3 keyboard shortcuts to open new page: Search, New group, Shortcut modal. What changes do you think we should make in order to solve the problem?The solution is to close any popover open which prevents new page being being active. 1. Close any popover when Search page (Ctrl+K) or New group page (Ctrl+Shift+K) is opening App/src/components/PopoverMenu/index.js Lines 59 to 75 in 39d9b66 We can use similar approach for NEW_GROUP, SEARCH shortcuts. So in subscribe callback, just call this.props.onClose() to close modal.
2. Close any popver when Shortcut modal is opening App/src/components/PopoverMenu/index.js Lines 34 to 44 in 39d9b66 We can catch shortcut modal opened state in componentDidUpdate of PopoverMenu and call this.props.onClose() to close popover.
3. Close shortcut modal when Search page or New group page is opening App/src/libs/Navigation/AppNavigator/AuthScreens.js Lines 116 to 121 in 39d9b66 Just call KeyboardShortcutsActions.hideKeyboarShortcutModal() in subscribe callbacks.
Demo: demo.mov |
|
@aimane-chnaif any updates/feedback on the proposals above? |
|
No acceptable proposal found yet for general solution. I will answer to each proposal today |
melvin-bot
bot
changed the title
|
@aimane-chnaif why do you think adding a new ONYK key is not a good/better solution when we already use ONYX keys even for something as simple as displaying a particular modal e.g. Keyboard shortcuts modal |
|
Using onyx value to get modal open/closed status is not recommended at all. bug.mov2 issues in this video:
|
|
@situchan yes that looks like a weird bug, but I still don't understand what does that have to do with using onyx or not? Are you saying that using onyx is the cause of this weird issue? |
Updated ProposalPlease re-state the problem that we are trying to solve in this issue.Avoid the popovers & modals from being displayed over the most recently opened modal. What is the root cause of that problem?The root cause of the problem is that we don't dismiss those popovers & modals before opening the new modal thus it appears over it. What changes do you think we should make in order to solve the problem?If we are really concerned about adding new onyx keys then we can can use the lib actions approach where we keep track of BaseModal's & Popover's @aimane-chnaif This proposal doesn't close the context menu, it still appears over the side panels. |
@priyeshshah11 If we introduce new onyx key for every modal, I am curious what will be key and what will be value?
This should be in
What do you mean? Can you share video? Also, don't post proposal similar to previous ones. |
|
@aimane-chnaif I got your point about the shortcuts modal open key, thanks for explaining.
The new key can be called isClosableModalOrPopoverOpen or some better word than close, the value should simply be a boolean. And yes, you're correct it should be applied to both BaseModal & Popover as per my latest proposal.
I am talking about this
With the new approach a lot of proposals are going to be similar with some differences, so what do you want me to do when I see issues/improvements with current proposals? we're definitely not supposed to edit proposals once posted. |
It's working well for me. I already tested this case as well before posing proposal. |
|
I think @situchan's proposal sounds good too. Let's do it. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @situchan You have been assigned to this job by @puneetlath! |
|
@puneetlath, @aimane-chnaif, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick! |
|
Not overdue. Waiting for PR to hit production |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.77-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-03-09. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
@situchan @Harshdeepjoshi can you please apply to the Upwork job? https://www.upwork.com/jobs/~0106757eb17dcdb76c @aimane-chnaif can you please help with the checklist? Thanks! |
|
I don't think any PR caused regression. |
|
@Harshdeepjoshi @situchan sent you hiring offers! @aimane-chnaif paid! |
|
All paid. Thanks everyone! |
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:
Expected Result:
options for adding payment method should not appear (Overlap) on search page
Actual Result:
options for adding payment method appear (Overlap) on search page
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.66-0
Reproducible in staging?: y
Reproducible in production?: y
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:
2023-02-07.00-39-17.mp4
Recording.1457.mp4
Expensify/Expensify Issue URL:
Issue reported by: @Harshdeepjoshi
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675711618695179
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: