Skip to content
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

getAnchoredPosition does not respect margin on document body #242

Open
EnixCoda opened this issue Oct 31, 2023 · 5 comments
Open

getAnchoredPosition does not respect margin on document body #242

EnixCoda opened this issue Oct 31, 2023 · 5 comments

Comments

@EnixCoda
Copy link
Contributor

EnixCoda commented Oct 31, 2023

Hi, I noticed that the tooltips in PR pages are not aligned properly when body has margin set.

Check out the comparison below, you can see the tooltip shifts 200px relative to body between the 2 cases. And 200px is exactly the margin-left set to body.

no margin on body Manually add 200px margin on body
image image

Why would body has margin?

I noticed this issue because I use a GitHub browser extension that adds a sidebar to the page and set margin to body so that the sidebar would not overlap page content. I understand that primer is not responsible for handling of user's browser extension use case. But perhaps my use case has revealed a tiny defect of primer/behaviors and would help improve it. :)

image

Some insights I have noticed

Tooltip's position was set by

https://github.com/primer/view_components/blob/81eea01d0034c55771ce3e6c2afc685d364afc6c/app/components/primer/alpha/tool_tip.ts#L416-L417

And the position was calculated from getAnchoredPosition

export function getAnchoredPosition(

In getAnchoredPosition, the relativeRect has the correct left value for tooltip, if it was set to tooltip's left style, it would align properly.

const relativeRect = {
top: parentElementRect.top + borderTop,
left: parentElementRect.left + borderLeft,
}

However, the relativeRect was then passed to pureCalculateAnchoredPosition and it returned a different left.

function pureCalculateAnchoredPosition(
viewportRect: BoxPosition,
relativePosition: Position,
floatingRect: Size,
anchorRect: BoxPosition,
{side, align, allowOutOfBounds, anchorOffset, alignmentOffset}: PositionSettings,
): AnchorPosition {
// Compute the relative viewport rect, to bring it into the same coordinate space as `pos`
const relativeViewportRect: BoxPosition = {
top: viewportRect.top - relativePosition.top,
left: viewportRect.left - relativePosition.left,
width: viewportRect.width,
height: viewportRect.height,
}
let pos = calculatePosition(floatingRect, anchorRect, side, align, anchorOffset, alignmentOffset)
let anchorSide = side
let anchorAlign = align
pos.top -= relativePosition.top
pos.left -= relativePosition.left
// Handle screen overflow
if (!allowOutOfBounds) {
const alternateOrder = alternateOrders[side]
let positionAttempt = 0
if (alternateOrder) {
let prevSide = side
// Try all the alternate sides until one does not overflow
while (
positionAttempt < alternateOrder.length &&
shouldRecalculatePosition(prevSide, pos, relativeViewportRect, floatingRect)
) {
const nextSide = alternateOrder[positionAttempt++]
prevSide = nextSide
// If we have cut off in the same dimension as the "side" option, try flipping to the opposite side.
pos = calculatePosition(floatingRect, anchorRect, nextSide, align, anchorOffset, alignmentOffset)
pos.top -= relativePosition.top
pos.left -= relativePosition.left
anchorSide = nextSide
}
}
// If using alternate anchor side does not stop overflow,
// try using an alternate alignment
const alternateAlignment = alternateAlignments[align]
let alignmentAttempt = 0
if (alternateAlignment) {
let prevAlign = align
// Try all the alternate alignments until one does not overflow
while (
alignmentAttempt < alternateAlignment.length &&
shouldRecalculateAlignment(prevAlign, pos, relativeViewportRect, floatingRect)
) {
const nextAlign = alternateAlignment[alignmentAttempt++]
prevAlign = nextAlign
pos = calculatePosition(floatingRect, anchorRect, anchorSide, nextAlign, anchorOffset, alignmentOffset)
pos.top -= relativePosition.top
pos.left -= relativePosition.left
anchorAlign = nextAlign
}
}
// At this point we've flipped the position if applicable. Now just nudge until it's on-screen.
if (pos.top < relativeViewportRect.top) {
pos.top = relativeViewportRect.top
}
if (pos.left < relativeViewportRect.left) {
pos.left = relativeViewportRect.left
}
if (pos.left + floatingRect.width > viewportRect.width + relativeViewportRect.left) {
pos.left = viewportRect.width + relativeViewportRect.left - floatingRect.width
}
// If we have exhausted all possible positions and none of them worked, we
// say that overflowing the bottom of the screen is acceptable since it is
// likely to be able to scroll.
if (alternateOrder && positionAttempt < alternateOrder.length) {
if (pos.top + floatingRect.height > viewportRect.height + relativeViewportRect.top) {
pos.top = viewportRect.height + relativeViewportRect.top - floatingRect.height
}
}
}
return {...pos, anchorSide, anchorAlign}
}


Thank you for this awesome project!

@siddharthkp
Copy link
Member

Thanks for reporting! I'll poke around the function to see if there are any unintentional effects of including margin 😇

@EnixCoda
Copy link
Contributor Author

EnixCoda commented Nov 2, 2023

Thank you!

A new finding: I noticed that the PR review dialog has been updated today. It shifts for the same reason while the previous version used to work well.

image

@keithamus
Copy link
Member

@EnixCoda would you like to make a PR? With a PR we could try integrating it to see if there are any adverse effects (I think unlikely as we have margin:0 on body?).

@joshblack
Copy link
Member

Hi all! 👋 Just wanted to leave a comment to check-in and see if this issue was still present and, if not, to close out this issue.

@EnixCoda
Copy link
Contributor Author

Hi @joshblack

Yes this issue still presents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants