[HOLD for payment 2023-08-08] [$1000] Web - Assign Task - Delay in transitioning from a hand cursor to a "not allowed" sign.

[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. Clicked on the plus sign.
2. Clicked on "Assign Task".
3. Entered the title and clicked on "Next".
4. Filled in the "Assignee" and "Share somewhere" fields.
5. Clicked on "Confirm Task".
6. Upon clicking the "Mark as Done" button, the completed task is indicated by a green tick.
7. Observed a delay in the transition from a hand cursor to a "not allowed" sign.

Expected Result:

The transition from a hand cursor to a "not allowed" sign should occur without any delay.

Actual Result:

There is a delay in the transition from a hand cursor to a "not allowed" sign.

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.35-5
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
Expensify/Expensify Issue URL:

Bug.4.mp4

Recording.5281.mp4

Issue reported by: @usmantariq96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688195714793539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01620c737f6783071d
• Upwork Job ID: 1678893891094454272
• Last Price Increase: 2023-07-18

[melvin-bot]

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

[honnamkuan]

Proposal

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

When task is marked as completed, there is a delay before the cursor on the "To" row (including Completed status) get changed to disabled cursor

What is the root cause of that problem?

In PressableWithFeedback, the implementation will set setIsExecuting(true) when button is pressed, and then setIsExecuting(false) when interaction completed, the idea is to prevent registering quick repeated pressing, but when the onPress function executes really quickly, it caused a flashing of cursor from default to disabled, then back to default cursor

Therefore, there was a fix #18862 to introduce a delay of 1000ms after isDisabled:true before the cursor actually get changed to disabled style, to eliminate the flashing.
That is the cause of 1 second delay in cursor change after marking a task as completed, as reported in this ticket.

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

There is nothing wrong with the implementation, however the fixed delay of 1000ms is too long, we should set it to shorter time e.g. 100ms which would also prevent the cursor flashing, and does not add much of the delay of change in term of UX.

https://github.com/Expensify/App/blob/main/src/components/Pressable/GenericPressable/BaseGenericPressable.js#L121

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

@michaelhaxhiu Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@michaelhaxhiu Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[melvin-bot]

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

[melvin-bot]

Current assignee @michaelhaxhiu is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[michaelhaxhiu]

Let's move forward, reproducible & unique!

[tienifr]

Proposal

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

There is a delay in the transition from a hand cursor to a "not allowed" sign.

What is the root cause of that problem?

There's this PR #18865 that adds a timeout 1000ms in GenericPressable before the disabled cursor style kicks in. This is needed in order to fix the quick cursor style change that often occurs when the PressableWithFeedback button is clicked quickly. Can see in the PR and here for more context on that change.

But I don't think we should use the timeout approach in the first place, since it causes this issue.

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

There're 3 states of the Pressable:

1. Active
2. Disabled
3. Executing (means it's executing some actions after the user presses the button)

Currently we're not clearly handling the Executing state, we're just using the timeout to patch it.

To handle the Executing state properly, we can:

1. Pass the isExecuting from PressableWithFeedback to GenericPressable here

isExecuting={isExecuting}

2. In BaseGenericPressable here, derive the shouldUseDisabledCursor from the isDisabled and isExecuting

const shouldUseDisabledCursor = useMemo(() => isDisabled && !isExecuting, [isDisabled, isExecuting]);

and remove the state and the useEffect that uses timeout here

That means, we have 3 Pressable states in BaseGenericPressable clearly mapped to our props:

1. Active: isDisabled false
2. Disabled: isDisabled true, isExecuting false
3. Executing: isDisabled true, isExecuting true

And we don't need to use any timeout.

What alternative solutions did you explore? (Optional)

If we want to clearly define the state for the GenericPressable rather than reusing the isExecuting and isDisabled, we can introduce a new prop pressableState for that, but I found the above is the cleanest.

[rfwoolf]

@tienifr
I agree that your proposal would (in theory) solve the current bug/issue,
but it would regress a previous fix in PR 18865 and so there is a question of its affect on the previous issue you reference ( 18862),
where rapidly clicking on BaseGenericPressable caused a flickering effect (you can see a video of that problem here?
If you have already coded the fix, please try reproduce that "flickering" issue

[tienifr]

@rfwoolf my proposal is the correct fix for that linked issue, it will not regress it. I tested both issues and all work well. Feel free to highlight if you see any problem.

[rfwoolf]

I have tested @tienifr's proposal and it seems to work.

In case it may assist, there was 1 more line that needed to be changed:
In BaseGenericPressable.js after line 45, add isExecuting.
This adds isExecuting to the list of props of GenericPressable which will prevent a "isExecuting is undefined" error in tienifr's solution.

[honnamkuan]

I believe @tienifr's proposal will cause the cursor to never turn to disabled-style even when Pressable is disabled when isExecuting:true

[rfwoolf]

Here is a screenrecord after implementing @tienifr's proposal:
https://github.com/Expensify/App/assets/312073/a6d96cfd-a256-48c9-90a1-4b022f9cff24

Re @honnamkuan's concern:
Does the video contradict what you're saying?
Or can you think of another example I can test?

[mollfpr]

I'm sorry for the delay. I didn't have much time left, so that I will review this in the morning.

[dannyaka1991]

Proposal

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

When the user click "Mark as done" button in the task header, there is a delay before the cursor turns to a disabled cursor.

What is the root cause of that problem?

There is a timeout of 1000ms in GenericPressable component when the component becomes disabled and the cursor is changing to a disabled cursor. This is added to fix the cursor blinking issue when the user clicks a button which has a short executing time. You can check more details in below links.
#18865
#18862

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

There are two types of disability for the GenericPressable component. Permanent and temporary.
Permanent disability is because of task completion, disabled screen reader, etc.
Temporary disability is because of execution.

We need to add a timeout before cursor update, only in the case of temporary disability.
To implement this, we need to pass the isExecuting from PressableWithFeedback component to GenericPressable component as a prop.
If isDisabled is true and isExecuting is true, this means temporary disability and we need to update the cursor with a delay.
If isDisabled is true and isExecuting is false, this means permanent disability and we need to update the cursor immediately.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

📣 @dannyaka1991! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[dannyaka1991]

Contributor details
Your Expensify account email: akamefi202@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01b371ed61a241a259

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

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

[michaelhaxhiu]

Note: I'm preparing to go OOO for ~2 weeks and going to assign another BZ to oversee the final payment step on 08/08. Thank you in advance for helping push this along!

Payment summary:

• Bug reporter @usmantariq96 $250
• Contributor @tienifr $1500
• C+ @mollfpr $1500

Then @mollfpr can you please ensure the checklist is complete?

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR. This was expected before as created #18862.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

As is expected, #18862, we might not find a similar issue again. We already documented delay usage as an absolute necessity in the PR_REVIEW_GUIDELINES. So the regression test should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Test can only be done on the Web and Desktop

1. Open the App and sign-in
2. Click on the green FAB
3. Clicked on "Assign Task"
4. Enter the title and click on "Next"
5. Fill in the "Assignee" and "Share somewhere" fields
6. Clicked on "Confirm Task"
7. Upon clicking the "Mark as Done" button, the completed task is indicated by a green tick.
8. Verify that the transition from a hand cursor to a "not allowed" sign occurs without any delay.

[johncschuster]

Hi, @usmantariq96! I can't find you in Upwork. Can you please apply to this job?

[johncschuster]

Summary of payments

• Bug reporter @usmantariq96 $250
• Contributor @tienifr $1500
• C+ @mollfpr $1500 (please request this payment via NewDot)

CC @JmillsExpensify

[usmantariq96]

@johncschuster applied
Thanks

[mollfpr]

@johncschuster I think I'm still no eligible to request the payment via ND.

[melvin-bot]

@johncschuster, @youssef-lr, @michaelhaxhiu, @mollfpr, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@johncschuster, @youssef-lr, @michaelhaxhiu, @mollfpr, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@johncschuster, @youssef-lr, @michaelhaxhiu, @mollfpr, @tienifr Eep! 4 days overdue now. Issues have feelings too...

[youssef-lr]

What's holding us from closing this one?

[michaelhaxhiu]

Seems this got caught up. I'm back from OOO and stepping back in. Paid the last 2 contributors, closing now.