[HOLD for payment 2023-10-12] [$500] Web - Focus outline for checkbox not aligned with border in TaskView

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to FAB > Assign task. Enter title, share somewhere and create a new task.
2. In task view, click on composer and press shift + TAB 4 times (or more if there are comments in report).
3. Notice that, focus outline is not aligned with border, radius is less for outline.

Expected Result:

Radius of outline on focus should be aligned with border like we have for TaskPreview and all other components.

Actual Result:

Focus outline is not aligned with border.

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.72.6
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

vid2.mov

vid1.mov

Expensify/Expensify Issue URL:
Issue reported by: @Nikhil-Vats
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695154519892849

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

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

[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 @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[kbecciv]

Proposal by: @Nikhil-Vats
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695154519892849

Proposal

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

Focus outline for checkbox is not aligned with border in TaskView. It is aligned at all other places.

What is the root cause of that problem?

In TaskView, size of checkbox is larger than all other places so we use larger border-radius of 8px. We pass it via props.

App/src/components/ReportActionItem/TaskView.js

Line 92 in 6d67552

containerBorderRadius={8}

In all other places, we use the default border-radius for checkbox that is 4px.

App/src/components/Checkbox.js

Line 65 in 6d67552

containerBorderRadius: 4,

In order for the outline to be aligned, the border-radius for PressableWithFeedback (ancestor of Checkbox) should be 2px more than the border-radius of Checkbox but right now border-radius of PressableWithFeedback is fixed -

App/src/styles/styles.js

Lines 2642 to 2643 in 6d67552

	checkboxPressable: {
	borderRadius: 6,

So, except TaskView, outline is aligned for all checkboxes since they have 4px border-radius.
This was finalized in this PR but TaskView component was redesigned later and the checkbox size was made larger without keeping outline in mind which created this bug.

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

We need to make the border-radius of PressableWithFeedback dynamic by making it 2px more than the border-radius of Checkbox. For this, we can make a function in StyleUtils similar to getCheckboxContainerStyle which was created for dynamic border-radius for checkbox.

/**
 * Returns the checkbox pressable style
 */
function getCheckboxPressableStyle(borderRadius: number): ViewStyle | CSSProperties {
    return {
        padding: 2,
        justifyContent: 'center',
        alignItems: 'center',
        // eslint-disable-next-line object-shorthand
        borderRadius: borderRadius,
    };
}

Then, we can remove checkboxPressable styles from styles.js here and use the above utility function instead -

<PressableWithFeedback
    disabled={props.disabled}
    onPress={firePressHandlerOnClick}
    onMouseDown={props.onMouseDown}
    ref={props.forwardedRef}
    style={[props.style, StyleUtils.getCheckboxPressableStyle(props.containerBorderRadius + 2)]} // to align outline, border-radius of pressable should be 2px more than Checkbox
    onKeyDown={handleSpaceKey}
    accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
    accessibilityState={{checked: props.isChecked}}
    accessibilityLabel={props.accessibilityLabel}
    pressDimmingValue={1}
>

What alternative solutions did you explore? (Optional)

NA
Result -
https://github.com/Expensify/App/assets/31413407/48c6ec3d-c302-47ba-8d72-b2ef2c88fbad

[Nikhil-Vats]

Reposting the proposal as the video is not visible in the one above.

Proposal

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

Focus outline for checkbox is not aligned with border in TaskView. It is aligned at all other places.

What is the root cause of that problem?

In TaskView, size of checkbox is larger than all other places so we use larger border-radius of 8px. We pass it via props.

App/src/components/ReportActionItem/TaskView.js

Line 92 in 6d67552

containerBorderRadius={8}

In all other places, we use the default border-radius for checkbox that is 4px.

App/src/components/Checkbox.js

Line 65 in 6d67552

containerBorderRadius: 4,

In order for the outline to be aligned, the border-radius for PressableWithFeedback (ancestor of Checkbox) should be 2px more than the border-radius of Checkbox but right now border-radius of PressableWithFeedback is fixed -

App/src/styles/styles.js

Lines 2642 to 2643 in 6d67552

	checkboxPressable: {
	borderRadius: 6,

So, except TaskView, outline is aligned for all checkboxes since they have 4px border-radius.

This was finalized in this PR but TaskView component was redesigned later and the checkbox size was made larger without keeping outline in mind which created this bug.

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

We need to make the border-radius of PressableWithFeedback dynamic by making it 2px more than the border-radius of Checkbox. For this, we can make a function in StyleUtils similar to getCheckboxContainerStyle which was created for dynamic border-radius for checkbox.

/**
 * Returns the checkbox pressable style
 */
function getCheckboxPressableStyle(borderRadius: number): ViewStyle | CSSProperties {
    return {
        padding: 2,
        justifyContent: 'center',
        alignItems: 'center',
        // eslint-disable-next-line object-shorthand
        borderRadius: borderRadius,
    };
}

Then, we can remove checkboxPressable styles from styles.js here and use the above utility function instead -

<PressableWithFeedback
    disabled={props.disabled}
    onPress={firePressHandlerOnClick}
    onMouseDown={props.onMouseDown}
    ref={props.forwardedRef}
    style={[props.style, StyleUtils.getCheckboxPressableStyle(props.containerBorderRadius + 2)]} // to align outline, border-radius of pressable should be 2px more than Checkbox
    onKeyDown={handleSpaceKey}
    accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
    accessibilityState={{checked: props.isChecked}}
    accessibilityLabel={props.accessibilityLabel}
    pressDimmingValue={1}
>

What alternative solutions did you explore? (Optional)

NA

Result -

Screen.Recording.2023-09-20.at.2.04.16.AM.mov

[jliexpensify]

Removing myself as I was the 2nd B0 assigned

[jjcoffee]

Reviewing on Monday!

[jjcoffee]

@Nikhil-Vats has a really nice clear proposal with the correct RCA and the solution makes sense, so happy to go with them!

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[deetergp]

Thanks for a very nicely-written proposal, @Nikhil-Vats 👍

[Nikhil-Vats]

Working on the PR now and will have it ready for review tonight.

[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.3.77-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix checkbox outline in TaskView #28543

If no regressions arise, payment will be issued on 2023-10-12. 🎊

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:

• @jjcoffee requires payment
• @Nikhil-Vats requires payment
• @Nikhil-Vats requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A root cause has existed since the beginning
• 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: N/A
• 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: N/A
• Determine if we should create a regression test for this bug. No - too low impact
• 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.

[jjcoffee]

@sonialiap Checklist complete!

[melvin-bot]

@deetergp, @sonialiap, @jjcoffee, @Nikhil-Vats Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sonialiap]

@jjcoffee review + bonus = $750 - paid ✔️
@Nikhil-Vats report + fix + bonus = $800 - offer sent

[melvin-bot]

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

[sonialiap]

I'm OOO Oct 16-23. Adding leave buddy

@jliexpensify Requires payment once offers are accepted. Can be closed once paid https://www.upwork.com/ab/applicants/1704525429405278208/hired

[jjcoffee]

@jliexpensify Offer accepted!

[sonialiap]

I've issued payment to Joel ✔️

[Nikhil-Vats]

@sonialiap @jliexpensify I have accepted the offer.

[jliexpensify]

Paid and job closed!