Skip to content

Commit

Permalink
Fix initial Tooltip/Popover flicker in wrong position (#2318)
Browse files Browse the repository at this point in the history
## Summary:
There have been some instances in `webapp` where Tooltip or Popover components are initially rendered in the top left corner and then is moved to the correct position in the next render. This causes the Tooltip/Popover to flicker briefly before it is shown in the correct place. In this video, we are able to see the Tooltip in the top left corner if we go through a video frame by frame:

https://github.com/user-attachments/assets/029e75b4-fc98-4033-b2c2-46ec30326603

When we log the style props from `popper-js`, we see that it passes us a `transform` property when it is rendered in the correct position. 

| Styles when popper is in the corner | Styles when positioned properly |
| --- | --- |
| ![tooltip-log-incorrect](https://github.com/user-attachments/assets/4e49afa2-6e7d-4962-b3a0-42837c1e8edc) | ![tooltip-log-correct](https://github.com/user-attachments/assets/3ef65978-8463-4c7a-a984-1f52938dcc1a) |

Looking into `react-popper`, the popper is initially rendered in the top left corner because the [styles provided by `react-popper` is initialized to the top left](https://github.com/floating-ui/react-popper/blob/2bbdd4a4fb56b0832e7f1934961d383d71b8c40d/src/usePopper.js#L58-L62).

To fix this, we visually hide the tooltip contents while the Popper is positioning itself. That way, the initial render in the top corner is not shown to users.  We are able to show the tooltip contents once the [PopperJs onFirstUpdate option](https://popper.js.org/docs/v2/lifecycle/#hook-into-the-lifecycle) is called and the positioning is ready

With this change, we don't see the issue anymore in webapp ([znd](https://prod-znd-240910-25401-68b02e8.khanacademy.org/)):

https://github.com/user-attachments/assets/0bb74402-cf20-4b66-b748-630d7c2dac59

And we also don't see the Tooltip/Popover in the top left corner anymore when debugging with a breakpoint in Storybook:

Before:

https://github.com/user-attachments/assets/803b2c57-26dc-449a-8146-72e8a3c37024 

After:

https://github.com/user-attachments/assets/a9916da4-78b3-4ae2-bd8c-ece683b54ff2 

### Note on another issue

While debugging, when the Tooltip/Popover renders for the first time, we also briefly see the popper in another unexpected location (that is not the top left). While trying to debug this, it seems that this is happening because the Aphrodite styles for the Tooltip/Popover are not yet ready so the styles from popper-js + the visibility fix isn't being applied to the DOM. Not sure how to address this, so let me know if you have any ideas! It's also important to note that after the first time a tooltip is shown, it doesn't happen again. This also doesn't seem to be noticeable in webapp as the observable issue was when it would render in the top left.

https://github.com/user-attachments/assets/17452410-2b68-48cf-8616-c416f8b34489

Issue: WB-1738

## Test plan:
- Confirm that the Tooltip and Popover components continue to work as expected in Storybook
  - Can also confirm that the Tooltip and Popover is not shown in the corner in WB's Storybook when debugging with breakpoints (see Investigation Notes on how to reproduce the issue locally) 
- Confirm that using these changes in webapp addresses the original flickering issue
- Confirm that Tooltip and Popover components in webapp continue to work as expected

## Investigation Notes:
- The issue is more prevalent in webapp, and it is difficult to notice in Wonder Blocks' Storybook and in webapp's Storybook. It would only happen sometimes, and would happen even if there's only one Tooltip on the page (so it wasn't an issue with multiple tooltips)
- It is reproducible in WB's Storybook by adding a `console.log` in [TooltipPopper's _renderPositionedContent](https://github.com/Khan/wonder-blocks/blob/30b6728217cd106f6d56308338f3ecc25cb5baa6/packages/wonder-blocks-tooltip/src/components/tooltip-popper.tsx#L185), adding a breakpoint to that line, and seeing what the component looks like each time that breakpoint was reached.
- Updating to the latest related dependencies didn't address the issue (`@popperjs/[email protected]`, `react-popper` was already up to date at `2.3.0`, see #2315 for the changes I used to investigate this bug)
- Checked to see if this was an issue in previous versions of the Tooltip: Issue was still there when debugging with the breakpoint in commits 60aba5b from 7 months ago and 9f3752d from last year
- Explored other alternatives to setting `visibility: hidden`:
  -  Rendering nothing while Popper isn't ready: the tooltip wouldn't work as expected
  - Applying `display: none`: positioning of the tooltip wasn't correct

Author: beaesguerra

Reviewers: jandrade, beaesguerra

Required Reviewers:

Approved By: jandrade

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Lint (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️  dependabot

Pull Request URL: #2318
  • Loading branch information
beaesguerra authored Sep 11, 2024
1 parent badad6e commit fcab789
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 2 deletions.
6 changes: 6 additions & 0 deletions .changeset/modern-bats-crash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/wonder-blocks-popover": patch
"@khanacademy/wonder-blocks-tooltip": patch
---

Only show the `TooltipPopper` contents once the popper has positioned itself. This fixes the issue where Tooltips are initially rendered in the top left corner for a brief moment before moving to the correct position (which was causing a flickering effect).
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ describe("tooltip integration tests", () => {
// to activate the tooltip
await ue.hover(anchor);
await jest.runAllTimers();
expect(screen.getByRole("tooltip")).toBeInTheDocument();
// We add `hidden: true` because the tooltip is initially hidden while
// it is re-positioned
expect(screen.getByRole("tooltip", {hidden: true})).toBeInTheDocument();
// Trigger mouseleave and mouseenter event and run timers only after
// both have been triggered. This simulates the mouseenter event being
// triggered before the tooltip is closed from the mouseleave event
Expand Down
31 changes: 30 additions & 1 deletion packages/wonder-blocks-tooltip/src/components/tooltip-popper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ type Props = {
viewportPadding?: number;
};

type State = {
/**
* If the popper is ready to show content.
*/
isReady: boolean;
};

type DefaultProps = {
rootBoundary: Props["rootBoundary"];
viewportPadding: Props["viewportPadding"];
Expand Down Expand Up @@ -130,12 +137,19 @@ const smallViewportModifier: SmallViewportModifier = {
* A component that wraps react-popper's Popper component to provide a
* consistent interface for positioning floating elements.
*/
export default class TooltipPopper extends React.Component<Props> {
export default class TooltipPopper extends React.Component<Props, State> {
static defaultProps: DefaultProps = {
rootBoundary: "viewport",
viewportPadding: spacing.small_12,
};

constructor(props: Props) {
super(props);
this.state = {
isReady: false,
};
}

/**
* Automatically updates the position of the floating element when necessary
* to ensure it stays anchored.
Expand Down Expand Up @@ -192,6 +206,7 @@ export default class TooltipPopper extends React.Component<Props> {
popperProps: PopperChildrenProps,
): React.ReactNode {
const {children} = this.props;
const {isReady} = this.state;

// We'll hide some complexity from the children here and ensure
// that our placement always has a value.
Expand Down Expand Up @@ -223,6 +238,10 @@ export default class TooltipPopper extends React.Component<Props> {
right: popperProps.style.right,
position: popperProps.style.position,
transform: popperProps.style.transform,
// We hide the content if the popper isn't ready yet. This
// makes it so users do not see the tooltip in the wrong place
// while it re-positions itself
visibility: !isReady ? "hidden" : undefined,
},
updateBubbleRef: this._bubbleRefTracker.updateRef,
tailOffset: {
Expand All @@ -238,6 +257,15 @@ export default class TooltipPopper extends React.Component<Props> {
return children(bubbleProps);
}

handleFirstUpdate = () => {
// Once the popper is positioned for the first time, the popper is ready
// to show content. For more details on onFirstUpdate, see
// https://popper.js.org/docs/v2/lifecycle/#hook-into-the-lifecycle
this.setState({
isReady: true,
});
};

render(): React.ReactNode {
const {anchorElement, placement, rootBoundary, viewportPadding} =
this.props;
Expand Down Expand Up @@ -268,6 +296,7 @@ export default class TooltipPopper extends React.Component<Props> {
strategy="fixed"
placement={placement}
modifiers={modifiers}
onFirstUpdate={this.handleFirstUpdate}
>
{(props) => this._renderPositionedContent(props)}
</Popper>
Expand Down

0 comments on commit fcab789

Please sign in to comment.