Skip to content

TooltipV2: Set the tooltip position when the popover open not when the toggle event is triggered #4327

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.

Sign up for GitHub

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

Merged
broccolinisoup merged 15 commits into from
Mar 12, 2024
Merged

TooltipV2: Set the tooltip position when the popover open not when the toggle event is triggered #4327

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f9516d2
do not set position if it was set before
broccolinisoup Feb 29, 2024
84c4bb3
console
broccolinisoup Feb 29, 2024
db335e9
only position set on tooltip show
broccolinisoup Feb 29, 2024
d9ffbe8
remove event listener with the same func signature
broccolinisoup Feb 29, 2024
1697c8b
just test
broccolinisoup Feb 29, 2024
b2f5576
calculate anchor position after popover-open class is added
broccolinisoup Feb 29, 2024
5ffe0bd
clean up
broccolinisoup Feb 29, 2024
1ca64c7
comment tidy up
broccolinisoup Feb 29, 2024
ba8bbe6
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Feb 29, 2024
1a4cd23
fix tests
broccolinisoup Mar 4, 2024
f67a6fb
changeset
broccolinisoup Mar 4, 2024
d68c967
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Mar 6, 2024
47f7569
move setting popover auto back in the render
broccolinisoup Mar 6, 2024
bcad60d
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Mar 8, 2024
78ff65b
update snapshots
broccolinisoup Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/beige-meals-doubt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Tooltip v2: Set the tooltip position when popover-open attribute is added to the tooltip element not the toggle event is triggered
46 changes: 18 additions & 28 deletions packages/react/src/TooltipV2/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const StyledTooltip = styled.div`
/* Overriding the default popover styles */
display: none;
&[popover] {
position: absolute;
Copy link
Contributor

@keithamus keithamus Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AIUI this is the main thing that is doing all the heavy lifting. I think you could just apply this line and ignore the rest and it would still work?

Copy link
Member Author

@broccolinisoup broccolinisoup Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely this line does all the heavy lifting and actually the rest is not directly related to this change but the existing issue* just became more visible when we added this line (no idea why 🤷🏻‍♀️ ).

*The existing issue (at least it is an issue in my opinion) is where we set the position.

Previously the flow was:

  1. hover/focus event is triggered
  2. tooltip.showPopover() function is called
  3. :popover-open attribute is added to the tooltip div
  4. Adding :popover-open triggers the toggle event as well as triggers the animation that changes the tooltip's opacity from 0 to 1
  5. Position is set in the toggle event's callback function.

I believe the wiggling issue (video in the PR description) happens because we set the position a little too late (step 5) but we make the tooltip visible in the step 3. So in this PR I am setting the position in Step 3 and that makes the toggle event redundant.

Let me know if you have any concern or suggestions.

padding: 0.5em 0.75em;
width: max-content;
margin: auto;
Expand Down Expand Up @@ -196,7 +197,23 @@ export const Tooltip = React.forwardRef(

const openTooltip = () => {
if (tooltipElRef.current && triggerRef.current && !tooltipElRef.current.matches(':popover-open')) {
tooltipElRef.current.showPopover()
const tooltip = tooltipElRef.current
const trigger = triggerRef.current
tooltip.setAttribute('popover', 'auto')
Copy link
Contributor

@keithamus keithamus Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should always be set so that the browser can correctly expose the right AT nodes. We should not set it as the tooltip is being opened.

Copy link
Member Author

@broccolinisoup broccolinisoup Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Although the tooltip is not exposed to AT when it is not opened but I think it still makes sense to set the initial attributes in the render rather than when it is opened.

tooltip.showPopover()
/*
* TOOLTIP POSITIONING
*/
const settings = {
side: directionToPosition[direction].side,
align: directionToPosition[direction].align,
}
const {top, left, anchorAlign, anchorSide} = getAnchoredPosition(tooltip, trigger, settings)
Copy link
Member

@siddharthkp siddharthkp Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect! We can't calculate boundingRect for an element with display:none, so this is the right timing.

If needed (race condition etc), we can wait for position to be set before animating the tooltip's opacity.

// This is required to make sure the popover is positioned correctly i.e. when there is not enough space on the specified direction, we set a new direction to position the ::after
const calculatedDirection = positionToDirection[`${anchorSide}-${anchorAlign}` as string]
setCalculatedDirection(calculatedDirection)
tooltip.style.top = `${top}px`
tooltip.style.left = `${left}px`
}
}
const closeTooltip = () => {
Expand Down Expand Up @@ -238,33 +255,6 @@ export const Tooltip = React.forwardRef(
apply()
}
}

/*
* TOOLTIP POSITIONING
*/
const tooltip = tooltipElRef.current
const trigger = triggerRef.current
tooltip.setAttribute('popover', 'auto')
const settings = {
side: directionToPosition[direction].side,
align: directionToPosition[direction].align,
}

const positionSet = () => {
const {top, left, anchorAlign, anchorSide} = getAnchoredPosition(tooltip, trigger, settings)

tooltip.style.top = `${top}px`
tooltip.style.left = `${left}px`
// This is required to make sure the popover is positioned correctly i.e. when there is not enough space on the specified direction, we set a new direction to position the ::after
const calculatedDirection = positionToDirection[`${anchorSide}-${anchorAlign}` as string]
setCalculatedDirection(calculatedDirection)
}

tooltip.addEventListener('toggle', positionSet)

return () => {
tooltip.removeEventListener('toggle', positionSet)
}
}, [tooltipElRef, triggerRef, direction, type])

return (
Expand Down
7 changes: 5 additions & 2 deletions packages/react/src/__tests__/ActionMenu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {render as HTMLRender, waitFor} from '@testing-library/react'
import {render as HTMLRender, waitFor, act} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import {axe} from 'jest-axe'
import React from 'react'
Expand Down Expand Up @@ -360,7 +360,10 @@ describe('ActionMenu', () => {
),
)
const button = component.getByRole('button')
button.focus()
act(() => {
button.focus()
})

expect(component.getByRole('tooltip')).toBeInTheDocument()
})

Expand Down