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.49k stars 2.84k forks source link

[HOLD for payment 2023-06-30] [$1000] Jittery transition of tooltip when the inner content changes #17555

Closed kavimuru closed 1 year ago

kavimuru 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. Navigate to any chat > Click on chat header
  2. Click on the clipboard icon at the details page
  3. Keep the cursor at clipboard and wait for the tooltip to change again to "Copy to clipboard"

Expected Result:

The tooltip transition is Jittery when the tooltip text changes.

Actual Result:

You shouldn't see any shaking/jitter in the transition.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.1 Reproducible in staging?: y Reproducible in production?: y 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/43996225/232654771-8182efd6-d634-40e9-b1b8-fc5f8a4d8efd.mp4

https://user-images.githubusercontent.com/43996225/232654786-3a83e4f3-19b9-4403-8df5-a8f864b32925.mov

Expensify/Expensify Issue URL: Issue reported by: @dukenv0307 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681726366513289

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b7a7c818708f281d
  • Upwork Job ID: 1651051677043404800
  • Last Price Increase: 2023-04-26
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

MelvinBot commented 1 year ago

@michaelhaxhiu Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot commented 1 year ago

@michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01b7a7c818708f281d

MelvinBot commented 1 year ago

Current assignee @michaelhaxhiu 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 - @mananjadhav (External)

MelvinBot commented 1 year ago

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

michaelhaxhiu commented 1 year ago

Going external, this one is reproducible & valid!

alitoshmatov commented 1 year ago

Might be helpful I think issue is caused here: https://github.com/Expensify/App/blob/57388640eb97255c0089c4c316d13747b6a1dd34/src/components/Tooltip/TooltipRenderedOnPageBody.js#L101

When tooltip content changes we let the width be determined itself by setting width to undefined and immediately after that recalculate the width and provide it manually.

bernhardoj commented 1 year ago

Proposal

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

Tooltip flickers when the content of the tooltip changes.

What is the root cause of that problem?

If we slow down the video, we can see that when the tooltip content changes, the position of the tooltip is late to update.

image image

Currently, we position the tooltip based on the component width and the tooltip width itself. When the tooltip content changes, the onLayout callback will be called to update the new tooltip width. https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/components/Tooltip/TooltipRenderedOnPageBody.js#L177-L178 https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/components/Tooltip/TooltipRenderedOnPageBody.js#L119-L124

As we can see, we will update the state inside onLayout callback, which could be late because the state is updated after the text content is updated.

content updated -> rendered -> onLayout -> width/height updated -> rendered

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

To calculate the size earlier, we can use useLayoutEffect (even the doc example uses tooltip as an example 🤣) which means we need to migrate the component to functional. Here is what we need to do:

  1. Migrate TooltipRenderedOnPageBody to functional component
  2. Add useLayoutEffect with props.text as the dependency array. Inside the hook: 2a. Get the width and height from getBoundingClientRect 2b. If both height & width is not 0, set the tooltip width and height
here is how it looks in functional component ``` const TooltipRenderedOnPageBody2 = (props) => { const [tooltipWidth, setTooltipWidth] = React.useState(0); const [tooltipHeight, setTooltipHeight] = React.useState(0); const [tooltipContentWidth , setTooltipContentWidth] = React.useState(undefined); const contentRef = React.useRef(); const wrapper = React.useRef(); const updateTooltipContentWidth = () => { setTooltipContentWidth(contentRef.current.offsetWidth); } React.useLayoutEffect(() => { const rect = wrapper.current.getBoundingClientRect(); if (rect.width !== 0 && rect.height !== 0) { setTooltipWidth(rect.width); setTooltipHeight(rect.height); } }, [props.text]); React.useEffect(() => { // We need to re-calculate the tooltipContentWidth if it is greater than maxWidth. // So that the wrapperWidth still be updated again with correct value if (tooltipContentWidth === undefined || tooltipContentWidth > props.maxWidth) { updateTooltipContentWidth(); } }, [tooltipContentWidth]) React.useEffect(() => { // Reset the tooltip text width to 0 so that we can measure it again. // eslint-disable-next-line react/no-did-update-set-state setTooltipContentWidth(undefined); }, [props.text, props.renderTooltipContent]); const { animationStyle, tooltipWrapperStyle, tooltipTextStyle, pointerWrapperStyle, pointerStyle, } = getTooltipStyles( props.animation, props.windowWidth, props.xOffset, props.yOffset, props.wrapperWidth, props.wrapperHeight, props.maxWidth, tooltipWidth, tooltipHeight, tooltipContentWidth, props.shiftHorizontal, props.shiftVertical, ); let content; if (props.renderTooltipContent) { content = ( {props.renderTooltipContent()} ); } else { content = ( {props.text} ); } return ReactDOM.createPortal( { setTooltipWidth(e.nativeEvent.layout.width); setTooltipHeight(e.nativeEvent.layout.height); }} style={[tooltipWrapperStyle, animationStyle]} > {content} , document.querySelector('body'), ); } ```

What alternative solutions did you explore? (Optional)

Previously the main proposal

Instead of relying on a fixed unit (tooltip width), we can use a percentage value to position the tooltip.

  1. Remove tooltipWidth calculation from here https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L187

  2. Add translateX -50% here (translateX should be put before the scale) https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L139-L146

  3. Change the left to 50% and add transform: [{translateX: '-50%'}, {translateX: -horizontalShift}] here https://github.com/Expensify/App/blob/6fb096948d862ee008445f809cff3ecd51b36f55/src/styles/getTooltipStyles.js#L222

https://user-images.githubusercontent.com/50919443/234776646-2ef430b4-0b37-4998-a6c1-7b0e461c2401.mov

mananjadhav commented 1 year ago

I think @bernhardoj proposal looks good. We'll just have to check any regression absolute tooltips.

@amyevans all youts.

bernhardoj commented 1 year ago

Just tested that my proposal won't cover the case when the tooltip is at the edge of the screen. That is because we still use tooltipWidth to calculate horizontalShift

https://user-images.githubusercontent.com/50919443/234837345-570f6380-d70b-4935-9be7-c44fab59b48f.mov

bernhardoj commented 1 year ago

Updated my proposal to cover all cases with useLayoutEffect

michaelhaxhiu commented 1 year ago

Niiiice one @bernhardoj thanks for being proactive on that. Let's get final 👍 from @amyevans and I'll proceed with assigning you.

amyevans commented 1 year ago

Proposal looks great, assigning you @bernhardoj 🎉

We should be able to close out https://github.com/Expensify/App/issues/16211 as a byproduct of the bug fix as well 💪, 2 🐦 1 🥌

MelvinBot commented 1 year ago

📣 @bernhardoj You have been assigned to this job by @amyevans! 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

Will open the PR soon

bernhardoj commented 1 year ago

PR is ready.

michaelhaxhiu commented 1 year ago

Thanks for the fast work @bernhardoj, next step is to get this reviewed by @mananjadhav

mananjadhav commented 1 year ago

I've started with the review and going through the updated changes by @bernhardoj. Relatively larger than expected as we're changing the class component to functional component.

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.12-0 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-05-16. :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:

mananjadhav commented 1 year ago

@amyevans I couldn't pinpoint to the exact PR causing this. We've been using measureTooltip since a long time. I am suspecting this PR must've caused it https://github.com/Expensify/App/pull/8494 but I couldn't confirm by running the specific branch.

Also, considering this isn't going to change that often, and we've moved to a functional component, I don't think we need a regression test here. @amyevans @michaelhaxhiu if you still feel we should add one, here's a proposal.

Regression Test Proposal

  1. Open any chat
  2. Click on their profile image of any participant
  3. Hover over the Copy icon next to the user email/phone number
  4. Verify a tooltip shows with a text "Copy to clipboard"
  5. Click the Copy icon
  6. Verify the tooltip text changes to "Copied"
  7. Wait for a while and verify the text changes back to "Copy to clipboard"
  8. Verify that the tooltip doesn't reposition when the text reverts.
  9. Repeat the steps 5-8 few times to verify step 9 for each iteration.
bernhardoj commented 1 year ago

I think this is not a regression as the initial implementation of the Tooltip has been using onLayout.

Btw, we have a regression (my bad) here https://github.com/Expensify/App/issues/18878#issuecomment-1550365511. I have found the issue and let me know if I should open the PR now.

cc: @mananjadhav @amyevans

mananjadhav commented 1 year ago

Yeah @bernhardoj please open the PR.

bernhardoj commented 1 year ago

PR is ready. https://github.com/Expensify/App/pull/19097

michaelhaxhiu commented 1 year ago

Please nudge me if I don't respond when it's time to process payment here (e.g. after the regression PR is merged and cleared)

michaelhaxhiu commented 1 year ago

Putting a date in the title that's more realistic, just so this doesn't appear behind.

amyevans commented 1 year ago

Also, considering this isn't going to change that often, and we've moved to a functional component, I don't think we need a regression test here.

Agreed, I don't think we need to add a regression test for this.

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.17-5 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-01. :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:

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.18-2 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-02. :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:

mananjadhav commented 1 year ago

@michaelhaxhiu Here's the last comment wrt to the Tooltip and @amyevans's response. This is ready for payout now, but also note it caused two regressions.

Also I think @dukenv0307 is due reporting bonus for reporting the bug here

bernhardoj commented 1 year ago

Actually, we still have 2 regression to be solved.

  1. https://github.com/Expensify/App/issues/19842 - wrong height issue
  2. https://expensify.slack.com/archives/C049HHMV9SM/p1685608239576139 - we should add preferred locale to tooltip key

I'm actually waiting for this PR to reach staging before fixing those 2 regression. As it is already reached staging, I will work on this now. So, maybe we can hold the payment for this fix, is that okay?

@mananjadhav @amyevans

michaelhaxhiu commented 1 year ago

Thanks for the insight! Agreed w/ @bernhardoj, let's hold payment until after those regressions are fully resolved.

bernhardoj commented 1 year ago

I have open the PR to fix the regressions here https://github.com/Expensify/App/pull/20340. Hopefully this will be the last one.

cc: @amyevans @mananjadhav

mananjadhav commented 1 year ago

Please allow me a day to review. I am unwell and would be offline most of the day.

mananjadhav commented 1 year ago

I checked the PR. The changes look fine, but @michaelhaxhiu @kavimuru do we have a set of regression test suite that you can share wrt Tooltips? I just want to make sure we cover everything before we go ahead and merge this one?

mananjadhav commented 1 year ago

Quick bump @michaelhaxhiu @kavimuru @amyevans if you can help with the previous comment?

michaelhaxhiu commented 1 year ago

hmm maybe we can ask @applausebot for their input as they interact the most with the test suites, I'm not sure off the top!

mananjadhav commented 1 year ago

@kavimuru @applausebot Quick bump.

michaelhaxhiu commented 1 year ago

@isagoico recommended the following from our internal slack channel 🧠

We currently don't have tests to cover that tooltip. Imho we can add these tests to the TC that I shared above:

User detail page

  1. Click on the header of the conversation
  2. Verify there's a copy option next to the users email
  3. Hover over the copy button
  4. Verify a tooltip is displayed with the action ("Copy to Clipboard")
  5. Click on the copy button
  6. Verify the tooltip displays "Copied"
mananjadhav commented 1 year ago

Thanks I think it won't cover all the regressions, may be with this issue we could update the Tooltip regressions along the way? I'll start with the given list and some that I am aware of.

On top of my head I can think of:

  1. Tooltip doesn't flicker
  2. Absolute Position of the tooltip works as expected
  3. Copied resets to Copy to Clipboard after few seconds on Safari
  4. Long Tooltip the arrows are aligned with the Bubble.
  5. Multiline Tooltips show up without text being truncated
  6. Switching between Pin/Unpin tooltip doesn't flicker and Unpin doesn't go multi-line.

@michaelhaxhiu I'll need a day to cover the testing for all the scenarios and still keeping 🤞. I don't want to add any more regressions 🙈

michaelhaxhiu commented 1 year ago

Should we pay this out and hold the regression tests as the final step?

michaelhaxhiu commented 1 year ago

Since there was 2 regressions, the payout would be as follows:

amyevans commented 1 year ago

@michaelhaxhiu There's still another PR (https://github.com/Expensify/App/pull/20340) awaiting review/deployment/holding period so I'm not sure we're clear to proceed