melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
allroundexperts
commented
1 year ago This PR:
https://github.com/Expensify/App/pull/12572
introduced resetsOnClickOutside to the Hoverable component. This causes this issue on Safari.
Remove the resetsOnClickOutside property. It's not really checking if the click is outside. It resets the state nonetheless.
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>
Pujan92
commented
1 year ago Proposal
As resetsOnClickOutside added to solve this issue, we can pass value false for our use case(for copy actions)
resetsOnClickOutside={!_.isUndefined(this.props.resetsOnClickOutside) ? this.props.resetsOnClickOutside : true}<Tooltip text={text} resetsOnClickOutside={false}>
melvin-bot[bot]
commented
1 year ago Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~014886ff5b8f1db983
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @francoisl (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
s77rt
commented
1 year ago diff --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>First of all, in this GH issue the bug is https://github.com/Expensify/App/issues/12025 (we are basically reopening the issue) The applied fix https://github.com/Expensify/App/pull/12572 is hacky and causes this problem. Why hacky? because it does not address the root issue and the proposed fix can be bypassed by pressing enter instead of a mouse click.
The root issue is the use of <Freeze /> where the callback of setState is suspended thus the this.props.onHoverOut is not called and the tooltip become stuck.
Ideally and the simplest solution is to exclude <Hoverable /> Unfortunately this does not seem as a feature that exists and I don't think we should get rid of <Freeze /> either, so we have to fix it in another approach.
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 isHovered state is actually updated, given that this is not a serious matter I'd say that's an okay price to pay, not ideal but better.
mananjadhav
commented
1 year ago Thanks for the proposal everyone. I am somewhat inclined towards @s77rt's proposal but don't like the fact that it is hacky.
The root issue is the use of
where the callback of setState is suspended
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?
s77rt
commented
1 year ago @mananjadhav Just disable the <Freeze /> by passing prop freeze={false} You will notice it will work just fine.
Unfortunately I can't find a clear statement about the setState callback, that's something I deducted
Docs may be worth checking:
https://github.com/software-mansion/react-freeze#when-component-subtree-is-frozen-what-happens-to-state-changes-that-are-executed-on-that-subtree
https://beta.reactjs.org/apis/react/Suspense
Pujan92
commented
1 year ago @Pujan92 I don't want to revert this without knowing the impact or causing regression. Do you have any strong arguments for your proposals?
In my proposal, I am conveying to pass value to the resetsOnClickOutside property for our use case and I think it isn't related to revert.
getusha
commented
1 year ago @mananjadhav After some inspection i have confirmed that this only happens when we click it on the edge/corners of the clickable.
Can anyone confirm this? it helps
s77rt
commented
1 year ago @getusha Are you testing on Safari?
getusha
commented
1 year ago Testing on chrome. IMO It can't be platform specific issue.
s77rt
commented
1 year ago 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.
getusha
commented
1 year ago Right! makes sense i see onMouseEnter is supported on specific versions, so we can use onMouseOver which is supported on every browser.
rushatgabhane
commented
1 year ago @getusha looks like you have a proposal
s77rt
commented
1 year ago Right! makes sense i see onMouseEnter is supported on specific versions, so we can use onMouseOver which is supported on every browser.
Not exactly what I meant, the event is supported by Safari but not fired at the same conditions as Chrome
fedirjh
commented
1 year ago 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 Hoverable , and I think that's wrong
if (this.props.resetsOnClickOutside) {
this.setIsHovered(false);
return;
}I agree with @Ollyws https://github.com/Expensify/App/issues/12025#issuecomment-1286890133 and @s77rt https://github.com/Expensify/App/issues/13146#issuecomment-1332734440 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 onMouseLeave event is firing correclty but the state isn't updating nor triggering the callback to hide the tooltip
melvin-bot[bot]
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.
vijeeshin
commented
1 year ago Proposal
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.
francoisl
commented
1 year ago 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 mousedown to click solves the issue. Is it because Safari doesn't fire mousedown the same way as other browsers?
vijeeshin
commented
1 year ago @francoisl No, actually issue is there in all browser. In any browser you click and hold the mouse(any movement on mouse will show tooltip, otherwise not) on same position it will happens. I tried all the mouse event actions, 'click' does the job. Tooltip need some event to trigger. Safari event trigger is more sensitive, thats why its more visible in that, other browsers too have this problem but less visible.
getusha
commented
1 year ago I like @vijeeshin 's approach, can you implement it and attach video results on your proposal?
vijeeshin
commented
1 year ago @getusha Here is the proof
https://user-images.githubusercontent.com/772974/205429817-7b166c62-0779-48e1-92b6-7044f825cd90.mov
getusha
commented
1 year ago @vijeeshin it breaks everything.
vijeeshin
commented
1 year ago @getusha Will check that
s77rt
commented
1 year ago There is no need of resetHoverStateOnOutsideClick This must be removed, it's only a workaround.
The real problem is on the fact that the element is suspended and callback setState does not get called.
getusha
commented
1 year ago @s77rt after applying the solution The Tooltip is not dismissed unless we move the mouse, IMO it is new issue which is equivalent to this.
s77rt
commented
1 year ago @getusha Yes indeed, I have noticed that, I had an idea to solve this already. Will get back to this soon
vijeeshin
commented
1 year ago Here is another work around
in ContextMenuItem.js
-77 <Tooltip text={text}>
+77 <Tooltip text={text} isDelayButtonStateComplete={this.props.isDelayButtonStateComplete}>
in Tooltip.js
-40 componentDidUpdate(prevProps) {
if (this.props.windowWidth === prevProps.windowWidth && this.props.windowHeight === prevProps.windowHeight) {
return;
}
this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition());
this.getWrapperPositionPromise.promise
.then(({x, y}) => this.setState({xOffset: x, yOffset: y}));
}+40 componentDidUpdate(prevProps) {
if(prevProps.isDelayButtonStateComplete !== this.props.isDelayButtonStateComplete) {
this.showTooltip();
setTimeout(()=>{
this.hideTooltip();
},1000);
}
if (this.props.windowWidth === prevProps.windowWidth && this.props.windowHeight === prevProps.windowHeight) {
return;
}
this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition());
this.getWrapperPositionPromise.promise
.then(({x, y}) => this.setState({xOffset: x, yOffset: y}));
}https://user-images.githubusercontent.com/772974/205440704-d2b394e8-1d5c-4888-bb2f-e9ca537100d7.mov
https://user-images.githubusercontent.com/772974/205440748-1a35d76a-72eb-4d0e-80ac-8e5dab7a535a.mov
vijeeshin
commented
1 year ago Keep mousedown event as earlier. No need to change that to click event
s77rt
commented
1 year ago diff --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>
diff --git a/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js b/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
index 433620deec..aa9591548f 100644
--- a/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
+++ b/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
@@ -13,9 +13,9 @@ export default (
},
},
) => {
- const translateX = Animated.multiply(progress.interpolate({
+ const right = Animated.multiply(progress.interpolate({
inputRange: [0, 1],
- outputRange: [isSmallScreenWidth ? screen.width : variables.sideBarWidth, 0],
+ outputRange: [isSmallScreenWidth ? -screen.width : -variables.sideBarWidth, 0],
extrapolate: 'clamp',
}), inverted);
@@ -25,7 +25,7 @@ export default (
if (isFullScreenModal && !isSmallScreenWidth) {
cardStyle.opacity = opacity;
} else {
- cardStyle.transform = [{translateX}];
+ cardStyle.right = right;
}
return ({Fixed this issue https://github.com/Expensify/App/issues/13146#issuecomment-1336120352
The issue is that the tooltip stays open upon opening a modal till you move the mouse as shown in @getusha 's video.
The reason for that is the use of CSS translate as it does not fire mouse events (mouseleave in this case) I have found a very similar issue to demonstrate and proof my findings: https://stackoverflow.com/questions/57507374/mouseleave-event-is-not-fired-when-element-transformed
Luckily the modals use a fixed position and we can easily swap transform translate with right and that's exactly what I did.
Now the only downside of my proposal is that as stated before, the callback functions onHoverIn and onHoverOut may be called before isHovered state is actually updated.
mananjadhav
commented
1 year ago Thanks all for posting the proposals. Please allow me a day to go through them.
kevinksullivan
commented
1 year ago Hi @mananjadhav , do you have an update on the reviews?
melvin-bot[bot]
commented
1 year ago @francoisl, @mananjadhav, @kevinksullivan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
francoisl
commented
1 year ago @mananjadhav did you have time to review the proposals? If needed, we can also bring this to Slack for second opinions.
mananjadhav
commented
1 year ago hi @francoisl I am going to do that today.
jatinsonijs
commented
1 year ago Proposal
Plz Ignore not working for safari
Tooltips are available for web only. We can add key on Hoverable into tooltip component so that other platforms not impact.
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..2824703dd9 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -190,6 +190,7 @@ class Tooltip extends PureComponent {
onHoverIn={this.showTooltip}
onHoverOut={this.hideTooltip}
resetsOnClickOutside
+ key={this.props.text}
>
{child}
</Hoverable>@jatinsonijs we have discussed this proposal before for another issue related to tooltip https://github.com/Expensify/App/issues/11486#issuecomment-1286885163
I don't think key={this.props.text} is a valid solution. This completely re-renders tooltip and sometimes breaks animation while changing text.
jatinsonijs
commented
1 year ago What about this one @fedirjh this one will not impact tooltip it will apply on Hoverable component
Tooltips are available for web only. We can add key into tooltip component so that other platforms not impact.
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 8d3a345492..2824703dd9 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -190,6 +190,7 @@ class Tooltip extends PureComponent { onHoverIn={this.showTooltip} onHoverOut={this.hideTooltip} resetsOnClickOutside + key={this.props.text} > {child} </Hoverable>
getusha
commented
1 year ago @jatinsonijs can you check if this glitch https://github.com/Expensify/App/issues/11486#issuecomment-1286888933 exist after applying your updated solution
jatinsonijs
commented
1 year ago @getusha actually its already happening no matter if my solution is applied or not in the Reimburse expenses screen which was showed in video of linked issue. And yes my first solution is sometime breaking animation. But second one working fine on another places.
fedirjh
commented
1 year ago @jatinsonijs this is the expected behavior (that's how it was working before), as you can see the tooltip is rendered once and only the text changes on click , so we don't have to re-render any component here , instead we have identify the root cause that bring this issue and fix it .
jatinsonijs
commented
1 year ago Yes @fedirjh I already added the root cause. this issue only happen on edges outside of icon. when your cursor on icon (success icon green one) its already working fine no need to change. But if cursor is little outside its create issue bcz there is no change trigger (under the cursor view is still same so onHoverIn not trigger again). its not happen earlier may be due to below condition was not there
if (this.props.resetsOnClickOutside) { this.setIsHovered(false); return; }
But i think it's also required for some cases. like below if we remove it new issues will occur
And this function (resetHoverStateOnOutsideClick) name should be corrected its not just outside click. It is click anywhere type if resetsOnClickOutside is true.
mananjadhav
commented
1 year ago This got missed out from the tracking. I am on this today. Quick note @jatinsonijs Issue is more like it isn't consistent between Safari and Chrome. Are you suggesting that if the cursor is on the green tick it will show up on Safari too?
jatinsonijs
commented
1 year ago Yes @mananjadhav right creating problem on safari even when cursor is on icon and on chrome (edges).
Proposal
diff --git a/src/components/ContextMenuItem.js b/src/components/ContextMenuItem.js
index c87fc5e9b4..458019bb8b 100644
--- a/src/components/ContextMenuItem.js
+++ b/src/components/ContextMenuItem.js
@@ -75,7 +75,7 @@ class ContextMenuItem extends Component {
return (
this.props.isMini
? (
- <Tooltip text={text}>
+ <Tooltip text={text} dismissOnClick={false}>
<Pressable
focusable
accessibilityLabel={text}
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..1f35d55f04 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,7 @@ class Tooltip extends PureComponent {
containerStyles={this.props.containerStyles}
onHoverIn={this.showTooltip}
onHoverOut={this.hideTooltip}
- resetsOnClickOutside
+ resetsOnClickOutside={this.props.dismissOnClick}
>
{child}
</Hoverable>
diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js
index 627f1c0171..03f1707320 100644
--- a/src/components/Tooltip/tooltipPropTypes.js
+++ b/src/components/Tooltip/tooltipPropTypes.js
@@ -32,6 +32,9 @@ const propTypes = {
/** Maximum number of lines to show in tooltip */
numberOfLines: PropTypes.number,
+
+ /** Should hide tooltip on inside (hoverable) area press */
+ dismissOnClick: PropTypes.bool,
};
const defaultProps = {
@@ -42,6 +45,7 @@ const defaultProps = {
text: '',
maxWidth: variables.sideBarWidth,
numberOfLines: CONST.TOOLTIP_MAX_LINES,
+ dismissOnClick: true,
};
export {I think resetsOnClickOutside not correct name for tooltip bcz its not clear to code reader why tooltip should be reset. If we pass false in it why it should not reset on outside click but actually its works different in code.
We can use new props dismissOnClick or dissmissOnInsideClick. And we can also update prop name on Hoverable also resetOnClick or resetOnInsideClick it's using only for Tooltip.
Reason why it's behaving different on chrome and safari is still not clear. Even on Safari it's not working with key prop.
jatinsonijs
commented
1 year ago @mananjadhav I think something related to safari internal behaviour and React
https://reactjs.org/docs/events.html
The onMouseEnter and onMouseLeave events propagate from the element being left to the one being entered instead of ordinary bubbling and do not have a capture phase.
I have created basic example to check how its working in newly created project.
https://codesandbox.io/s/sad-monad-jegjrq
Safari not able to detect element changes until we move mouse.
melvin-bot[bot]
commented
1 year ago @francoisl, @mananjadhav, @kevinksullivan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
vijeeshin
commented
1 year ago componentDidMount() {
document.addEventListener('keydown', 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 = () => {
this.hoverDisabled = true;
};
this.enableHover = () => {
this.hoverDisabled = false;
};
document.addEventListener('touchstart', this.disableHover);
// Remember Touchend fires before `mouse` events so we have to use alternative.
document.addEventListener('touchmove', this.enableHover);
}
componentWillUnmount() {
document.removeEventListener('keydown', this.resetHoverStateOnOutsideClick);
document.removeEventListener('touchstart', this.disableHover);
document.removeEventListener('touchmove', this.enableHover);
}keydown will trigger 3 events at once
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 clipboardbut notCopiedafter the text is copied. (Videos to compare Chrome and Safari are attached)Expected Result:
Copiedtooltip should be shownActual Result:
Copiedtooltip 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:
https://user-images.githubusercontent.com/43996225/204621989-5fad386a-c68b-4480-942b-158c8a71e4fe.mov
https://user-images.githubusercontent.com/43996225/204621991-264ae997-d575-4e1e-8cac-f35b5ccd3b88.mov
https://user-images.githubusercontent.com/43996225/204622210-9f209d52-af51-4250-a377-2cb5ed25e153.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