fix: prevent parent view scroll when press space bar and arrow #56435

linhvovan29546 · 2025-02-06T00:23:35Z

Explanation of Change

Fixed Issues

$ #55832
PROPOSAL: #55832 (comment)

Tests

Precond: User has a long Workspace list

Access staging.new.expensify.com
Sign into a valid account
Go to Settings > Workspace (WS) > Tap on 3 dot menu on a WS
With the popup open, tap on "Space Bar" on keyboard
Verify that the Workspace list does not scroll down

Verify that no errors appear in the JS console

Precond: User has a long list

Access staging.new.expensify.com
Sign into a valid account
Go to Settings > Workspace (WS) > Categories > Tap on category item
With the RHN open, tap on "Space Bar" on keyboard
Verify that the Categories list does not scroll down
We can apply this test to other lists as well

Offline tests

Same Test above

QA Steps

Same Test above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

This change only apply for web and desktop

Android: mWeb Chrome

Screen.Recording.2025-02-10.at.8.52.20.PM.mov

iOS: Native

This change only apply for web and desktop

iOS: mWeb Safari

Screen.Recording.2025-02-10.at.7.14.19.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2025-02-06.at.7.41.49.AM.mov

Screen.Recording.2025-02-06.at.7.44.16.AM.mov

Screen.Recording.2025-02-06.at.7.44.51.AM.mov

MacOS: Desktop

Screen.Recording.2025-02-06.at.7.48.23.AM.mov

melvin-bot · 2025-02-06T00:49:48Z

@DylanDylann 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]

src/components/PopoverMenu.tsx

DylanDylann · 2025-02-10T08:02:43Z

src/CONST.ts

