[HOLD for payment 2023-10-05] [$500] Web - Focus Border is not disappearing on Click

[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. Open chat, hover over a chat to get popup with menu items.
2. Use Tab/f11 to navigate to that element.
3. Once it's on Focus click on it, Click multiple times, Click on neighbouring elements,

Expected Result:

Border should be disappeared after clicking

Actual Result:

Border is not disappeared even clicking

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

Screen.Recording.2023-08-26.at.1.06.40.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @Charan-hs
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693035839221489

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0165692f1106821d38
• Upwork Job ID: 1699153497359552512
• Last Price Increase: 2023-09-12
• Automatic offers:
• DylanDylann | Contributor | 26739286
• Charan-hs | Reporter | 26739289

[melvin-bot]

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

[AJV009]

I am not sure if this is an error because you see as soon as you move the mouse out of the message the circle goes away, right?

[melvin-bot]

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

[DylanDylann]

Proposal

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

• Web - MenuItem - Focus Border not disappearing onClick

What is the root cause of that problem?

• Currently, when clicking on the emoji, we call preventDefault() which will ignore the default behavior of the browser: blur the current activeElement:
  

  App/src/components/BaseMiniContextMenuItem.js

  Line 56 in 6b1a667

  onMouseDown={(e) => e.preventDefault()}

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

• The e.preventDefault is added to fix the issue: "prevent blur on composer when click on mini context menu item". So when clicking on emoji we can check if the report action composer is focused. If true, do not blur it. Like below:

 onMouseDown={(e) => {
                    if (ReportActionComposeFocusManager.isFocused() || ReportActionEditComposeFocusManager.isFocused()) {
                        e.preventDefault();
                    }
                }}

• The ReportActionComposeFocusManager.isFocused() has appeared in our source code, it will expose the current focus state of the report action composer.
• We can also create a similar ReportActionEditComposeFocusManager.isFocused() to expose the current focus state of the edit composer as well

What alternative solutions did you explore? (Optional)

• NA

Result

Screencast.from.06-09-2023.21.32.41.webm

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0165692f1106821d38

[melvin-bot]

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

[melvin-bot]

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

[johncschuster]

Not overdue. This was just triaged, Melv.

[ArekChr]

@DylanDylann I think document.activeElement can be used only in index.website.js files, so this should be a separate function or maybe is another way to blur the active element?

[DylanDylann]

@ArekChr

• As my first solution [HOLD for payment 2023-10-05] [$500] Web - Focus Border is not disappearing on Click #26431 (comment), we can remove the e.preventDefault()
• Also we need to check if this bug occurs on mobile and desktop, if not, we just need to call document.activeElement.blur();

[DylanDylann]

@ArekChr
I just tested on the Desktop and saw that document.activeElement.blur(); work well as below video

Screen.Recording.2023-09-06.at.16.29.51.mov

So that, If we come up with this solution we need to define a new function for native and web/desktop separately

[getusha]

Proposal

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

Focused outline is not dismissed on click

What is the root cause of that problem?

In

App/src/components/BaseMiniContextMenuItem.js

Line 56 in 6b1a667

onMouseDown={(e) => e.preventDefault()}

We have e.preventDefault() which prevents the default behavior of blurring and transferring the focus to the clicked target element, which makes it preserve the previously focused element.

In first place the e.preventDefault() was added to avoid the composer being blurred when we click on the emoji inside mini context menu. which was introduced here #24252 aiming to solve #20079

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

we should call e.preventDefault() only if the focused element is not an input. this will make sure we fix this issue without re-introducing #20079

It will be something like this.

onMouseDown={(e) => {
     if (!['TEXTAREA', 'INPUT'].includes(DomUtils.getActiveElement().nodeName)) { return; }
     e.preventDefault()
}}

Since we are using DomUtils.getActiveElement it won't be necessary to separate the files.

App/src/libs/DomUtils/index.js

Lines 5 to 7 in 57b7606

	function getActiveElement() {
	return document.activeElement;
	}

We can also utilize blurActiveElement if we decided to manually blur the focus.

Result

Screen.Recording.2023-09-06.at.3.49.14.PM.mov

What alternative solutions did you explore? (Optional)

N/A

cc @ArekChr

[DylanDylann]

@ArekChr Updated proposal #26431 (comment). Please help review again. Thanks!

[getusha]

@DylanDylann you updated your proposal after mine and your solution is also based on my approach. whether to use ReportActionComposeFocusManager or not can be handled in the PR the proposal is not to call e.preventDefault if composer is focused.

[DylanDylann]

@getusha your proposal will check if the current active element is [input, textarea] or not, If not, we will blur the current active elements. My proposal will check if the current active element is exactly the main composer/edit composer or not, if not, we will blur the current active. I want the blur action will affect all the elements except exactly the main composer and the edit composer, because we cannot guarantee that there will be other input elements in the future or not

[getusha]

@getusha your proposal will check if the current active element is [input, textarea] or not, If not, we will blur the current active elements. My proposal will check if the current active element is exactly the main composer/edit composer or not, if not, we will blur the current active. I want the blur action will affect all the elements except exactly the main composer and the edit composer because we cannot guarantee that there will be other input elements in the future or not

the only difference is that you're using Focus manager which can be suggested by c+, the main proposal is we are triggering e.preventDefault when the composer is not focused, which you updated to it after my proposal.

[DylanDylann]

@getusha yeah. The GitHub will store the edit history. I just want to make sure this issue is fixed as best as possible.

[melvin-bot]

@johncschuster, @ArekChr Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[johncschuster]

Ok! I've issued payment to @DylanDylann and @Charan-hs. @getusha, can you accept the proposal in Upwork? I'll issue payment to you as soon as that's done.

Thanks, everyone!

[getusha]

@johncschuster accepted.

[johncschuster]

Awesome. Thank you! I'll get payment issued now.

[johncschuster]

Payment has been issued to @getusha! Thanks for your patience while I worked this out, everyone!

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[johncschuster]

Oh, whoops. I think I forgot to close this one.

@ArekChr, do you think we need regression test steps?

[ArekChr]

Yes, we can add regression steps here.

Regression test proposal.

1. Open chat and hover over a chat to get a popup with menu items.
2. Use Tab/f11 to navigate to that element.
3. Once it's on Focus, click on one of the emojis multiple times.
4. Click another emoji and verify if the hover border from the previous one disappears.

Do we agree 👍 or 👎

[johncschuster]

Thanks, @ArekChr! I'll get that regression test issue created shortly.

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[johncschuster]

This is on me. I need to get the regression issue going. Will do that shortly.

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@johncschuster, @ArekChr, @jasperhuangg, @DylanDylann 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[johncschuster]

Everything is done except the regression test issue. Doing that now.