[$2000] Web - Chat -If you click on a chat message, press Esc-message is highlighted with a blue frame

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

---

Issue found when executing PR #15650

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Login with any account
3. Go to any chat
4. Write some text
5. Click outside the compose box - on a message in a chat
6. Press Esc

Expected Result:

Nothing should happen

Actual Result:

If you click on a message in the chat, press Esc - message is highlighted with a blue frame.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• Windows / Chrome
• MacOS / Desktop

Version Number: 1.2.80.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug5967837_Recording__345.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014f93d9e7848a9dc1
• Upwork Job ID: 1633587892979748864
• Last Price Increase: 2023-03-15

[MelvinBot]

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

[MelvinBot]

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

[lschurr]

I reproduced this in Chrome. Definitely an odd one.

[MelvinBot]

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

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~014f93d9e7848a9dc1

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[getusha]

I first reported this here https://expensify.slack.com/archives/C049HHMV9SM/p1676142137019119

cc @kbecciv

[eh2077]

I think this issue is related to keyboard navigation or accessibility and they are supposed to be on hold, see the original discussion thread https://expensify.slack.com/archives/C01GTK53T8Q/p1668604822482759

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[bernhardoj]

It's how the native elements works. Ref: https://www.github.com/adobe/react-spectrum/issues/2821 (you can test the fiddle on the issue)

So, I don't think this is a bug because chat message is focusable.

[osofus]

Proposal

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

On web, focused elements are displayed with a blue border when the mouse is not over it.

You don't have to do all the steps listed to reproduce this.
All you need to do is, click on any message in any report, press Esc and then move the mouse away from the message.

Proposal.15723-1.mov

What is the root cause of that problem?

Pressing Esc focuses the message, which was selected by clicking the mouse.
When mouse is over a focused message, the app is programmed to display the context menu.
If the mouse is not over, then :focus-visible CSS styling takes effect, which put the blue border aroud the message due to:

