[HOLD for payment 2023-06-19] [$1000] Checkbox border turns green once another window is opened and revert back

[kavimuru]

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. Open the Settings menu.
2. Click on " Workspaces."
3. Open any Workspace.
4. Select "Manage Members."
5. Click "Select all."
6. In the presence of admin only as a member observe the checkbox, green border appears for a fraction of second, when clicked.
7. Open another window and observe the checkbox border turns green permanently till clicked again.

Expected Result:

The checkbox should always have a white border when the admin is the only member present, as the admin cannot be selected when using the "Select all" option.

Actual Result:

The border of checkbox "Select all" turns green once another window is opened and reverted back.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.13.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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Bug.1.mp4

Recording.560.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f763eb9e34751150
• Upwork Job ID: 1659525973736022016
• Last Price Increase: 2023-06-02

[melvin-bot]

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

[allroundexperts]

Proposal

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

Checkbox border turns green once focus is shifted back to window

What is the root cause of that problem?

When we un-select the checkbox, we're simply calling onBlur function here. This function just sets the state to be false, which in turn removes the green border. The element itself is actually still focused and when the window becomes active again (after a blur), the onFocus handler of the Pressable component kicks in since the checkbox never went out of focus.

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

We need to take the ref of the Checkbox and call the blur method here. This will not only change the blur state of the Checkbox to be false, but will also remove the focus from the Checkbox.

What alternative solutions did you explore? (Optional)

None

[hungvu193]

This is decided is not a bug here: #11283 (comment)
cc @parasharrajat

[parasharrajat]

Yes, not a big deal. We can close this.

[allroundexperts]

Yes, not a big deal. We can close this.

@parasharrajat It might not be something of a higher priority, but this IS a bug IMO.

[parasharrajat]

Why do you think this is a bug? When you move to focus back to the old tab, the browser focuses on the previously active element for A11Y. It is the same as pressing the Tab key to highlight an element.

This is not a bug but a feature.

[allroundexperts]

Why do you think this is a bug? When you move to focus back to the old tab, the browser focuses on the previously active element for A11Y. It is the same as pressing the Tab key to highlight an element.

This is not a bug but a feature.

@parasharrajat If the element was active previously, then IT should have the green border prior to the tab switching as well.

[allroundexperts]

This is happening with CheckBox only. See what happens for Radio Inputs.

Screen.Recording.2023-05-12.at.3.22.18.PM.mov

[parasharrajat]

This does not answer my question but repel it. Even does not align with your proposal where you are trying to remove the focus #18801 (comment).

[allroundexperts]

This does not answer my question but repel it. Even does not align with your proposal where you are trying to remove the focus #18801 (comment).

I mean, if you look at the code, you can clearly see that we're removing the focus styles without removing the actual focus. This is something that doesn't feel correct and is actually causing this inconsistency when compared with other components that we have in our App. Regardless of what I'm proposing, what makes it a bug IMO is that if an active element does not have a border before the tab switching, then it should not have a border after tab switching as well.

[parasharrajat]

what makes it a bug IMO is that if an active element does not have a border before the tab switching, then it should not have a border after tab switching as well.

Why? What is the reasoning behind this?

I don't think we should be worrying about this issue at all. This is not worth solving. IMO, it gives an advantage to the user. When the user returns back to the App, they can easily see which element was last active. Also, this is browser-native behavior.

[melvin-bot]

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

[mallenexpensify]

Oooooof, I feel like there are so many small, edge case, possible-bugs that require a bunch of random-ish steps in order to reproduce. Like... if we didn't fix this, would any user/customer ever think it was a bug? (end.. rant).

I'll come back to this tomorrow

[allroundexperts]

Oooooof, I feel like there are so many small, edge case, possible-bugs that require a bunch of random-ish steps in order to reproduce. Like... if we didn't fix this, would any user/customer ever think it was a bug? (end.. rant).

I'll come back to this tomorrow

@mallenexpensify I agree with you fully. I think adding a Low priority label would help Melvin to understand the situation maybe?

[melvin-bot]

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

[mallenexpensify]

I think adding a Low priority label would help Melvin to understand the situation maybe?

Agreed but... then we'd like just end up with a lot of these lil bugs never getting fixed then we'd have an app with ton of tiny bugs! (well.. maybe. I'm way less grumpy today, glad I slept on it).

So.. is this a small and edge case bug, yup.
Should we fix it... yes, unless there's a technical reason we might not want to that I'm unaware of. 👀 plz @MariaHCD to see if you agree, if so, please add External if you think it can be worked on by a contributor.

[MariaHCD]

I definitely agree that this is a small, edge case situation. I don't think a customer would actually notice but it is odd that the checkbox would still be highlighted when the user navigates away.

So, I agree with @allroundexperts, if the element wasn't highlighted before the tab switch, then it shouldn't be highlighted after.

While I don't necessarily think this is a bug since it's more of an small improvement, I still think we should fix it. Like @mallenexpensify said we don't want to end up with an app that has tiny, weird issues like these.

[melvin-bot]

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

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

• Replaced focus state with focus-visible #20252

If no regressions arise, payment will be issued on 2023-06-19. 🎊

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

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:

• [@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.
• [@mallenexpensify] 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: Fix green outline at checkbox #13181
• The offending PR has been commented on: Fix green outline at checkbox #13181 (comment)
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1686682753141099
• Determine if we should create a regression test for this bug: I'm leaning to a no, this is pretty minor bug

[parasharrajat]

I just noticed that we are using CSS styles in the implemented solution. That style is using theme colors that are hard-coded and not controlled with our theme variables.

IMO, it should be changed.

[MariaHCD]

Nice catch, @parasharrajat! @s77rt @bernhardoj let's find a way to have this controlled by our theme variables instead of hardcoding it and create a follow up PR.

Do you want to bring it to slack to get a consensus?

I think the consensus on Slack will still be that we shouldn't be hardcoding this. So I think we should first try to find an alternative solution and if we can't, we can move the conversation to Slack.

Thoughts?

[bernhardoj]

Here is the alternative solution I find from ChatGPT + SO:

useEffect(() => {
    const styleElement = document.createElement('style');
    const css = `
    div[role="checkbox"]:focus-visible > div[data-checkbox], div[role="checkbox"]:focus[data-focusvisible-polyfill] > div[data-checkbox] {
        border-color: ${themeColors.borderFocus} !important;
    }
    `;
    styleElement.appendChild(document.createTextNode(css));
    document.head.appendChild(styleElement);
}, []);

We will create a new style element and append it to head when App.js mounts. We can also use this solution for the splash screen background color

App/web/index.html

Lines 109 to 115 in 862955c

	#splash {
	position: absolute;
	bottom: 0;
	left: 0;
	right: 0;
	top: 0;
	background-color: #061B09;

[s77rt]

I prefer to keep this hard-coded for now. We already have the blue outline and the splash background hard-coded. Those need to be fixed too and I think such change can be handled once we add the light/dark theme switch feature. Otherwise if you have a strong preference on fixing this now we can probably use HtmlWebpackPlugin's templateParameters to pass the theme colours.

[MariaHCD]

I think such change can be handled once we add the light/dark theme switch feature

That's a fair point. Removing those hardcoded colors from index.html might already be a consideration for the Theme Switching / Light Mode project.

@shawnborton @grgia Can you confirm if removing hardcoded color values such as for the background color of the splash screen from index.html is something that is or should be part of Theme Switching / Light Mode?

App/web/index.html

Lines 109 to 115 in 862955c

	#splash {
	position: absolute;
	bottom: 0;
	left: 0;
	right: 0;
	top: 0;
	background-color: #061B09;

We introduced another hardcoded color here for focused checkboxes and we'd like to know whether we should fix that now but also don't want to cause any conflicts if there is already a plan in place to replace those hardcoded values.

[grgia]

for the hardcoded color on the splash, I believe that will remain theme-independent because it is created before the app is loaded. However, we should avoid hard-coding any colors that are seen after the app is loaded, especially ones that would be affected by a theme change. Does that help @MariaHCD?

[MariaHCD]

Thanks, @grgia!

So I think we'll need to fix the hardcoded color now. Thoughts, @bernhardoj @s77rt?

[s77rt]

@MariaHCD At this point looks like we have nothing to do. We have this PR #20997 that applied this discussed solution i.e add padding. Except that they didn't change the border color. @shawnborton seems okay with that so looks we all set for now. (The green hardcoded is also removed since it's no longer used and the generic blue one is used instead - the blue color is still hard coded though)

[mallenexpensify]

I see a lot of conversation after the PR hit production, any reason I shouldn't pay everyone now? It's been over 7 days

[s77rt]

I think it's ready for payment. No further action is required from our end. cc @MariaHCD can you please confirm the same

[mallenexpensify]

eeeeps, the old job closed. I was going to try to get this paid before the weekend.
@usmantariq96, @bernhardoj , @s77rt , can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~0161807157644d1613

[s77rt]

@mallenexpensify Accepted!

[usmantariq96]

@mallenexpensify offer accepted

[bernhardoj]

@mallenexpensify accepted

[MariaHCD]

We have this PR #20997 that applied this discussed solution i.e add padding.

Perfect, thanks! Yes, looks like we're good to close this out.

[mallenexpensify]

Thanks all, everyone is paid (inc. bonus for timeliness, I added this to our design guideline sheet