[$250] mWeb - category-Long pressing&selecting category, page moves

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.48-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Go to profile -- workspaces -- workspace
3. Tap categories
4. Scroll down to middle in the category list
5. Long press and select it

Expected Result:

Long pressing and selecting category, page must not move

Actual Result:

Long pressing and selecting category, page moves

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/402d3dd6-9d96-447f-808a-170462a283b7

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021846158108420275296
• Upwork Job ID: 1846158108420275296
• Last Price Increase: 2024-11-12

Issue Owner

Current Issue Owner: @eVoloshchak

[melvin-bot[bot]]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[QichenZhu]

Edited by proposal-police: This proposal was edited at 2024-10-12 06:15:15 UTC.

Proposal

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

Long pressing and selecting an item causes the list to scroll.

What is the root cause of that problem?

We had a similar issue before (https://github.com/Expensify/App/issues/49537), where long pressing an item caused the list to scroll. The fix was to ignore the focus event triggered by touch.

https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/components/SelectionList/index.tsx#L48

https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/components/SelectionList/BaseSelectionList.tsx#L472-L475

Now, after long pressing, the focus event is ignored correctly. But when the context menu closes, focus returns to the item, and this time it can’t be ignored since it’s triggered programmatically by focus-trap, not by touch.

https://github.com/focus-trap/focus-trap/blob/6c757fa2a90bb818b7f702ba4129d14cdc733f7e/index.js#L769

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

We should ignore focus events triggered programmatically, similar to what was done in the previous issue.

import * as Browser from '@libs/Browser';

onFocus={(event) => {

https://github.com/Expensify/App/blob/7256ad6eef046be3345e0b260252f6ec47fc4203/src/components/SelectionList/BaseSelectionListItemRenderer.tsx#L77-L79

    // Insert this after the above code.
    if (Browser.isMobileChrome() && event && event.nativeEvent && !event.nativeEvent.sourceCapabilities) {
        return;
    }

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot[bot]]

@bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~021846158108420275296

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[mkzie2]

Proposal

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

Long pressing and selecting category, page moves

What is the root cause of that problem?

In mWeb, we ignore the focus here if we press long on an item. But the problem here is after the select modal is closed the item is focused again by FocusTrap and isScreenTouched is false because it's triggered programmatically.

https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/components/SelectionList/index.tsx#L48

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

We should deactivate the focus trap on the categories page by adding focusTrapSettings prop to ScreenWrapper

focusTrapSettings={{active: false}}

https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L348

We can check all the places that we use SelectionListWithModal and do the same approach

What alternative solutions did you explore? (Optional)

NA

[melvin-bot[bot]]

@eVoloshchak, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[eVoloshchak]

@QichenZhu, when testing your proposal I noticed that the page is instead scrolled the moment you long press on the item. Could you double-check please?

https://github.com/user-attachments/assets/2b85887e-9dfa-4ed8-9e25-54296bab3858

[eVoloshchak]

@mkzie2, the approach you're proposing does fix the issue, however, it makes it so the long press isn't registered sometimes. It's not 100% reproducible, but I managed to catch it in the video below

https://github.com/user-attachments/assets/0ea493ba-556c-4c21-b009-3d3b4b1cfcf2

[QichenZhu]

@eVoloshchak Thanks for testing. Could you confirm if you tested with the diff below? I intended to append an if clause, not replace the existing one.

+ import * as Browser from '@libs/Browser';

- onFocus={() => {
+ onFocus={(event) => {
    if (shouldIgnoreFocus || isDisabled) { 
        return; 
    } 
+ 
+     if (Browser.isMobileChrome() && event && event.nativeEvent && !event.nativeEvent.sourceCapabilities) {
+         return;
+     }

[eVoloshchak]

@QichenZhu, thank you for that correction! It works now, but has the same bug I've described in this comment , the long press doesn't register sometimes

https://github.com/user-attachments/assets/d85446cc-1a9a-44af-98af-36d353c592cd

[QichenZhu]

the long press doesn't register sometimes

It looks like the long press delay is a bit long. Is that what you mean? It usually needs 500 ms to trigger. We can adjust it with delayLongPress. Should we check with the design team?

[melvin-bot[bot]]

@eVoloshchak @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[eVoloshchak]

It looks like the long press delay is a bit long. Is that what you mean?

No, the problem I'm encountering is long press not registering sometimes. The menu item is highlighted as usual, but the context menu doesn't open. Take a look at the video above at 00:10 timestamp

[QichenZhu]

@eVoloshchak I can't reproduce it on my end. As you can reproduce it with both proposals, could it also be reproducible with the main branch, considering that the two proposals are quite different?

[eVoloshchak]

@QichenZhu, it's not reproducible on the main branch. This might be an emulator issue (I've tested staging on both physical and emulator devices, but can test proposals only in the emulator). @mkzie2, could you please check if you can reproduce this issue on your end?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[QichenZhu]

@eVoloshchak After more testing, I can get similar results in two ways:

1. Moving the mouse while pressing. If you move the mouse a certain distance, the highlight remains, but the long press is canceled. However, if you move it only a little, the long press is still triggered.

https://github.com/user-attachments/assets/1fe0de22-3a35-41c6-bc65-a01ef4822aec

2. Releasing the mouse a bit early. The timing is subtle. It should be longer than a click but shorter than a long press. I think this behavior is expected.

https://github.com/user-attachments/assets/01255c67-7c55-4085-a8c8-a5247918b1cb

Also, it would be helpful to know more details about your environment, like if you're using a remote desktop setup, and whether your pointer device is a mouse, trackpad, or trackball, etc.

BTW, I noticed the mouse cursor blinks in your video. Not sure if this is related.

[melvin-bot[bot]]

@eVoloshchak, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[wildan-m]

Proposal

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

In mWeb > category Long pressing and selecting category cause unwanted page moves

What is the root cause of that problem?

When we long press on the chat item, the item receives the focus event. When the item receives the focus, it will update the arrow key focus index to the item index which will scroll the list to the focused item/index. This focus trap also triggers when we hide the selection modal after long pressing an item.

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

In src/components/SelectionList/BaseSelectionListItemRenderer.tsx we can store long pressed item

        const longPressedItem = useRef<TItem | null>(null);

......

                <ListItem
                    item={item}
                    isFocused={isItemFocused}
                    isDisabled={isDisabled}
                    showTooltip={showTooltip}
                    canSelectMultiple={canSelectMultiple}
                    onLongPressRow={(item: TItem) => {
                        longPressedItem.current = item;
                        onLongPressRow?.(item);
                    }}                    

Return early from onFocus when the long pressed item key matches the item to be focused. Since this only happens in mobile chrome, it's safe to limit the condition to it.

                    onFocus={() => {
                   const isFocusedItemLongPressed = longPressedItem.current?.keyForList && longPressedItem.current?.keyForList === item.keyForList;
                        // clear after use
                        longPressedItem.current = null;

                        if (shouldIgnoreFocus || isDisabled || (isFocusedItemLongPressed && Browser.isMobileChrome())) {
                            return;
                        }
                        setFocusedIndex(normalizedIndex);

Branch for this solution

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@eVoloshchak, @bfitzexpensify Still overdue 6 days?! Let's take care of this!

[melvin-bot[bot]]

@eVoloshchak, @bfitzexpensify 12 days overdue. Walking. Toward. The. Light...

[melvin-bot[bot]]

@eVoloshchak, @bfitzexpensify 12 days overdue now... This issue's end is nigh!

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[eVoloshchak]

@QichenZhu, I cannot reproduce this issue anymore. I suspect this might be simulator-related

I think we need to dig a bit deeper on this one to figure out the correct RCA. @mkzie2, @QichenZhu, @wildan-m, do you have any theories on why this issue is not reproducible on Search page where we also use BaseSelectionList?

https://github.com/user-attachments/assets/4143f490-7929-4840-8e46-a88d3e86731f

[QichenZhu]

Proposal

Updated to align with main branch changes

[QichenZhu]

do you have any theories on why this issue is not reproducible on Search page where we also use BaseSelectionList

@eVoloshchak The search screen is not a modal, so its focus-trap is inactive. The categories settings is a modal and has an active focus-trap.

[wildan-m]

Proposal Updated

• Update to align new codes
• Add empty check

@eVoloshchak I can consistently reproduce it. Try to chose lowest items

https://github.com/user-attachments/assets/f63b8f59-3d23-4331-97cc-c67668c64c4a