[$1000] Web - Bug in Keyboard Shortcuts Modal opening/closing behaviour

[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. Open ND
2. Press CTRL + J to open the Keyboard Shortcuts Modal
3. Press CTRL + J

Expected Result:

Keyboard Shortcuts Modal should not close on pressing CTRL + J after opening Keyboard Shortcuts Modal

Actual Result:

Keyboard Shortcuts Modal closes on pressing CTRL + J after opening Keyboard Shortcuts Modal

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.45-7
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: Any additional supporting documentation

Screen.Recording.2023-07-26.at.8.07.07.PM.mov

Recording.3930.mp4

Expensify/Expensify Issue URL:
Issue reported by: @shubham1206agra
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690382150018799

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01caad76786cb2948d
• Upwork Job ID: 1684656116445630464
• Last Price Increase: 2023-07-27

[melvin-bot]

Triggered auto assignment to @maddylewis (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[samh-nl]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bug in Keyboard Shortcuts Modal opening/closing behaviour

What is the root cause of that problem?

In the shortcut listener, isShortcutsModalOpen is always false as this is an async event and so it points to the initial value and not the actual one. This leads to ModalActions.close(); being called, which results in the shortcuts modal being closed if it was previously open.

App/src/components/KeyboardShortcutsModal.js

Lines 113 to 126 in 0d876a4

	const unsubscribeShortcutModal = KeyboardShortcut.subscribe(
	openShortcutModalConfig.shortcutKey,
	() => {
	if (isShortcutsModalOpen) {
	return;
	}
	ModalActions.close();
	KeyboardShortcutsActions.showKeyboardShortcutModal();
	},
	openShortcutModalConfig.descriptionKey,
	openShortcutModalConfig.modifiers,
	true,
	);

However, the KeyboardShortcutsActions.showKeyboardShortcutModal(); does not re-open it, as this function also checks if isShortcutsModalOpen is set. The difference is that this uses a synced copy retrieved using Onyx.connect, therefore leading to a return statement.

App/src/libs/actions/KeyboardShortcuts.js

Lines 14 to 19 in 0d876a4

	function showKeyboardShortcutModal() {
	if (isShortcutsModalOpen) {
	return;
	}
	Onyx.set(ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN, true);
	}

What changes do you think we should make in order to solve the problem?

We can replace the callback inside the listener with just KeyboardShortcutsActions.showKeyboardShortcutModal();.

And then inside the KeyboardShortcutsActions.showKeyboardShortcutModal(); function, we add the ModalActions.close();

The reason this will work is because isShortcutsModalOpen inside this function is retrieved using Onyx.connect and thus uses the correct value.

What alternative solutions did you explore? (Optional)

1. Adding isShortcutsModalOpen add as a dependency in useEffect, however this may be a less elegant solution as it adds complexity compared to my main proposal, which reduces duplicate code and simplifies it overall.
2. Create a ref using useRef that we sync with isShortcutsModalOpen, to ensure that the value is correct. This is also less appealing due to the increase in complexity.

[c3024]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing CTRL+J when the keyboard shortcuts modal open closes the keyboard shortcuts modal.

What is the root cause of that problem?

This here has empty dependency array and the change of isShortcutModalOpen is not reflected here so the early return statement never gets triggered and only closes the modal with ModalActions.close();

App/src/components/KeyboardShortcutsModal.js

Lines 112 to 141 in 82636ee

	useEffect(() => {
	const unsubscribeShortcutModal = KeyboardShortcut.subscribe(
	openShortcutModalConfig.shortcutKey,
	() => {
	if (isShortcutsModalOpen) {
	return;
	}
	ModalActions.close();
	KeyboardShortcutsActions.showKeyboardShortcutModal();
	},
	openShortcutModalConfig.descriptionKey,
	openShortcutModalConfig.modifiers,
	true,
	);
	if (isShortcutsModalOpen) {
	// The modal started open, which can happen if you reload the page when the modal is open.
	subscribeOpenModalShortcuts();
	}
	return () => {
	if (unsubscribeShortcutModal) {
	unsubscribeShortcutModal();
	}
	unsubscribeOpenModalShortcuts();
	};
	// We only want this to run on mount
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

What changes do you think we should make in order to solve the problem?

Add isShortcutsModalOpen to the dependency array in the useEffect above. We can combine it with the next useEffect as it also has the same dependency for DRY.

What alternative solutions did you explore? (Optional)

Use useRef and keep it updated with Onyx data for isShortcutsModalOpen because the useEffect after adding isShortcutsModalOpen to dependency array sets up subscriptions and unsubscriptions frequently on change of isShortcutsModalOpen . But the simple solution of adding to useEffect is preferable because this modal is for shortcut keys and this modal is not repeatedly opened and closed in real world cases.

[shubham1206agra]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Bug in Keyboard Shortcuts Modal opening/closing behaviour

What is the root cause of that problem?

useEffect has empty dependency array and the change of isShortcutModalOpen is not reflected here so the early return statement never gets triggered and only closes the modal with ModalActions.close();.

App/src/components/KeyboardShortcutsModal.js

Lines 113 to 126 in 0d876a4

	const unsubscribeShortcutModal = KeyboardShortcut.subscribe(
	openShortcutModalConfig.shortcutKey,
	() => {
	if (isShortcutsModalOpen) {
	return;
	}
	ModalActions.close();
	KeyboardShortcutsActions.showKeyboardShortcutModal();
	},
	openShortcutModalConfig.descriptionKey,
	openShortcutModalConfig.modifiers,
	true,
	);

However, the KeyboardShortcutsActions.showKeyboardShortcutModal(); does not re-open it, as this function also checks if isShortcutsModalOpen is set. The difference is that this uses a retrieved copy using Onyx.connect which is not syncing correctly, therefore leading to a return statement.

App/src/libs/actions/KeyboardShortcuts.js

Lines 14 to 19 in 0d876a4

	function showKeyboardShortcutModal() {
	if (isShortcutsModalOpen) {
	return;
	}
	Onyx.set(ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN, true);
	}

Another problem
On refresh, while opening this modal, the value inside Onyx is different from value reflected in KeyboardShortcutsModal component. Non persistence of KeyboardShortcutsModal is intentional. But the workaround is not very good.
https://expensify.slack.com/archives/C01GTK53T8Q/p1683563738386549?thread_ts=1683551147.132719&cid=C01GTK53T8Q

What changes do you think we should make in order to solve the problem?

I think we should not use onyx to store visibility state as this is not persistable modal, and so we should use state variable inside KeyboardShortcutsModal component.

Since KeyboardShortcutsModal is functional component, we can expose the methods to update the variables using useImperativeHandle hook, and we can use the function by getting ref from Expensify component.

We can update the KeyboardShortcutsActions similar to src/libs/Growl.js to call update functions one-at-a-time so that any other issues will not occur.

To stop running useEffect in which CTRL+J shortcut is subscribed every time visibility changes, we can take the function from KeyboardShortcutsModal component to Expensify component.

Since #23696 (comment) is intentional, we should update language here

App/src/languages/en.js

Line 1350 in ac74ab9

openShortcutDialog: 'Opens the keyboard shortcuts dialog',

and

expose another function named toggleShortcutModalConfig to toggle shortcuts modal.

What alternative solutions did you explore? (Optional)

NA

[aimane-chnaif]

This is not a bug - #15413 (comment)

[samh-nl]

@aimane-chnaif Thanks for finding that. If we want to keep this behavior then I think we need to implement it properly. Currently, when looking at it with a fresh view, it seems to be a positive side effect from a bug.

[shubham1206agra]

This is not a bug - #15413 (comment)

Oh
Then I think we need to update the language here

App/src/languages/en.js

Line 1350 in ac74ab9

openShortcutDialog: 'Opens the keyboard shortcuts dialog',

[samh-nl]

Yes and also change openShortcutModalConfig to for example toggleShortcutModalConfig

[shubham1206agra]

Yes and also change openShortcutModalConfig to for example toggleShortcutModalConfig

I think to keep both, don't know when we need them