-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon. #13146
Comments
Triggered auto assignment to @kevinksullivan ( |
ProposalIssueThis PR: FixRemove the diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a34549..af46f8154 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,6 @@ class Tooltip extends PureComponent {
containerStyles={this.props.containerStyles}
onHoverIn={this.showTooltip}
onHoverOut={this.hideTooltip}
- resetsOnClickOutside
>
{child}
</Hoverable>
ResultScreen.Recording.2022-11-30.at.5.43.34.AM.mov |
Proposal As App/src/components/Tooltip/index.js Line 192 in 1527fdc
App/src/components/ContextMenuItem.js Line 77 in 1527fdc
|
Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new. |
Job added to Upwork: https://www.upwork.com/jobs/~014886ff5b8f1db983 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Triggered auto assignment to @francoisl ( |
Proposaldiff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js
index 07b8a8741e..c17fa804b6 100644
--- a/src/components/Hoverable/hoverablePropTypes.js
+++ b/src/components/Hoverable/hoverablePropTypes.js
@@ -19,9 +19,6 @@ const propTypes = {
/** Function that executes when the mouse leaves the children. */
onHoverOut: PropTypes.func,
-
- // If the mouse clicks outside, should we dismiss hover?
- resetsOnClickOutside: PropTypes.bool,
};
const defaultProps = {
@@ -29,7 +26,6 @@ const defaultProps = {
containerStyles: [],
onHoverIn: () => {},
onHoverOut: () => {},
- resetsOnClickOutside: false,
};
export {
diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js
index f06ed56027..b4703d7cfc 100644
--- a/src/components/Hoverable/index.js
+++ b/src/components/Hoverable/index.js
@@ -16,13 +16,9 @@ class Hoverable extends Component {
};
this.wrapperView = null;
-
- this.resetHoverStateOnOutsideClick = this.resetHoverStateOnOutsideClick.bind(this);
}
componentDidMount() {
- document.addEventListener('mousedown', this.resetHoverStateOnOutsideClick);
-
// we like to Block the hover on touch devices but we keep it for Hybrid devices so
// following logic blocks hover on touch devices.
this.disableHover = () => {
@@ -38,7 +34,6 @@ class Hoverable extends Component {
}
componentWillUnmount() {
- document.removeEventListener('mousedown', this.resetHoverStateOnOutsideClick);
document.removeEventListener('touchstart', this.disableHover);
document.removeEventListener('touchmove', this.enableHover);
}
@@ -50,7 +45,8 @@ class Hoverable extends Component {
*/
setIsHovered(isHovered) {
if (isHovered !== this.state.isHovered && !(isHovered && this.hoverDisabled)) {
- this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
+ this.setState({isHovered});
+ isHovered ? this.props.onHoverIn() : this.props.onHoverOut();
}
// we reset the Hover block in case touchmove was not first after touctstart
@@ -59,28 +55,6 @@ class Hoverable extends Component {
}
}
- /**
- * If the user clicks outside this component, we want to make sure that the hover state is set to false.
- * There are some edge cases where the mouse can leave the component without the `onMouseLeave` event firing,
- * leaving this Hoverable in the incorrect state.
- * One such example is when a modal is opened while this component is hovered, and you click outside the component
- * to close the modal.
- *
- * @param {Object} event - A click event
- */
- resetHoverStateOnOutsideClick(event) {
- if (!this.state.isHovered) {
- return;
- }
- if (this.props.resetsOnClickOutside) {
- this.setIsHovered(false);
- return;
- }
- if (this.wrapperView && !this.wrapperView.contains(event.target)) {
- this.setIsHovered(false);
- }
- }
-
render() {
if (this.props.absolute && React.isValidElement(this.props.children)) {
return React.cloneElement(React.Children.only(this.props.children), {
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..af46f8154d 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,6 @@ class Tooltip extends PureComponent {
containerStyles={this.props.containerStyles}
onHoverIn={this.showTooltip}
onHoverOut={this.hideTooltip}
- resetsOnClickOutside
>
{child}
</Hoverable> DetailsFirst of all, in this GH issue the bug is #12025 (we are basically reopening the issue) The root issue is the use of In my proposal, I have basically reverted the previous PR and decoupled the functions calls to be called immediately and not in the callback. This however means that the functions may get called before the |
Thanks for the proposal everyone. I am somewhat inclined towards @s77rt's proposal but don't like the fact that it is hacky.
I couldn't confirm this statement. I did a quick search and couldn't find any related issues as well. Can you point me to some doc, issue where we can confirm. I am going to also tag @rushatgabhane @getusha to see if they've come across this when they solved the linked issue. @allroundexperts @Pujan92 I don't want to revert this without knowing the impact or causing regression. Do you have any strong arguments for your proposals? |
@mananjadhav Just disable the |
@mananjadhav New.Expensify.4.mp4Can anyone confirm this? it helps |
@getusha Are you testing on Safari? |
Testing on chrome. |
It's browser specific, my guess is that onMouseEnter is triggered again on Chrome but not on Safari.. But this is not the root issue. |
Right! makes sense i see onMouseEnter is supported on specific versions, so we can use onMouseOver which is supported on every browser. |
@getusha looks like you have a proposal |
Not exactly what I meant, the event is supported by Safari but not fired at the same conditions as Chrome |
This is a regression from #12572 , the proposed solution behave like onclick event , it does reset the hovered state on all click events even when pointer is inside the if (this.props.resetsOnClickOutside) {
this.setIsHovered(false);
return;
} I agree with @Ollyws #12025 (comment) and @s77rt #13146 (comment) for their proposal , the root issue for the other issue #12025 is that the state isn't updating (frozen) when navigation to other screens , the |
Looks like something related to 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 Feel free to drop a note in #expensify-open-source with any questions. |
Generally in Browsers, the Tooltip is triggered on mouse hover/over action. In our case, When the element is changed, the mouse is already on the element, on this situation mouse hovering/over the event(subsequent tooltip display) will trigger only if there is a mouse movement. otherwise, we have to explicitly trigger the hover event or tooltip display show program. In this solution, I have bubbled the click event to trigger the mouse hover event. |
Thanks for the proposals and explanations about the root issue, everyone. It looks like @s77rt has a good proposal. @mananjadhav do the new explanations answer the questions you had? @vijeeshin's proposal looks pretty straightforward too, though admittedly I don't understand how and why changing the event listener from |
@mananjadhav can you address in the |
@s77rt paid $12000 We need to fill this out too. @mollfpr can you tackle the first three? @kevinksullivan, can you do the last one? 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:
|
|
Compensating @mananjadhav 25% of the job price for their C+ work before they went OOO. This issue has close to 400 comments 😮 and there were multiple proposals that Manan reviewed. |
Thanks a lot @mallenexpensify for the consideration. |
@mallenexpensify can you plz help here for regression bonus, I cannot comment on linked issue #14121 |
Addressed #14121 (comment) |
Just to leave a note here 😄, this solution is temporary since it breaks accessibility (can confirm TAB, all clickable have two focus) Currently we are ignoring those issues but when we start to fix accessibility this solution is must to remove. |
@getusha That's wrong. All we have to do is pass |
@s77rt if it can be fixed why it's not fixed yet? |
@getusha As I said this has been addressed already, we opted not to do it. |
@s77rt Do you mind sharing the link to the discussion? |
This PR was reverted here as it was a temporary solution for this issue. |
We are not reverting the PR for that case, we are not really reverting as the main purpose of that PR is to remove In the linked PR we are actually reintroducing the bugs that we were trying to fix here:
The first bug will be resolved after Navigation Reboot so it's okay. But the second bug will persist. Results after that PR: Kooha-2023-04-15-06-52-52.mp4 |
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:
Copy to clipboard
but notCopied
after the text is copied.(Videos to compare Chrome and Safari are attached)
Expected Result:
Copied
tooltip should be shownActual Result:
Copied
tooltip after the text is copied doesn't show upWorkaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.33-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
copied-tooltip-not-showingup-safari.mov
copied-tooltip-not-showingup-chrome.mov
Recording.19.mp4
Expensify/Expensify Issue URL:
Issue reported by: @mananjadhav
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669714466881009
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: