Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.3k stars 2.74k forks source link

[$250] Settings - The timezone list returns to the top of the list when holding down Tab #44245

Open izarutskaya opened 2 months ago

izarutskaya commented 2 months ago

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: v9.0.1-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): crowdtest.expensify+test002@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-internal team

Action Performed:

  1. Open the app
  2. Go to Account Settings > Profile
  3. Open the Timezone > Open time zone list
  4. From the first item (Tap on the Tab button, and the time zone will scroll to the top)
  5. Keep long pressing a Tab key (~15s)
  6. Tap on the Tab button for more time

Expected Result:

The list will run gradually to the end of the list.

Actual Result:

The timezone list returns to the top of the list when holding down the Tab. Observation can be seen that the list does not run to the end but gradually moves upward when holding the Tab button for a period of time.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/d1b0e8e1-0ad4-47b3-95ec-f8aa2981d5c5

https://github.com/Expensify/App/assets/115492554/4738993c-4761-42bc-8608-3803f2db43e7

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0198bd49e44880048d
  • Upwork Job ID: 1805660378353287866
  • Last Price Increase: 2024-07-23
  • Automatic offers:
    • QichenZhu | Contributor | 103311257
Issue OwnerCurrent Issue Owner: @QichenZhu
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @lschurr (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.

devguest07 commented 2 months ago

Proposal

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

The timezone list returns to the top of the list when holding down Tab

What is the root cause of that problem?

When the Tab key is pressed repeatedly or held down, numerous JavaScript calls are triggered for the Tab listener action. In the browser, these JavaScript calls are handled by the event loop and callback queue asynchronously. Due to the high volume of JavaScript calls, the browser cannot render each Tab action sequentially. As a result, we may not see the focus move smoothly through each item; instead, the focus might jump ahead by several items. For example, if the Tab key is pressed 100 times rapidly, the focus might appear to jump from item 1 directly to item 10, which is the effect observed by the QA team.

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

To address the issue of the Tab key causing the focus to jump when pressed rapidly, we can implement debouncing. Debouncing ensures that a function is not called too frequently within a short period of time. We can debounce the onFocusedIndexChange callback to limit the number of times the function is executed.

https://github.com/Expensify/App/blob/d19a574fbf1f81d03274c84e5a8460001100963c/src/hooks/useArrowKeyFocusManager.ts#L76

// Debounce the onFocusedIndexChange callback 
const debouncedOnFocusedIndexChange = useCallback(debounce(onFocusedIndexChange, 100), [onFocusedIndexChange]);

What alternative solutions did you explore?

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

lschurr commented 2 months ago

@allroundexperts Can you review the proposal here?

allroundexperts commented 2 months ago

Thanks for your proposal @devguest07. I don't think debouncing is the proper solution here. Your RCA seems to be unconvincing to me. Try pressing tab on GH for example. It would switch focus smoothly.

allroundexperts commented 2 months ago

Still looking for better proposals.

melvin-bot[bot] commented 2 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

lschurr commented 2 months ago

Still waiting on proposals

lschurr commented 2 months ago

Same as above.

melvin-bot[bot] commented 2 months ago

@allroundexperts @lschurr 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!

drminh2807 commented 2 months ago

Proposal

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

The timezone list returns to the top of the list when holding down Tab

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

melvin-bot[bot] commented 2 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

lschurr commented 1 month ago

Looks like we have a new proposal here @allroundexperts

lschurr commented 1 month ago

Bump on this @allroundexperts

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

lschurr commented 1 month ago

Bumped in Slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1721243256819679

allroundexperts commented 1 month ago

@drminh2807 Thanks for your proposal. I think that disabling the focus is not fixing the issue at its root. Are we following a similar pattern elsewhere?

lschurr commented 1 month ago

Bump on this @drminh2807

melvin-bot[bot] commented 1 month ago

@allroundexperts @lschurr this issue is now 4 weeks old, please consider:

Thanks!

allroundexperts commented 1 month ago

Still looking for proposals.

QichenZhu commented 1 month ago

Proposal

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

The timezone list often focuses on the text input after holding the Tab key for a long while.

What is the root cause of that problem?

The text input in the base selection list is set to select its text on focus.

https://github.com/Expensify/App/blob/17a8bbd8e9a5730f7979ba816d6d373d1787bc8f/src/components/SelectionList/BaseSelectionList.tsx#L689

However, the text selection is executed after a timeout, so the text input is selected and re-focused regardless of where the focus is, since the timeout has lower priority than other synchronous code and micro-tasks.

https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/TextInput/index.js#L287-L297

Screenshot 2024-07-24 at 2 38 27 AM

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

Consider setting the selectTextOnFocus attribute of the TextInput in BaseSelectionList to false on web and desktop platforms.

What alternative solutions did you explore? (Optional)

Another option is to check if the text input is still focused before selecting the text in the setTimeout function.

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

lschurr commented 1 month ago

@allroundexperts - Will you check out the new proposal?

lschurr commented 1 month ago

Bump on this one @allroundexperts

allroundexperts commented 1 month ago

@QichenZhu Thanks for your proposal. I think your RCA is correct and the solutions make sense as well.

react-native-web is actively maintained. I'd much prefer to have your secondary solution applied to react-native-web package since it seems like a genuine bug. Can you raise an issue in that repository and raise a fix there?

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

πŸ“£ @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 πŸ“–

luacmartins commented 1 month ago

I agree that fixing it in react-native-web would be ideal.

QichenZhu commented 1 month ago

Raised an issue and PR in react-native-web. Waiting for their review.

lschurr commented 1 month ago

Any update on this one @QichenZhu?

QichenZhu commented 1 month ago

My PR is still pending review from react-native-web. Should we use patch-package in the meantime?

allroundexperts commented 1 month ago

I don't think so. Let's wait and see if they approve.

QichenZhu commented 1 month ago

No updates. Awaiting upstream review.

lschurr commented 3 weeks ago

PR is still open

QichenZhu commented 3 weeks ago

Can we move this to weekly?

luacmartins commented 3 weeks ago

Done

lschurr commented 2 weeks ago

Looks like we're still waiting on the PR review

lschurr commented 5 days ago

Looks like all changes on the PR were approved - anything holding us back on this one @QichenZhu @allroundexperts?