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.

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

Show tooltip below as soon as another element overlap the top edge #20240

Merged
merged 4 commits into from
Jun 27, 2023
Merged

Show tooltip below as soon as another element overlap the top edge #20240

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d36268
show tooltip below as soon as another element overlap the top edge
bernhardoj Jun 6, 2023
336f951
update comment
bernhardoj Jun 6, 2023
c5ac3f5
Merge branch 'main' into fix/improve-tooltip-overlap-check
bernhardoj Jun 9, 2023
e52bb39
Merge branch 'main' into fix/improve-tooltip-overlap-check
bernhardoj Jun 27, 2023
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
24 changes: 9 additions & 15 deletions src/styles/getTooltipStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid
}

/**
* Determines if there is an overlapping element at the top of both given coordinates.
* Determines if there is an overlapping element at the top of a given coordinate.
* (targetCenterX, y)
* |
* v
* _ _ _ _ _
* | |
* | |
* (x, targetCenterY) -> | |
* | |
* | |
* |_ _ _ _ _|
*
Expand All @@ -67,37 +67,31 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid
* and the top edge of the wrapped component.
* @param {Element} [tooltip] - The reference to the tooltip's root element
* @param {Number} tooltipTargetWidth - The width of the tooltip's target
* @param {Number} tooltipTargetHeight - The height of the tooltip's target
* @returns {Boolean}
*/
function isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetWidth, tooltipTargetHeight) {
function isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetWidth) {
if (typeof document.elementFromPoint !== 'function') {
return false;
}

// Use the x and y center position of the target to prevent wrong element
// returned by elementFromPoint in case the target has a border radius or is a multiline text.
// Use the x center position of the target to prevent wrong element returned by elementFromPoint
// in case the target has a border radius or is a multiline text.
const targetCenterX = xOffset + tooltipTargetWidth / 2;
const targetCenterY = yOffset + tooltipTargetHeight / 2;
const elementAtTargetCenterX = document.elementFromPoint(targetCenterX, yOffset);
const elementAtTargetCenterY = document.elementFromPoint(xOffset, targetCenterY);
const tooltipRef = (tooltip && tooltip.current) || tooltip;

// Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself
if (!elementAtTargetCenterX || !elementAtTargetCenterY || (tooltipRef && (tooltipRef.contains(elementAtTargetCenterX) || tooltipRef.contains(elementAtTargetCenterY)))) {
if (!elementAtTargetCenterX || (tooltipRef && tooltipRef.contains(elementAtTargetCenterX))) {
return false;
}

const rectAtTargetCenterX = elementAtTargetCenterX.getBoundingClientRect();
const rectAtTargetCenterY = elementAtTargetCenterY.getBoundingClientRect();

// Ensure it's not overlapping with another element by checking if the yOffset is greater than the top of the element
// and less than the bottom of the element
const isOverlappingAtTargetCenterX = yOffset > rectAtTargetCenterX.top && yOffset < rectAtTargetCenterX.bottom;
const isOverlappingAtTargetCenterY = yOffset > rectAtTargetCenterY.top && yOffset < rectAtTargetCenterY.bottom;

// Return true only if both elementAtTargetCenterX and elementAtTargetCenterY overlap with another element
return isOverlappingAtTargetCenterX && isOverlappingAtTargetCenterY;
return isOverlappingAtTargetCenterX;
Comment on lines -99 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused a regression #26041. Now we not taking into consideration the height the tooltip shows up at the bottom if there is any element above the tooltip marker, e.g. if we have the unread indicator shown before a report action then the tooltip will show at bottom even though we have enough space at the top.

Before:
Screenshot 2023-09-09 at 11 01 04 PM

After:
Screenshot 2023-09-09 at 11 01 23 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue that this PR is fixing does not seem to be reproducible, do you think we can revert the PR?

Copy link
Contributor Author

@bernhardoj bernhardoj Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't checked it thoroughly as I can't reach my laptop right now, but I wouldn't say it's a regression. That's how the overlap checking works. It's overlapping if something touches the top of an element. In case of unread indicator, could the height of the unread indicator overlaps with the display name?

For the history (let's use Avatar as an example)

  1. The overlapping logic check whether some element touch the x and y (top-left) of an Avatar
  2. Then, I have PR to change the position to be the center of both x and y (center) of an Avatar
  3. Last, this PR which now check the y and center of the x (top-center) of an Avatar.

We change from center to top-center because it requires an element to overlap vertically 50% (which I believe still the same if you revert this, so I don't think we can revert this PR).

One thing that could be improved is to move down a little bit of the y like 1%, so it would require an element to overlap the Avatar 1% vertically (from the top)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick response. I think it would be safer to just ignore the unread indicator and keep the overlapping logic as is for now.

}

/**
Expand Down Expand Up @@ -160,9 +154,9 @@ export default function getTooltipStyles(
if (isTooltipSizeReady) {
// Determine if the tooltip should display below the wrapped component.
// If either a tooltip will try to render within GUTTER_WIDTH logical pixels of the top of the screen,
// Or the wrapped component is overlapping at top-left with another element
// Or the wrapped component is overlapping at top-center with another element
// we'll display it beneath its wrapped component rather than above it as usual.
shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetWidth, tooltipTargetHeight);
shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, tooltip, tooltipTargetWidth);

// When the tooltip size is ready, we can start animating the scale.
scale = currentSize;
Expand Down