+            shortcutKey: 'Space',
+            modifiers: [],
+            trigger: {
+                DEFAULT: {input: ' '},


Do we need to add some options for IOS/MACOS platform?

Please use constant like keyInputLeftArrow, keyInputRightArrow instead of hardcode

I see that the input value is the same for iOS/macOS, so I don't think it's necessary to add it.

Same answer as below: KeyCommand does not have a space input. Can I update the name to keyInputSpace = ' ' instead?

I see that the input value is the same for iOS/macOS

@linhvovan29546 Could you give an reference to this point?

I don't have a reference, but I've tested pressing space, and the useKeyboardShortcut returns a callback

src/components/PopoverMenu.tsx

DylanDylann · 2025-02-10T08:05:40Z

src/libs/KeyboardShortcut/index.ts

@@ -42,7 +42,7 @@ const keyInputUpArrow = KeyCommand?.constants?.keyInputUpArrow?.toString() ?? 'k
 const keyInputDownArrow = KeyCommand?.constants?.keyInputDownArrow?.toString() ?? 'keyInputDownArrow';
 const keyInputLeftArrow = KeyCommand?.constants?.keyInputLeftArrow?.toString() ?? 'keyInputLeftArrow';
 const keyInputRightArrow = KeyCommand?.constants?.keyInputRightArrow?.toString() ?? 'keyInputRightArrow';
-
+const keySpace = ' ';


Please using constant as above definition

KeyCommand does not have a space input. Can I update the name to keyInputSpace = ' ' instead?

src/hooks/useScrollEnabled/index.ts

src/hooks/useScrollEnabled/index.web.ts

DylanDylann · 2025-02-10T08:15:26Z

src/components/PopoverMenu.tsx

+    const keyboardShortcutSpaceCallback = useCallback(() => {
+        if (!scrollViewRef.current) {
+            return;
+        }
+        const scrollableElement = scrollViewRef.current as HTMLDivElement;
+        // If the popover menu is scrollable, allow scrolling with the space bar instead of preventing it.
+        const clientHeight = scrollableElement.clientHeight;
+        const scrollTop = scrollableElement.scrollTop;
+        const scrollHeight = scrollableElement.scrollHeight;
+        const remainingScrollSpace = scrollHeight - clientHeight - scrollTop;
+
+        scrollableElement.scrollBy(0, Math.min(clientHeight, remainingScrollSpace));
+    }, []);


I think we only need e.preventDefault in this callback, right?

By default, we called e.preventDefault in KeyboardShortcut.subscribe

Why do we need to call scrollableElement.scrollBy

The PopoverMenu supports scrolling if shouldUseScrollView is true. However, preventing the space key will also prevent scrolling within the PopOverMenu. To avoid this, I handle scrolling manually instead. Wdyt?

@linhvovan29546 Could you help to point out a specific case that has shouldUseScrollView is true? I think it will be weird when there are 2 scrolling in the same time. Thus, to make it simpler, should we call e.preventDefault only when shouldUseScrollView is false?

If we call e.preventDefault only when shouldUseScrollView is false, the issue still exists. For example, see the case below. Note: I have set shouldUseScrollView to true for testing

Screen.Recording.2025-02-11.at.3.11.01.PM.mov

I have an idea based on useBottomTabIsFocused. I'll update it

linhvovan29546 · 2025-02-10T09:08:30Z

I've updated and replied to the review change.

DylanDylann · 2025-02-11T06:42:53Z

src/hooks/useScrollEnabled/index.ts

+const useScrollEnabled: UseScrollEnabled = () => {
+    const isFocused = useIsFocused();
+    const prevIsFocused = usePrevious(isFocused);
+    return !(prevIsFocused && !isFocused);


Suggested change

return !(prevIsFocused && !isFocused);

return !prevIsFocused || isFocused);

More readable

My code is not working when isFocused re-renders twice, causing !(prevIsFocused && !isFocused) to return true even when the screen is not focused. I'll revert to the previous code.

@DylanDylann I noticed that isFocused remains false when calling const isFocused = useIsFocused(); inside the bottom tab, which causes scrolling to be disabled. Do you have any ideas on how to fix this?

DylanDylann · 2025-02-11T06:45:31Z

src/components/PopoverMenu.tsx

+    const keyboardShortcutSpaceCallback = useCallback(() => {
+        if (!scrollViewRef.current) {
+            return;
+        }
+        const scrollableElement = scrollViewRef.current as HTMLDivElement;
+        // If the popover menu is scrollable, allow scrolling with the space bar instead of preventing it.
+        const clientHeight = scrollableElement.clientHeight;
+        const scrollTop = scrollableElement.scrollTop;
+        const scrollHeight = scrollableElement.scrollHeight;
+        const remainingScrollSpace = scrollHeight - clientHeight - scrollTop;
+
+        scrollableElement.scrollBy(0, Math.min(clientHeight, remainingScrollSpace));
+    }, []);


@linhvovan29546 Could you help to point out a specific case that has shouldUseScrollView is true? I think it will be weird when there are 2 scrolling in the same time. Thus, to make it simpler, should we call e.preventDefault only when shouldUseScrollView is false?

linhvovan29546 added 2 commits February 6, 2025 07:14

fix: prevent parent view scroll when press space bar and arrow

923ddd9

fix: eslint fail

99b5da9

linhvovan29546 changed the title ~~[WIP]fix: prevent parent view scroll when press space bar and arrow~~ fix: prevent parent view scroll when press space bar and arrow Feb 6, 2025

linhvovan29546 marked this pull request as ready for review February 6, 2025 00:49

linhvovan29546 requested a review from a team as a code owner February 6, 2025 00:49

melvin-bot bot requested a review from DylanDylann February 6, 2025 00:49

melvin-bot bot removed the request for review from a team February 6, 2025 00:49

linhvovan29546 commented Feb 6, 2025

View reviewed changes

src/components/PopoverMenu.tsx Show resolved Hide resolved

linhvovan29546 commented Feb 6, 2025

View reviewed changes

src/components/PopoverMenu.tsx Show resolved Hide resolved

DylanDylann reviewed Feb 10, 2025

View reviewed changes

linhvovan29546 added 2 commits February 10, 2025 15:26

fix: duplicate estlint

130e2b0

fix: apply scroll enable for specific platform

740d828

linhvovan29546 added 3 commits February 10, 2025 19:08

fix: rename variable, use const instead hard code

b804d17

fix: disabled scroll if prev focus is true

ead0e57

fix: eslint fail

bbcfd4e

DylanDylann reviewed Feb 11, 2025

View reviewed changes

fix: useScrollEnabled alway false in the bottom tab

3895981

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent parent view scroll when press space bar and arrow #56435

fix: prevent parent view scroll when press space bar and arrow #56435

linhvovan29546 commented Feb 6, 2025 •

edited

Loading

melvin-bot bot commented Feb 6, 2025

DylanDylann Feb 10, 2025

linhvovan29546 Feb 10, 2025

DylanDylann Feb 10, 2025

linhvovan29546 Feb 10, 2025

DylanDylann Feb 10, 2025

linhvovan29546 Feb 10, 2025

DylanDylann Feb 10, 2025

linhvovan29546 Feb 10, 2025

DylanDylann Feb 10, 2025

linhvovan29546 Feb 10, 2025

DylanDylann Feb 11, 2025

linhvovan29546 Feb 11, 2025

linhvovan29546 Feb 11, 2025

linhvovan29546 commented Feb 10, 2025

DylanDylann Feb 11, 2025

DylanDylann Feb 11, 2025

linhvovan29546 Feb 11, 2025

linhvovan29546 Feb 11, 2025

DylanDylann Feb 11, 2025

	return !(prevIsFocused && !isFocused);
	return !prevIsFocused \|\| isFocused);

fix: prevent parent view scroll when press space bar and arrow #56435

Are you sure you want to change the base?

fix: prevent parent view scroll when press space bar and arrow #56435

Conversation

linhvovan29546 commented Feb 6, 2025 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhvovan29546 commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhvovan29546 commented Feb 6, 2025 •

edited

Loading