:focus-visible { outline: 0; box-shadow: inset 0px 0px 0px 1px #5AB0FF; }

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

Prevent the Esc key from focusing the selected message.
We have to override the default behaviour in this case.

I don't have a definite plan to achieve this, but putting this proposal out there just in case it helps someone else find the solution.
Also, I want to stay on the loop with this one.

What alternative solutions did you explore?

Removing the box-shadow removes the blue border from being displayed around a focused message when the mouse is not over it.

:focus-visible { outline: 0; }

But it is better not to change this, as not showing the blue border around focused objects affects the tab key shortcut also.

Proposal.15723-2.mov

[AmjedNazzal]

The issue is because of default browser behaviour, it is a normal behaviour which can be fixed.

To fix the issue I will add a css rule to remove this behaviour and after that it shouldn't appear again, I have managed to remove the issue on my web developer tool as I will show the issue being elemenated in the screen recoding attached.

Untitled.design.mp4

[MelvinBot]

📣 @AmjedNazzal! 📣

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>

[AmjedNazzal]

Contributor details
Your Expensify account email: amjednazzal@outlook.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0113f371085fef1cd8

[MelvinBot]

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

[moezh]

Proposal

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

If you click on a message in the chat, and then press Esc, the message is highlighted with a blue frame.

What is the root cause of that problem?

The default behavior when pressing Esc after selecting a message, in this case, is to focus on the message by using the :focus-visible CSS styling.

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

To ensure consistency with the behavior of the rest of the app, we need to focus on the input text of the composer when the ESC key is pressed after selecting a message. This can be achieved by adding this.focus(false) to the code introduced by PR #15650. By implementing this change, we can resolve the issue of the inconsistent behavior and provide a more seamless user experience.

Screen.Recording.2023-03-09.at.10.06.20.AM.mov

What alternative solutions did you explore? (Optional)

No alternative solutions were explored.

[mdneyazahmad]

Proposal

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

When any key not just ESC is pressed on a focused chat list item, this is highlighted, and outine is not consistent. It is good to have it for accessibility, but the issue here is that on hover over the message item, outline is invisible, and when the hover moves to other message, the outline again shows.

issue.mp4

What is the root cause of that problem?

App/web/index.html

Line 44 in 22cfc7a

:focus-visible {

There is a slight difference between :focus and :focus-visible. :focus-visible is not applied unless the focus is caused by keyboard event. This is what happens:

App/src/pages/home/report/ReportActionItem.js

Line 229 in 22cfc7a

<PressableWithSecondaryInteraction

1. When user click on the message item, PressableWithSecondaryInteraction gets focused but :focus-visible is not applied, as the focus is initiated by a click.
2. Now if you press any key (you are already focused on Pressable), this applied :focus-visible as this is a keyboard event.

The reason it hides outline is that.

App/src/pages/home/report/ReportActionItem.js

Line 244 in 22cfc7a

<View

This applies a background color, and the outline is invisible (inside this element)

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

App/src/pages/home/report/ReportActionItem.js

Line 238 in 22cfc7a

<Hoverable>

We can blur the focused item when it is hovered. As this will not be focused, there will be no outline. This does no break keyboard accessibility, because it only blurs, if the user moves his cursor, and keyboard is still accessible.

            <PressableWithSecondaryInteraction
                pointerEvents={this.props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ? 'none' : 'auto'}
                ref={el => this.popoverAnchor = el}
                onPressIn={() => this.props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
                onPressOut={() => ControlSelection.unblock()}
                onSecondaryInteraction={this.showPopover}
                preventDefaultContentMenu={!this.props.draftMessage}
                withoutFocusOnSecondaryInteraction
                // add this prop
                dataSet={{
                    type: 'message-item',
                }}

                <Hoverable onHoverIn={() => {
                    if (!document.activeElement) {
                        return;
                    }

                    if (document.activeElement.dataset.type !== 'message-item') {
                        return;
                    }

                    document.activeElement.blur();
                }}

What alternative solutions did you explore? (Optional)

None

Result

fixed.mp4

[lschurr]

@sobitneupane could you review these proposals?

[s77rt]

Proposal

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

Clicking a message then pressing the ESC key shows a blue frame.

What is the root cause of that problem?

The ReportActionItem is focusable where focus-visible style is applied.

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

1. Make the inner View of ReportActionItem focusable (but not tabbable) i.e. pass focusable={false}. This will make clicking on the message item get this inner view focused instead of the outer Pressable.

2. For the inner view ^ set accessibilityRole="none" so we can have a way of distinction.

3. On index.html add a style that overrides the default focus-visible for elements that have no accessibility role (selector: [role=presentation]:focus-visible).

I need to explain the behaviour of focusable prop on View:

• focusable unset => tabIndex unset => not focusable not tabbable
• focusable set to false => tabIndex = -1 => focusable but not tabbable (We want this behavior)
• focusable set to true => tabIndex = 0 => focusable and tabbable

What alternative solutions did you explore? (Optional)

• Make ReportActionItem un-focusable (may require RNW changes)

[lschurr]

@sobitneupane are you able to take a look at these proposals?

[sobitneupane]

It looks like a valid issue to me. The focused message is highlighted on pressing any key which is not the expected behavior. I believe the expected behavior here is: the focused message should be highlighted only when pressing "Tab" key.
And It looks like the issue is present throughout the app.

I would like to have @pecanoro's opinion on this.

[pecanoro]

@sobitneupane Yes, you are right, it happens when you click all keys, not only Esc! So I agree we should fix that and focusable only on tab

[sobitneupane]

We are expecting updated proposals.

cc: @osofus @moezh @mdneyazahmad @s77rt

[mdneyazahmad]

@sobitneupane my proposal is aligned with the requirements.

[s77rt]

@sobitneupane @pecanoro

The focused message is highlighted on pressing any key which is not the expected behavior

we should fix that and focusable only on tab

Focusable elements behave that way, it's not a bug in E/App. Here is a demo, click the button then press any key. Personally I don't see the point of having the message item being focusable since there is no action on pressing the ENTER key.

[pecanoro]

I was testing on other apps (Slack) and websites and you are right, no elements besides the ones that we can interact with are focusable. Is there any reason why these were focusable in the first place?

[s77rt]

@pecanoro I'm not really sure but maybe it was unintended since the message item is a Pressable it's focusable by default.

[pecanoro]

@sobitneupane What are your thoughts, do you think we can remove the focusable property of it without causing any issues?

[sobitneupane]

I agree with @s77rt. My first thought was if we remove focus from ReportActionItem IOU Message and Attachments might not be focusable. But turns out it is not the case. Even when ReportActionItem is not focusable, individual pressable IOU Messages and Attachments will remain focusable.

[sobitneupane]

@mdneyazahmad Thanks for the proposal.

The ReportActionItem will remain focusable and will be focused by pressing Tab with your proposal. We are looking to completely get rid of focusable nature of ReportActionItem as it is not pressable (i.e, no action on pressing)

[sobitneupane]

@s77rt Thanks for the proposal.

Can you please elaborate your proposal.

Make ReportActionItem un-focusable i.e pass focusable={false}.

Can you please add an permalink where you are suggesting this change?

This ^ will require RNW changes here.

Do we need to make any change in react native web? What sort of changes are required?

Please update your proposal and let me know.

[mdneyazahmad]

@sobitneupane slack also focuses on message list item, but the navigation there is different than ours. When you press up arrow or down arrow, it focusses the corresponding message, in our case, it scrolls.

Selecting prev and next message with arrow is good for accessibility, otherwise it might need multiple Tab to skip messages. When the message list item gets focused, screen reader will read it.

focusable-message.mp4

[s77rt]

@sobitneupane

• The permlink where to add focusable={false}.
• RNW had a lot of changes regarding when to apply tabIndex. The easiest way (and the safest) is just to remove that attribute on render as this won't require any RNW changes.

Checking @mdneyazahmad uploaded video, I assume that we will implement arrow navigation for messages in the future, so completely removing focus from message items does not look like as a future proof solution. Updating my proposal based on that...

[s77rt]

Proposal

Updated

[sobitneupane]

I assume that we will implement arrow navigation for messages in the future, so completely removing focus from message items does not look like as a future proof solution.

@pecanoro Can you please confirm that? Are we planning to move with arrow navigation for messages in the near future?

I think even if we decide to do so, we can undo the changes. But for now I don't see any reason to keep ReportActionItem focusable.

[mdneyazahmad]

@sobitneupane can you please review my proposal? This will work for both keyboard and mouse users.

I think even if we decide to do so, we can undo the changes. But for now I don't see any reason to keep ReportActionItem focusable.

I believe, we should not remove something that is already working, and that we may decide to reintroduce in future.

[pecanoro]

@mdneyazahmad how did you manage to select the message on Slack? On the mac app, I can't seem to be able to focus it.

[s77rt]

@pecanoro

1. Have the name focused
2. Press Shift+TAB (navigate back)

[pecanoro]

That is incredibly odd so you can only focus it if you navigate back but if you navigate forward... That seems like a bug in Slack hahaha However, I tested on WhatsApp and they do let you focus on each message and then go back and down.

I am not sure if we will implement arrow navigation any time soon, I couldn't find any convos about it. Maybe we should open an issue for it since the other platforms seem to have it.

What I noticed is that, in other apps, when you type a letter, it will just write in the composer but in our case, it just focuses the element, which it's pretty odd. Honestly, the more I look into this, the more I think we should do nothing when it comes to the Esc letter and open another issue to actually fix the problem of typing letters, which should just type in the message composer instead.

[sobitneupane]

issue to actually fix the problem of typing letters, which should just type in the message composer instead.

I agree @pecanoro .

[pecanoro]

Ok, let's close this one. Thank you everyone for your proposals and sorry for having to close this one. I reported the composer problem in #expensify-bugs here so we can fix that instead.