search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.59k stars 2.92k forks source link

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

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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. Go to staging.new.expensify.com
  2. Login with any account
  3. Open a chat with user B
  4. Send a message
  5. Hover over Add Reaction option and select one
  6. 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

https://user-images.githubusercontent.com/93399543/224170559-4b02ec93-d791-4502-a5b8-ca6e8725302b.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 commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

kadiealexander commented 1 year ago

Reproduced on Mac OS in Chrome:

https://user-images.githubusercontent.com/59587260/224203086-7aa2b3e3-ecc6-49ae-afbc-9a98a68df185.mp4

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

bernhardoj commented 1 year ago

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.

https://github.com/Expensify/App/blob/b69b5f3f31d3054dd9eadb1a284d5efb490daf15/src/components/Tooltip/index.js#L203-L210

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

image

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: ``` ``` 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.

https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/components/Reactions/MiniQuickEmojiReactions.js#L68-L73

https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/components/BaseMiniContextMenuItem.js#L49-L51

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

getusha commented 1 year ago

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.

https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/components/Tooltip/index.js#L151-L159

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

Screenshot 2023-03-09 at 7 24 51 PM

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 commented 1 year ago

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

bernhardoj commented 1 year ago

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 https://github.com/Expensify/App/issues/15808 (there are several times this kind of issues getting reported).

parasharrajat commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

What alternative solutions did you explore? (Optional)

None

MelvinBot commented 1 year ago

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 commented 1 year ago

Not overdue, proposals still under review

s77rt commented 1 year ago

Proposal

Updated

pecanoro commented 1 year ago

@parasharrajat We got some updated proposals for you to review when you have a little bit of time.

parasharrajat commented 1 year ago

Thanks, I will check asap.

parasharrajat commented 1 year ago

Discussing internally.

parasharrajat commented 1 year ago

I think the following should be done for this issue.

  1. Make absolute as default. This seems like the right time to remove the absolute prop and add the wrapper only when children do not fit into the criteria for the absolute prop.
  2. Add focusable false to wrapper View in Tooltip.

All proposals are technically correct. I think we can go with @bernhardoj for this issue(Please check my comments above for the suggested approach). It is adding a cleanup of the absolute prop of Tooltip. And also talked about setting focusable false.

@getusha @s77rt Thanks for the proposals. I hope to see your contributions to other issues. There is plenty for all.

cc: @pecanoro

:ribbon: :eyes: :ribbon: C+ reviewed

MelvinBot commented 1 year ago

📣 @bernhardoj You have been assigned to this job by @pecanoro! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bernhardoj commented 1 year ago

I will open the PR in a few hours.

bernhardoj commented 1 year ago

PR is ready for review.

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

parasharrajat commented 1 year ago

PR is under review.

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

pecanoro commented 1 year ago

@MelvinBot Chill, PR on review!

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Huh... This is 4 days overdue. Who can take care of this?

parasharrajat commented 1 year ago

We are close to finding a solution instead of workarounds.

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Huh... This is 4 days overdue. Who can take care of this?

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

parasharrajat commented 1 year ago

PR under review.

MelvinBot commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Huh... This is 4 days overdue. Who can take care of this?

pecanoro commented 1 year ago

PR still being worked on.

melvin-bot[bot] commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

parasharrajat commented 1 year ago

PR might be merged tomorrow.

pecanoro commented 1 year ago

I have ni clue why Melvin keeps telling us the issue is overdue when it's not, crazy bots 😄

melvin-bot[bot] commented 1 year ago

@pecanoro, @parasharrajat, @bernhardoj, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

parasharrajat commented 1 year ago

Close to be merged.

parasharrajat commented 1 year ago

PR is awaiting review from @pecanoro on https://github.com/Expensify/App/pull/16052#issuecomment-1553373872. Please check a few conversations before and after to get the full context.

parasharrajat commented 1 year ago

Update: Original PR is merged https://github.com/Expensify/App/pull/16052. There was a regression which we fixed immediately at https://github.com/Expensify/App/pull/19659.

We found an issue https://github.com/Expensify/App/pull/16052#issuecomment-1560384042 during that PR which is also tackled at https://github.com/Expensify/App/pull/19659. IMO, this was not related to the original PR but the refactor made it eminent. Thus I think there be a separate compensation for this.

Thoughts @bernhardoj @pecanoro @kadiealexander ?

melvin-bot[bot] commented 1 year ago

⚠️ 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.

bernhardoj commented 1 year ago

That would be nice if the team agree, but I would accept any decision.

Btw, I found that the current isOverlappingTop miss the case for a text element. Here is how to reproduce:

  1. Send a multiline text link message, i.e. 
    [a
b](google.com)
  2. Hover over the link and notice the tooltip shows below it. It should show above.

I think this is because when we target the center y of a text, elementFromPoint will not return the text element itself, but the container of the text.

Screenshot 2023-05-27 at 16 52 21

My plan is to target both center of y and x. So, we will have 2 element, one using the centerX, one using the centerY. For it to be true, both overlapping condition of both element must be true. Here is the snippet.

show code ```diff diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index fb9e8ab83f..23b52618bb 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -60,26 +60,34 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid * @param {Number} tooltipTargetHeight - The height of the tooltip's target * @returns {Boolean} */ -function isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetHeight) { +function isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetWidth, tooltipTargetHeight) { if (typeof document.elementFromPoint !== 'function') { return false; } - // Use the vertical center of the target to prevent wrong element returned by elementFromPoint in case the target has a border radius. - const centerY = yOffset + tooltipTargetHeight / 2; - const element = document.elementFromPoint(xOffset, centerY); + // Use the vertical position of the target to prevent wrong element returned by elementFromPoint + // in case the target has a border radius or is a text. + const targetCenterX = xOffset + tooltipTargetWidth / 2; + const targetCenterY = yOffset + tooltipTargetHeight / 2; + const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset); + const elementAtTargetCenterY = document.elementFromPoint(xOffset, targetCenterY); const tooltipRef = (tooltip && tooltip.current) || tooltip; // Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself - if (!element || (tooltipRef && tooltipRef.contains(element))) { + if (!elementAtTargetCenterX || !elementAtTargetCenterY || (tooltipRef && tooltipRef.contains(elementAtTargetCenterX)) || tooltipRef.contains(elementAtTargetCenterY)) { return false; } - const rect = element.getBoundingClientRect(); + const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect(); + const rectAtTargetCenterY = elementAtTargetCenterY.getBoundingClientRect(); // Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element // and less than the bottom of the element - return yOffset > rect.top && yOffset < rect.bottom; + const isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom; + const isOverlappingAtTargetCenterY = yOffset > rectAtTargetCenterY.top && yOffset < rectAtTargetCenterY.bottom; + + // Return true only if both elementAtTargetCenterX and elementAtTargetCenterY overlap with another element + return isOverlappingAtTargetCenterX && isOverlappingAtTargetCenterY; } ```

If you agree, I will open another PR.

cc: @parasharrajat @pecanoro

bernhardoj commented 1 year ago

@parasharrajat @pecanoro Any thought on above comment?

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.19-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-06-05. :confetti_ball:

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.

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

melvin-bot[bot] commented 1 year ago

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: