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

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6631349_1728609479255.Screenrecorder-2024-10-11-06-43-07-484.mp4

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-19
• Automatic offers:
• QichenZhu | Contributor | 105066058

Issue Owner

Current Issue Owner: @eVoloshchak

[melvin-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 (#49537), where long pressing an item caused the list to scroll. The fix was to ignore the focus event triggered by touch.

App/src/components/SelectionList/index.tsx

Line 48 in e4969b2

shouldIgnoreFocus={Browser.isMobileChrome() && isScreenTouched}

App/src/components/SelectionList/BaseSelectionListItemRenderer.tsx

Lines 77 to 79 in 7256ad6

	if (shouldIgnoreFocus || isDisabled) {
	return;
	}

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) => {

App/src/components/SelectionList/BaseSelectionListItemRenderer.tsx

Lines 77 to 79 in 7256ad6

	if (shouldIgnoreFocus || isDisabled) {
	return;
	}

    // 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]

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

[melvin-bot]

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

[melvin-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.

App/src/components/SelectionList/index.tsx

Line 48 in e4969b2

shouldIgnoreFocus={Browser.isMobileChrome() && isScreenTouched}

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}}

App/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

Line 348 in e4969b2

<ScreenWrapper

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]

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

[melvin-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?

Screen.Recording.2024-10-22.at.19.10.06.mov

[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

Screen.Recording.2024-10-22.at.19.26.36.mov

[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

Screen.Recording.2024-10-24.at.16.22.57.mov

[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]

@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?

[melvin-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?

Screen.Recording.2024-11-12.at.21.53.56.mov

[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

Kapture.2024-11-13.at.10.07.04.mp4

[wildan-m]

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

@eVoloshchak I believe the issue is due to the search page being under Tab.Screen, causing different focus behavior compared to other screens. For instance based on this investigation #52202 (comment), isFocused will always false under Tab.Screen, so the focus prop was passed to the selection list. #52278

[melvin-bot]

@eVoloshchak, @bfitzexpensify Huh... This is 4 days overdue. Who can take care of this?

[melvin-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]

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

[bfitzexpensify]

Bump on this one @eVoloshchak - thank you!

[melvin-bot]

@eVoloshchak, @bfitzexpensify 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[eVoloshchak]

I think we should proceed with @QichenZhu's proposal

🎀👀🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @nkuoch, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @QichenZhu 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

This issue has not been updated in over 15 days. @nkuoch, @eVoloshchak, @bfitzexpensify, @QichenZhu eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!