[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

kbecciv · 2023-03-09T22:03:46Z

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:

Go to staging.new.expensify.com
Login with any account
Open a chat with user B
Send a message
Hover over Add Reaction option and select one
Start using Tab key

Expected Result:

Reaction emoji is highlighted once when using Tab key

Actual Result:

Reaction emoji is highlighted twice when using Tab key

Workaround:

Unknown

Platforms:

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

Version Number: 1.2.81.0

Reproducible in staging?: Yes

Reproducible in production?: No

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

Recording.2291.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/~010d30323d77b01052
Upwork Job ID: 1634009246856626176
Last Price Increase: 2023-03-10

MelvinBot · 2023-03-09T22:03:53Z

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

MelvinBot · 2023-03-09T22:03:57Z

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

kadiealexander · 2023-03-10T01:49:55Z

Reproduced on Mac OS in Chrome:

2023-03-10_14-48-48.copy.mp4

MelvinBot · 2023-03-10T01:52:20Z

Job added to Upwork: https://www.upwork.com/jobs/~010d30323d77b01052

MelvinBot · 2023-03-10T01:52:24Z

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

MelvinBot · 2023-03-10T01:52:34Z

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

MelvinBot · 2023-03-10T01:52:40Z

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

bernhardoj · 2023-03-10T03:08:46Z

Proposal

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

The emoji reaction is highlighted twice when navigating using Tab key.

What is the root cause of that problem?

Currently, we have 2 ways of rendering a Tooltip and it is controlled by the absolute prop. If absolute is false, the Tooltip children will be wrapped by a View, otherwise none. The children then get wrapped by a Hoverable component with the same absolute prop which works the same way.

App/src/components/Tooltip/index.js

Lines 203 to 210 in b69b5f3

    
           <Hoverable 
        
               absolute={this.props.absolute} 
        
               containerStyles={this.props.containerStyles} 
        
               onHoverIn={this.showTooltip} 
        
               onHoverOut={this.hideTooltip} 
        
           > 
        
               {child} 
        
           </Hoverable>

Let's look at the difference between setting the absolute to true and false. Let say we have this component.

<Tooltip>
    <Pressable />
</Tooltip>

The component above will be rendered like this:

absolute false

<View>                     <= View from Hoverable
    <View>                 <= View from Tooltip (focusable)
        <Pressable />   <= the children (focusable)
    <View />
</View>

absolute true

<Pressable />   <= the children (focusable)

As you can see above, rendering Tooltip with absolute false will render multiple view which one of them is focusable. This makes the keyboard tab will focus on the View first and then to the children (Pressable). With absolute true, all onMouseEnter, onMouseLeave, etc. will be attached to the children instead, which IMO is clearer. The old Tooltip component always wraps the children with a View, then the absolute prop is introduced by this PR to support Tooltip for an absolute positioned component, which I think should be the default way of rendering the Tooltip. However, absolute props will only work for a single child which makes total sense. If we have multiple children, then we obviously need a wrapper to handle the hovering.

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

As I said above, absolute true should be the default of Tooltip. It is much better so the hovering will really only happen on the area of the component that we wrap. For example, let say we have an avatar with a tooltip and absolute is set to false. The user will be able to hover over the edge of the tooltip component and the tooltip will show. With absolute to true, the tooltip will only show if we hover over the avatar component (the circle).

It sounds like I'm supporting to use the absolute prop, but I actually think that it's better to get rid of that prop. Why? Currently, our Tooltip handle the logic whether to wrap the Tooltip children with a View or not (which is basically the same as, if the children is an array, wrap it with a View, otherwise don't). At the end, all Tooltip children will be a single child. We can reduce the responsibility of our Tooltip component by removing the absolute prop, make sure the children are not multiple, and never wrap the children with a View. If we need to wrap the children with a View, do it inside the children, not inside the Tooltip component. This is the same as the TouchableOpacity component where we can't supply multiple children. This way, our Tooltip is only responsible for showing/hiding the Tooltip, nothing else. (this changes will also apply to Hoverable).

The above changes will also prevent such kind of this issue.

more info

The mentioned issue above easily happens if we wrap the Tooltip children with a View. For example:

<Tooltip>
    <Text style={{marginRight: 16}}/>
</Tooltip>

On screen, it looks fine. But because we have the margin right, we are able to hover over the right of the text by 16 and the tooltip will show and also it won't be center positioned. But with the changes suggested above, the issue will never happen because all the positioning and hovering logic is relative to the children.

Last thing, we shouldn't always set the focusable to true. (I assume we don't want a text with a tooltip is navigable through tab key.) Only set the focusable to true if the children have an onPress props, otherwise set it to false (Boolean(this.children.props.onPress)).

setting the focusable to false is actually the main point for the issue. The refactor is trying to make the component clearer and also prevent a Text to be navigable by tab key

I can open a draft PR if you want to see the changes needed.

What alternative solutions did you explore? (Optional)

(my previous main solution)
We should just set the focusable to false too at the emoji reaction inside mini context menu here.

App/src/components/Reactions/MiniQuickEmojiReactions.js

Lines 68 to 73 in 657b439

    
           <BaseMiniContextMenuItem 
        
               key={emoji.name} 
        
               isDelayButtonStateComplete={false} 
        
               tooltipText={`:${emoji.name}:`} 
        
               onPress={() => props.onEmojiSelected(emoji)} 
        
           >

App/src/components/BaseMiniContextMenuItem.js

Lines 49 to 51 in 657b439

    
           const BaseMiniContextMenuItem = props => ( 
        
               <Tooltip text={props.tooltipText}> 
        
                   <Pressable

So, we can pass like tooltipFocusable to false and pass it to the tooltip focusable

getusha · 2023-03-10T03:25:26Z

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

Context menu item is highlighted twice ( have two focusable elements )

What is the root cause of that problem?

When using the tooltip's "View" feature to encompass any clickable component, it can lead to two elements being in focus while navigating via the "tab" key - the clickable element that performs an action and the non-interactive element.

App/src/components/Tooltip/index.js

Lines 151 to 159 in 657b439

    
               <View 
        
                   ref={el => this.wrapperView = el} 
        
                   onBlur={this.hideTooltip} 
        
                   focusable={this.props.focusable} 
        
                   style={this.props.containerStyles} 
        
               > 
        
                   {this.props.children} 
        
               </View> 
        
           );

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

We should add absolute prop to the context menu items tooltip

I previously expressed my concerns on this matter here, and while it was acknowledged and addressed, it was not implemented in the PR.

What alternative solutions did you explore? (Optional)

parasharrajat · 2023-03-10T11:40:06Z

Thanks, everyone for the proposals. FYI, I have reviewed the proposals. But I need to confirm a few things before we move ahead on this issue.

Started with

[$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon. #13146 (comment)
Refactor tooltip #14048 (comment)
Pointing out the regression.
Finding a solution to these scenes where we miss early detection of regression even after a documented process.

bernhardoj · 2023-03-11T01:41:19Z

I think this is the best time to do a little refactor to the Tooltip component.

The reason behind absolute is to have the tooltip hover area the same size as the component, but currently it will only work for a single child view, if there are multiple children, we need a wrapper which is totally makes sense. Here are some changes that we need:

We should just get rid of the absolute property and force every tooltip to only have a single child, similar to TouchableOpactiy. So, every tooltip that we have now that have multiple children need to be wrapped by a View (I only found one, AvatarWithIndicator ).
Set the focusable property to true only if the child is either TouchableOpacity, TouchableWithoutFeedback, & Pressable, otherwise set it to false. We don't want a touchable component to have focusable set to false.

This will solve all the current (double tab) issue and future issues like this one #15808 (there are several times this kind of issues getting reported).

parasharrajat · 2023-03-11T06:19:51Z

@bernhardoj What and why is missing from proposal? I mean you didn't explain what are you trying to achieve in second paragraph and why.

Why should we get rid of absolute prop?
...
....
Etc

Do you mind following the format if you are proposing?

bernhardoj · 2023-03-11T12:43:03Z

I have updated my proposal above to include the refactor explanation. I'm a little bit struggling to explain it, so if something is not clear/missing, please ask it 😄.

s77rt · 2023-03-12T07:09:31Z

Proposal

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

Context Menu Items are Tabbable twice.

What is the root cause of that problem?

We have two nested focusable elements, the Pressable and the Tooltip. The Tooltip is made focusable intentionally here and this bug was addressed already but decided to be ignored as keyboard navigation was not a concern.

More context: I'm the author of the linked PR and the sole purpose of having the Tooltip focusable is so the blur event from its children bubbles up so we can hide it. That being said the Tooltip need to be focusable but not necessary tabbable i.e. tabIndex does not have to be 0 setting it to -1 is enough.

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

Set tabIndex of Tooltip to -1. This can be achieved by setting focusable to false here and here.

How focusable prop behaves on RNW (View):

focusable unset => tabIndex unset (not focusable and not tabbable)
focusable set to false => tabIndex = -1 (focusable but not tabbable)
focusable set to true => tabIndex = 0 (focusable and tabbable)

What alternative solutions did you explore? (Optional)

None

MelvinBot · 2023-03-12T07:09:35Z

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.

pecanoro · 2023-03-13T15:00:24Z

Not overdue, proposals still under review

melvin-bot · 2023-06-29T20:00:37Z

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:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Refactor tooltip #14048
[@parasharrajat] 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: Refactor tooltip #14048 (comment)
[@parasharrajat] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1689751878178739
[@parasharrajat] Determine if we should create a regression test for this bug.
[@parasharrajat] 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.
[@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/303724

parasharrajat · 2023-06-30T05:55:23Z

Last PR merged.

kadiealexander · 2023-07-05T23:32:29Z

@parasharrajat before I issue an Upworks contract, are you being paid via NewDot now?

I've issued an Upworks contract to @bernhardoj :)

bernhardoj · 2023-07-06T11:43:35Z

@kadiealexander I just realized there are 2 contracts on upwork

1

2

parasharrajat · 2023-07-06T14:46:28Z

Please hold my payments for now.

kadiealexander · 2023-07-07T01:44:22Z

@bernhardoj super weird! I've closed one and paid the other.

@parasharrajat, could you please complete this checklist while waiting for your NewDot payment?

bernhardoj · 2023-07-07T02:25:33Z

@kadiealexander thanks!

pecanoro · 2023-07-18T00:42:29Z

@parasharrajat Please, fill out the list whenever you have a few minutes so we can finally close this one!

parasharrajat · 2023-07-19T07:38:56Z

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:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Refactor tooltip #14048
[@parasharrajat] 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: Refactor tooltip #14048 (comment)
[@parasharrajat] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1689751878178739
[@parasharrajat] Determine if we should create a regression test for this bug. Yes
[@parasharrajat] 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.

Regression Test Steps

Send a message
Hover over the message to see the mini context menu.
Select a quick reaction from the mini menu.
Press the Tab key.
Verify that each tab key selects the next action only once and the user doesn't need to press Tab twice on a single action to move to the next action.

Do you agree 👍 or 👎 ?

pecanoro · 2023-07-27T20:35:08Z

@kadiealexander All yours!

kadiealexander · 2023-07-28T04:55:51Z

Completed Bug0 checklist: #15796 (comment)

Payouts due:

Issue Reporter: n/a
Contributor: $1000 @bernhardoj (Paid via Upwork)
Contributor+: $1000 @parasharrajat (Paid via Newdot)

Eligible for 50% #urgency bonus? No

Upwork job is here.

kadiealexander · 2023-07-28T04:56:13Z

All done, will shift to weekly while we wait for Newdot payment to be issued.

parasharrajat · 2023-08-01T08:46:12Z

Payment Requested.

JmillsExpensify · 2023-08-03T03:11:46Z

Reviewed details for @parasharrajat. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.

parasharrajat · 2023-08-04T00:31:32Z

We are good to close this @kadiealexander

melvin-bot · 2023-12-02T08:24:41Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 9, 2023

melvin-bot bot locked and limited conversation to collaborators Mar 9, 2023

melvin-bot bot assigned kadiealexander Mar 9, 2023

kadiealexander added the External Added to denote the issue can be worked on by a contributor label Mar 10, 2023

melvin-bot bot unlocked this conversation Mar 10, 2023

melvin-bot bot changed the title ~~Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key~~ [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key Mar 10, 2023

melvin-bot bot assigned parasharrajat Mar 10, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 10, 2023

melvin-bot bot assigned pecanoro Mar 10, 2023

This was referenced Mar 10, 2023

Refactor tooltip #14048

Merged

[$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon. #13146

Closed

melvin-bot bot added the Overdue label Mar 13, 2023

melvin-bot bot removed the Overdue label Mar 13, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 5, 2023

kadiealexander changed the title ~~[HOLD for payment 2023-07-06] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key~~ [HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key Jul 7, 2023

kadiealexander added Weekly KSv2 and removed Daily KSv2 labels Jul 7, 2023

melvin-bot bot added the Overdue label Jul 17, 2023

melvin-bot bot removed the Overdue label Jul 18, 2023

melvin-bot bot added the Overdue label Jul 27, 2023

melvin-bot bot removed the Overdue label Jul 27, 2023

pecanoro added Daily KSv2 and removed Weekly KSv2 labels Jul 27, 2023

kadiealexander added Weekly KSv2 and removed Daily KSv2 labels Jul 28, 2023

kadiealexander closed this as completed Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

kbecciv commented Mar 9, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Mar 9, 2023

MelvinBot commented Mar 9, 2023 •

edited by kadiealexander

Loading

kadiealexander commented Mar 10, 2023 •

edited

Loading

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

bernhardoj commented Mar 10, 2023 •

edited

Loading

getusha commented Mar 10, 2023 •

edited

Loading

parasharrajat commented Mar 10, 2023 •

edited

Loading

bernhardoj commented Mar 11, 2023

parasharrajat commented Mar 11, 2023

bernhardoj commented Mar 11, 2023

s77rt commented Mar 12, 2023 •

edited

Loading

MelvinBot commented Mar 12, 2023

pecanoro commented Mar 13, 2023

melvin-bot bot commented Jun 29, 2023 •

edited by kadiealexander

Loading

parasharrajat commented Jun 30, 2023

kadiealexander commented Jul 5, 2023 •

edited

Loading

bernhardoj commented Jul 6, 2023

parasharrajat commented Jul 6, 2023

kadiealexander commented Jul 7, 2023

bernhardoj commented Jul 7, 2023

pecanoro commented Jul 18, 2023

parasharrajat commented Jul 19, 2023

pecanoro commented Jul 27, 2023

kadiealexander commented Jul 28, 2023 •

edited

Loading

kadiealexander commented Jul 28, 2023

parasharrajat commented Aug 1, 2023

JmillsExpensify commented Aug 3, 2023

parasharrajat commented Aug 4, 2023

melvin-bot bot commented Dec 2, 2023

[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

Comments

kbecciv commented Mar 9, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

MelvinBot commented Mar 9, 2023

MelvinBot commented Mar 9, 2023 • edited by kadiealexander Loading

Bug0 Triage Checklist (Main S/O)

kadiealexander commented Mar 10, 2023 • edited Loading

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

MelvinBot commented Mar 10, 2023

bernhardoj commented Mar 10, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

getusha commented Mar 10, 2023 • edited Loading

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

parasharrajat commented Mar 10, 2023 • edited Loading

bernhardoj commented Mar 11, 2023

parasharrajat commented Mar 11, 2023

bernhardoj commented Mar 11, 2023

s77rt commented Mar 12, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

MelvinBot commented Mar 12, 2023

pecanoro commented Mar 13, 2023

melvin-bot bot commented Jun 29, 2023 • edited by kadiealexander Loading

parasharrajat commented Jun 30, 2023

kadiealexander commented Jul 5, 2023 • edited Loading

bernhardoj commented Jul 6, 2023

parasharrajat commented Jul 6, 2023

kadiealexander commented Jul 7, 2023

bernhardoj commented Jul 7, 2023

pecanoro commented Jul 18, 2023

parasharrajat commented Jul 19, 2023

Regression Test Steps

pecanoro commented Jul 27, 2023

kadiealexander commented Jul 28, 2023 • edited Loading

kadiealexander commented Jul 28, 2023

parasharrajat commented Aug 1, 2023

JmillsExpensify commented Aug 3, 2023

parasharrajat commented Aug 4, 2023

melvin-bot bot commented Dec 2, 2023

kbecciv commented Mar 9, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Mar 9, 2023 •

edited by kadiealexander

Loading

kadiealexander commented Mar 10, 2023 •

edited

Loading

bernhardoj commented Mar 10, 2023 •

edited

Loading

getusha commented Mar 10, 2023 •

edited

Loading

parasharrajat commented Mar 10, 2023 •

edited

Loading

s77rt commented Mar 12, 2023 •

edited

Loading

melvin-bot bot commented Jun 29, 2023 •

edited by kadiealexander

Loading

kadiealexander commented Jul 5, 2023 •

edited

Loading

kadiealexander commented Jul 28, 2023 •

edited

Loading