[HOLD for payment 2024-01-11] [$500] Status - Highlighting minutes and pressing Back key does not reset it to 00

[izarutskaya]

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: v1.4.13-2
Reproducible in staging?: Y
Reproducible in production?: N
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
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to Settings > Profile > Status.
3. Click Clear after.
4. Click Custom > Time.
5. Highlight the minutes.
6. Press Back key on keyboard.

Expected Result:

The minutes will be reset to 00.

Actual Result:

Nothing happens to the minutes and the cursor moves to the beginning of hours.

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

Bug6313966_1702609442496.-871908312044640390bandicam_2023-12-15_10-03-29-401.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a5a5a8e401086431
• Upwork Job ID: 1735595606525665280
• Last Price Increase: 2023-12-15
• Automatic offers:
• s77rt | Reviewer | 28065612
• paultsimura | Contributor | 28065613

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @pecanoro (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[bernhardoj]

Proposal

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

Can't delete the whole hour or minute by selecting the text.

What is the root cause of that problem?

Currently, if we delete the whole hour/minute, the text will be empty and we do early return because it's not a numeric value.

App/src/components/TimePicker/TimePicker.js

Lines 119 to 124 in 8bef4bb

	const handleHourChange = (text) => {
	const isOnlyNumericValue = /^\d+$/.test(text.trim());
	// Skip if the user is pasting the text or use non numeric characters.
	if (selectionHour.start !== selectionHour.end || !isOnlyNumericValue) {
	return;
	}

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

Let's handle the case where the text is empty and set it to '00'

if (!text.trim()) {
    setHours('00');
    setSelectionHour({start: 2, end: 2});
    return;
}

(we need to do this for minute too)

[Julesssss]

Removing blocker status, you can still change it with the delete key using the caret:

[paultsimura]

Proposal

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

The backspace logic on TimePicker is broken.

What is the root cause of that problem?

This happens because of the function we are calling on the onKeyPress of the minutes input:

App/src/components/TimePicker/TimePicker.js

Lines 323 to 332 in 8bef4bb

	const handleFocusOnBackspace = useCallback(
	(e) => {
	if (selectionMinute.start !== 0 || e.key !== 'Backspace') {
	return;
	}
	hourInputRef.current.focus();
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[selectionMinute.start],
	);

The purpose of this function is to move the caret to the Hours section if we press Backspace while being at the beginning of the Minutes section. This is logical, but we do not check if there is an actual selection, we only check the caret position.

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

The whole idea is based on these 2 points:

1. Fix moving the cursor to hours – this should work only if the cursor is at the beginning of minutes, and there is no active selection.
2. Allow deleting hours & minutes by pressing Backspace when both digits are selected.

Now to the technical details.

First, we should alter the logic of handleFocusOnBackspace to work only if the caret is at position 0, but there is no actual selection:

if (selectionMinute.start !== 0 || selectionMinute.end !== 0 || e.key !== 'Backspace') {
    return;
}

App/src/components/TimePicker/TimePicker.js

Lines 323 to 332 in 8bef4bb

	const handleFocusOnBackspace = useCallback(
	(e) => {
	if (selectionMinute.start !== 0 || e.key !== 'Backspace') {
	return;
	}
	hourInputRef.current.focus();
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[selectionMinute.start],
	);

Second, we should fix the way we handle the hours & minutes change to allow deleting the selection:

Introduce 2 util functions as they will be reused:

    const resetHours = () => {
        setHours('00');
        setSelectionHour({start: 0, end: 0});
    }

    const resetMinutes = () => {
        setMinute('00');
        setSelectionMinute({start: 0, end: 0});
        focusHourInputOnLastCharacter();
    }

Here in the resetMinutes function, I've added a call of focusHourInputOnLastCharacter – it can be removed if we decide that we do not want to switch focus to Hours on removing the minutes by selection. But this is how it's working now when we are removing the first Minutes digit.

In both handleMinutesChange and handleHourChange, add an early return for an empty string at the beginning of the function (this will mean the whole value was selected & removed). I don't think we should use text.trim() here because it will clear the value on pressing space as well, not on explicitly removing the value by delete or backspace:

    const handleHourChange = (text) => {
        if (_.isEmpty(text)) {
            resetHours();
            return;
        }
        ...
    }

    const handleMinutesChange = (text) => {
        if (_.isEmpty(text)) {
            resetMinutes();
            return;
        }
        ...
    }

For the digital pad, make the following changes to add early returns by resetting hours or minutes if the full value was selected:

    if (isHourFocused) {
        if (selectionHour.start === 0 && selectionHour.end === 2) {
            resetHours();
            return;
        }
        ...
    } else if (isMinuteFocused) {
        if (selectionMinute.start === 0 && selectionMinute.end === 2) {
            resetMinutes();
            return;
        }
        
        if (selectionMinute.start === 0 && selectionMinute.end === 0) {
            focusHourInputOnLastCharacter();
            return;
        }
        ...
    }

App/src/components/TimePicker/TimePicker.js

Lines 263 to 278 in aa1d87c

	if (key === '<' || key === 'Backspace') {
	if (isHourFocused) {
	const newHour = replaceWithZeroAtPosition(hours, selectionHour.start);
	setHours(newHour);
	setSelectionHour(decreaseBothSelectionByOne(selectionHour));
	} else if (isMinuteFocused) {
	if (selectionMinute.start === 0) {
	focusHourInputOnLastCharacter();
	return;
	}
	const newMinute = replaceWithZeroAtPosition(minute, selectionMinute.start);
	setMinute(newMinute);
	setSelectionMinute(decreaseBothSelectionByOne(selectionMinute));
	}
	return;
	}

What alternative solutions did you explore? (Optional)

[dukenv0307]

Proposal

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

Nothing happens to the minutes and the cursor moves to the beginning of hours.

What is the root cause of that problem?

When we highlight all minutes and click on Backspace key, we will only focus on the hour

App/src/components/TimePicker/TimePicker.js

Lines 325 to 328 in 8bef4bb

	if (selectionMinute.start !== 0 || e.key !== 'Backspace') {
	return;
	}
	hourInputRef.current.focus();

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

1. Before focusing the hour input, we should clear the minutes

setMinute(replaceWithZeroAtPosition(minute, selectionMinute.start));
setSelectionMinute(decreaseBothSelectionByOne(selectionMinute));
focusHourInputOnLastCharacter();

App/src/components/TimePicker/TimePicker.js

Lines 325 to 328 in 8bef4bb

	if (selectionMinute.start !== 0 || e.key !== 'Backspace') {
	return;
	}
	hourInputRef.current.focus();

2. We should remove the comparison selection.start !== selection.end because now when we select one digit of hour or minutes and use Backspace key, it doesn't reset to 0.

App/src/components/TimePicker/TimePicker.js

Line 122 in 8bef4bb

if (selectionHour.start !== selectionHour.end || !isOnlyNumericValue) {

App/src/components/TimePicker/TimePicker.js

Line 191 in 8bef4bb

if (selectionMinute.start !== selectionMinute.end || !isOnlyNumericValue) {

What alternative solutions did you explore? (Optional)

NA

[s77rt]

@bernhardoj Thanks for the proposal. Your solution makes sense but it does not cover the minute case. Can you please update your proposal to cover it? Also we need to make it work with the backbutton in the software keyboard too

Screen.Recording.2023-12-16.at.2.15.05.AM.mov

[s77rt]

@paultsimura Thanks for the proposal. The approach looks good to me. Is there any reason we need to adjust the regex instead of just checking if the text is empty beforehand? Also same note above regarding the soft num pad backbutton

[s77rt]

@dukenv0307 Thanks for the proposal. I don't think we should clear the input on the key press event. Handling this on handleHourChange / handleMinutesChange is more fitting.

[dukenv0307]

@s77rt This key press function only apply for backspace.

[melvin-bot]

Current assignee @pecanoro is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[pecanoro]

Assigning @paultsimura!

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @paultsimura 🎉 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 📖

[paultsimura]

Thanks. The PR is ready for review: #33344

[paultsimura]

@pecanoro while working on this issue, we've decided to fix not only this one but a couple of other bugs related to the TimePicker (it was quite buggy actually). This led to spending a solid week of day-to-day development and testing from both the C and C+ sides.

I've seen that in such cases, contributors negotiate for a larger bounty since otherwise these bugs would have been fixed as separate issues later. I'll leave the bounty decision up to you, but could we please make this PR free from potential regression penalties? As it did cover a lot of logic changes to fix different cases, but we might have missed some edge cases.

For instance, here are 4 other fixed bugs that were out of scope here:

Unexpected re-conversion of 24h to 12h format when entering the first digit

12_24_on_input.mov

Jumping cursor when moving from hours to minutes

jumping_cursor.mov

Buggy behavior when removing digits using the "Delete" button

removal_by_delete.mov

Buggy behavior when pressing "Space"

space_removal.mov

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.21-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: TimePicker – support replacement & removal by full selection #33344

If no regressions arise, payment will be issued on 2024-01-11. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @paultsimura requires payment automatic offer (Contributor)
• @s77rt requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] 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.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: Status clear after #24620
• The offending PR has been commented on: Status clear after #24620 (comment)
• A discussion in #expensify-bugs has been started: I don't think this is needed
• Determine if we should create a regression test for this bug: The work done here is mostly adding missing features and fixing bugs for a component that is newly written. I don't think we require a regression test in this case

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @lschurr you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[lschurr]

Hmm @bfitzexpensify is already on this one. Are you able to handle, Benny? Un-assigning myself :)

[bfitzexpensify]

Payments complete, agreed we don't need a regression test here. Closing this one out.