From 70e694ba90500f978e6c0121dabf01f407957095 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Sat, 1 Apr 2023 15:21:19 +0200 Subject: [PATCH 01/16] Use BoundsObserver to keep tooltip over the wrapper --- package-lock.json | 113 +++++++++++++++++++++++++---- package.json | 3 +- src/components/Tooltip/index.js | 125 ++++++++++---------------------- 3 files changed, 142 insertions(+), 99 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8520c058c17..dc5489f3c6d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -34,6 +34,7 @@ "@react-navigation/drawer": "github:Expensify/react-navigation#react-navigation-drawer-v6.5.0-alpha1-gitpkg", "@react-navigation/native": "6.0.13", "@react-navigation/stack": "6.3.1", + "@react-ng/bounds-observer": "^0.2.1", "@ua/react-native-airship": "^15.2.3", "awesome-phonenumber": "^5.4.0", "babel-plugin-transform-remove-console": "^6.9.4", @@ -2592,6 +2593,11 @@ "@hapi/hoek": "^9.0.0" } }, + "node_modules/@html-ng/bounding-client-rect-observer": { + "version": "0.1.3", + "resolved": "https://registry.npmjs.org/@html-ng/bounding-client-rect-observer/-/bounding-client-rect-observer-0.1.3.tgz", + "integrity": "sha512-RV1Lz23ckbpOgU1bNGxxTS4XTCEFGxiXoEmi8EOHtzTVzS+AEMkoqxllugn6IHEMqNkbcHipURRupEJe8Dsp1g==" + }, "node_modules/@humanwhocodes/config-array": { "version": "0.5.0", "dev": true, @@ -7263,6 +7269,38 @@ "react-native-screens": ">= 3.0.0" } }, + "node_modules/@react-ng/bounds-observer": { + "version": "0.2.1", + "resolved": "https://registry.npmjs.org/@react-ng/bounds-observer/-/bounds-observer-0.2.1.tgz", + "integrity": "sha512-i0h7x0qOLJz+JKxhOpngHFob6PH2Qmra85aQ0e/viS1yYgidoBvPJHn8WPGn5LXff98fE+fPhngsaD7FSbxcwQ==", + "dependencies": { + "@html-ng/bounding-client-rect-observer": "^0.1.3", + "@types/react": "^18.0.31", + "@types/react-dom": "^18.0.11", + "react": "^18.2.0", + "react-dom": "^18.2.0" + } + }, + "node_modules/@react-ng/bounds-observer/node_modules/react-dom": { + "version": "18.2.0", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.2.0.tgz", + "integrity": "sha512-6IMTriUmvsjHUjNtEDudZfuDQUoWXVxKHhlEGSk81n4YFS+r/Kl99wXiwlVXtPBtJenozv2P+hxDsw9eA7Xo6g==", + "dependencies": { + "loose-envify": "^1.1.0", + "scheduler": "^0.23.0" + }, + "peerDependencies": { + "react": "^18.2.0" + } + }, + "node_modules/@react-ng/bounds-observer/node_modules/scheduler": { + "version": "0.23.0", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.23.0.tgz", + "integrity": "sha512-CtuThmgHNg7zIZWAXi3AsyIzA3n4xx7aNyjwC2VJldO2LMVDhFK+63xGqq6CsJH4rTAt6/M+N4GhZiDYPx9eUw==", + "dependencies": { + "loose-envify": "^1.1.0" + } + }, "node_modules/@sentry/browser": { "version": "7.11.1", "license": "BSD-3-Clause", @@ -15133,8 +15171,7 @@ }, "node_modules/@types/prop-types": { "version": "15.7.5", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@types/qs": { "version": "6.9.7", @@ -15154,15 +15191,23 @@ "license": "MIT" }, "node_modules/@types/react": { - "version": "18.0.24", - "license": "MIT", - "peer": true, + "version": "18.2.6", + "resolved": "https://registry.npmjs.org/@types/react/-/react-18.2.6.tgz", + "integrity": "sha512-wRZClXn//zxCFW+ye/D2qY65UsYP1Fpex2YXorHc8awoNamkMZSvBxwxdYVInsHOZZd2Ppq8isnSzJL5Mpf8OA==", "dependencies": { "@types/prop-types": "*", "@types/scheduler": "*", "csstype": "^3.0.2" } }, + "node_modules/@types/react-dom": { + "version": "18.2.4", + "resolved": "https://registry.npmjs.org/@types/react-dom/-/react-dom-18.2.4.tgz", + "integrity": "sha512-G2mHoTMTL4yoydITgOGwWdWMVd8sNgyEP85xVmMKAPUBwQWm9wBPQUmvbeF4V3WBY1P7mmL4BkjQ0SqUpf1snw==", + "dependencies": { + "@types/react": "*" + } + }, "node_modules/@types/react-native": { "version": "0.70.6", "license": "MIT", @@ -15186,8 +15231,7 @@ }, "node_modules/@types/scheduler": { "version": "0.16.2", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@types/seedrandom": { "version": "2.4.30", @@ -43021,6 +43065,11 @@ "@hapi/hoek": "^9.0.0" } }, + "@html-ng/bounding-client-rect-observer": { + "version": "0.1.3", + "resolved": "https://registry.npmjs.org/@html-ng/bounding-client-rect-observer/-/bounding-client-rect-observer-0.1.3.tgz", + "integrity": "sha512-RV1Lz23ckbpOgU1bNGxxTS4XTCEFGxiXoEmi8EOHtzTVzS+AEMkoqxllugn6IHEMqNkbcHipURRupEJe8Dsp1g==" + }, "@humanwhocodes/config-array": { "version": "0.5.0", "dev": true, @@ -46328,6 +46377,37 @@ "warn-once": "^0.1.0" } }, + "@react-ng/bounds-observer": { + "version": "0.2.1", + "resolved": "https://registry.npmjs.org/@react-ng/bounds-observer/-/bounds-observer-0.2.1.tgz", + "integrity": "sha512-i0h7x0qOLJz+JKxhOpngHFob6PH2Qmra85aQ0e/viS1yYgidoBvPJHn8WPGn5LXff98fE+fPhngsaD7FSbxcwQ==", + "requires": { + "@html-ng/bounding-client-rect-observer": "^0.1.3", + "@types/react": "^18.0.31", + "@types/react-dom": "^18.0.11", + "react": "^18.2.0", + "react-dom": "^18.2.0" + }, + "dependencies": { + "react-dom": { + "version": "18.2.0", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.2.0.tgz", + "integrity": "sha512-6IMTriUmvsjHUjNtEDudZfuDQUoWXVxKHhlEGSk81n4YFS+r/Kl99wXiwlVXtPBtJenozv2P+hxDsw9eA7Xo6g==", + "requires": { + "loose-envify": "^1.1.0", + "scheduler": "^0.23.0" + } + }, + "scheduler": { + "version": "0.23.0", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.23.0.tgz", + "integrity": "sha512-CtuThmgHNg7zIZWAXi3AsyIzA3n4xx7aNyjwC2VJldO2LMVDhFK+63xGqq6CsJH4rTAt6/M+N4GhZiDYPx9eUw==", + "requires": { + "loose-envify": "^1.1.0" + } + } + } + }, "@sentry/browser": { "version": "7.11.1", "requires": { @@ -51519,8 +51599,7 @@ "dev": true }, "@types/prop-types": { - "version": "15.7.5", - "peer": true + "version": "15.7.5" }, "@types/qs": { "version": "6.9.7", @@ -51537,14 +51616,23 @@ "dev": true }, "@types/react": { - "version": "18.0.24", - "peer": true, + "version": "18.2.6", + "resolved": "https://registry.npmjs.org/@types/react/-/react-18.2.6.tgz", + "integrity": "sha512-wRZClXn//zxCFW+ye/D2qY65UsYP1Fpex2YXorHc8awoNamkMZSvBxwxdYVInsHOZZd2Ppq8isnSzJL5Mpf8OA==", "requires": { "@types/prop-types": "*", "@types/scheduler": "*", "csstype": "^3.0.2" } }, + "@types/react-dom": { + "version": "18.2.4", + "resolved": "https://registry.npmjs.org/@types/react-dom/-/react-dom-18.2.4.tgz", + "integrity": "sha512-G2mHoTMTL4yoydITgOGwWdWMVd8sNgyEP85xVmMKAPUBwQWm9wBPQUmvbeF4V3WBY1P7mmL4BkjQ0SqUpf1snw==", + "requires": { + "@types/react": "*" + } + }, "@types/react-native": { "version": "0.70.6", "peer": true, @@ -51564,8 +51652,7 @@ "dev": true }, "@types/scheduler": { - "version": "0.16.2", - "peer": true + "version": "0.16.2" }, "@types/seedrandom": { "version": "2.4.30", diff --git a/package.json b/package.json index 0328a845019..d6702a75671 100644 --- a/package.json +++ b/package.json @@ -69,6 +69,7 @@ "@react-navigation/drawer": "github:Expensify/react-navigation#react-navigation-drawer-v6.5.0-alpha1-gitpkg", "@react-navigation/native": "6.0.13", "@react-navigation/stack": "6.3.1", + "@react-ng/bounds-observer": "^0.2.1", "@ua/react-native-airship": "^15.2.3", "awesome-phonenumber": "^5.4.0", "babel-plugin-transform-remove-console": "^6.9.4", @@ -196,8 +197,8 @@ "metro-react-native-babel-preset": "^0.73.3", "mock-fs": "^4.13.0", "onchange": "^7.1.0", - "prettier": "^2.8.8", "portfinder": "^1.0.28", + "prettier": "^2.8.8", "pusher-js-mock": "^0.3.3", "react-native-clean-project": "^4.0.0-alpha4.0", "react-native-flipper": "https://gitpkg.now.sh/facebook/flipper/react-native/react-native-flipper?9cacc9b59402550eae866e0e81e5f0c2f8203e6b", diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 44157343069..edca5f286be 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -1,12 +1,12 @@ -import _ from 'underscore'; +import _ from 'lodash'; import React, {PureComponent} from 'react'; import {Animated, View} from 'react-native'; +import {BoundsObserver} from '@react-ng/bounds-observer'; import TooltipRenderedOnPageBody from './TooltipRenderedOnPageBody'; import Hoverable from '../Hoverable'; import withWindowDimensions from '../withWindowDimensions'; -import {propTypes, defaultProps} from './tooltipPropTypes'; +import {defaultProps, propTypes} from './tooltipPropTypes'; import TooltipSense from './TooltipSense'; -import makeCancellablePromise from '../../libs/MakeCancellablePromise'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; class Tooltip extends PureComponent { @@ -30,57 +30,20 @@ class Tooltip extends PureComponent { // Whether the tooltip is first tooltip to activate the TooltipSense this.isTooltipSenseInitiator = false; - this.shouldStartShowAnimation = false; this.animation = new Animated.Value(0); this.hasHoverSupport = DeviceCapabilities.hasHoverSupport(); - this.getWrapperPosition = this.getWrapperPosition.bind(this); this.showTooltip = this.showTooltip.bind(this); this.hideTooltip = this.hideTooltip.bind(this); + this.updateBounds = this.updateBounds.bind(this); } - componentDidUpdate(prevProps) { - if (this.props.windowWidth === prevProps.windowWidth && this.props.windowHeight === prevProps.windowHeight) { - return; - } - - this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()); - this.getWrapperPositionPromise.promise.then(({x, y}) => this.setState({xOffset: x, yOffset: y})); - } - - componentWillUnmount() { - if (!this.getWrapperPositionPromise) { - return; - } - - this.getWrapperPositionPromise.cancel(); - } - - /** - * Measure the position of the wrapper view relative to the window. - * - * @returns {Promise} - */ - getWrapperPosition() { - return new Promise((resolve) => { - // Make sure the wrapper is mounted before attempting to measure it. - if (this.wrapperView && _.isFunction(this.wrapperView.measureInWindow)) { - this.wrapperView.measureInWindow((x, y, width, height) => - resolve({ - x, - y, - width, - height, - }), - ); - } else { - resolve({ - x: 0, - y: 0, - width: 0, - height: 0, - }); - } + updateBounds(bounds) { + this.setState({ + wrapperWidth: bounds.width, + wrapperHeight: bounds.height, + xOffset: bounds.x, + yOffset: bounds.y, }); } @@ -91,38 +54,22 @@ class Tooltip extends PureComponent { if (!this.state.isRendered) { this.setState({isRendered: true}); } + this.animation.stopAnimation(); - this.shouldStartShowAnimation = true; - - // We have to dynamically calculate the position here as tooltip could have been rendered on some elments - // that has changed its position - this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()); - this.getWrapperPositionPromise.promise.then(({x, y, width, height}) => { - this.setState({ - wrapperWidth: width, - wrapperHeight: height, - xOffset: x, - yOffset: y, - }); - // We may need this check due to the reason that the animation start will fire async - // and hideTooltip could fire before it thus keeping the Tooltip visible - if (this.shouldStartShowAnimation) { - // When TooltipSense is active, immediately show the tooltip - if (TooltipSense.isActive()) { - this.animation.setValue(1); - } else { - this.isTooltipSenseInitiator = true; - Animated.timing(this.animation, { - toValue: 1, - duration: 140, - delay: 500, - useNativeDriver: false, - }).start(); - } - TooltipSense.activate(); - } - }); + // When TooltipSense is active, immediately show the tooltip + if (TooltipSense.isActive()) { + this.animation.setValue(1); + } else { + this.isTooltipSenseInitiator = true; + Animated.timing(this.animation, { + toValue: 1, + duration: 140, + delay: 500, + useNativeDriver: false, + }).start(); + } + TooltipSense.activate(); } /** @@ -130,7 +77,7 @@ class Tooltip extends PureComponent { */ hideTooltip() { this.animation.stopAnimation(); - this.shouldStartShowAnimation = false; + if (TooltipSense.isActive() && !this.isTooltipSenseInitiator) { this.animation.setValue(0); } else { @@ -142,7 +89,10 @@ class Tooltip extends PureComponent { useNativeDriver: false, }).start(); } + TooltipSense.deactivate(); + + this.setState({isRendered: false}); } render() { @@ -205,14 +155,19 @@ class Tooltip extends PureComponent { key={[this.props.text, ...this.props.renderTooltipContentKey]} /> )} - - {child} - + + {child} + + ); } From 55b7e4a0abf6610df8bae62c3242e643f94c293a Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Wed, 10 May 2023 16:23:40 +0200 Subject: [PATCH 02/16] Change how tooltip opacity is calculated ...to prevent UI junks. --- src/styles/getTooltipStyles.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 42ad2df1247..141de6e5b4b 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -138,10 +138,11 @@ export default function getTooltipStyles( // Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. const wrapperWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1; - // Hide the tooltip entirely if it's position hasn't finished measuring yet. This prevents UI jank where the tooltip flashes in the top left corner of the screen. - const opacity = xOffset === 0 && yOffset === 0 ? 0 : 1; - const isTooltipSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0; + + // Hide the tooltip entirely if it's size hasn't finished measuring yet. This prevents UI jank where the tooltip flashes close to its expected position. + const opacity = isTooltipSizeReady ? 1 : 0; + const scale = !isTooltipSizeReady ? 1 : currentSize; let wrapperTop = 0; let wrapperLeft = 0; From 225f3d1d81cbabf9a5fe132393f23f8e668f1075 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 11 May 2023 19:56:19 +0200 Subject: [PATCH 03/16] Ensure that the tooltip is not "colliding" with itself ...which could occur when scrolling down. --- src/components/Tooltip/TooltipRenderedOnPageBody.js | 1 + src/styles/getTooltipStyles.js | 12 ++++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/components/Tooltip/TooltipRenderedOnPageBody.js b/src/components/Tooltip/TooltipRenderedOnPageBody.js index 360bcca2d79..cca9d66e4e8 100644 --- a/src/components/Tooltip/TooltipRenderedOnPageBody.js +++ b/src/components/Tooltip/TooltipRenderedOnPageBody.js @@ -101,6 +101,7 @@ const TooltipRenderedOnPageBody = (props) => { tooltipContentWidth, props.shiftHorizontal, props.shiftVertical, + wrapper.current, ), [ props.animation, diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 141de6e5b4b..1150c07dd2d 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -56,22 +56,24 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid * and the left edge of the wrapped component. * @param {Number} yOffset - The distance between the top edge of the window * and the top edge of the wrapped component. + * @param {Element} wrapper - The reference to the tooltip wrapper * @returns {Boolean} */ -function isOverlappingAtTop(xOffset, yOffset) { +function isOverlappingAtTop(xOffset, yOffset, wrapper) { if (typeof document.elementFromPoint !== 'function') { return false; } const element = document.elementFromPoint(xOffset, yOffset); - if (!element) { + // Ensure it's not itself + if (!element || element === wrapper) { return false; } const rect = element.getBoundingClientRect(); - // Ensure it's not itself + overlapping with another element by checking if the yOffset is greater than the top of the element + // 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 return yOffset > rect.top && yOffset < rect.bottom; } @@ -96,6 +98,7 @@ function isOverlappingAtTop(xOffset, yOffset) { * and a negative value shifts it to the left. * @param {Number} [manualShiftVertical] - Any additional amount to manually shift the tooltip up or down. * A positive value shifts it down, and a negative value shifts it up. + * @param {Element} wrapper - The reference to the tooltip wrapper * @returns {Object} */ export default function getTooltipStyles( @@ -111,12 +114,13 @@ export default function getTooltipStyles( tooltipContentWidth, manualShiftHorizontal = 0, manualShiftVertical = 0, + wrapper, ) { // 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 // we'll display it beneath its wrapped component rather than above it as usual. - const shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset); + const shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, wrapper); // Determine if we need to shift the tooltip horizontally to prevent it // from displaying too near to the edge of the screen. From dc9924d98d213e3ddbd3f8304c161fd23751d5b6 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 15:24:06 +0200 Subject: [PATCH 04/16] Respond to PR suggestions, part 1 --- src/components/Tooltip/index.js | 13 +++++++++---- src/styles/getTooltipStyles.js | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index edca5f286be..4f3ff89ac36 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -1,11 +1,11 @@ -import _ from 'lodash'; +import _ from 'underscore'; import React, {PureComponent} from 'react'; import {Animated, View} from 'react-native'; import {BoundsObserver} from '@react-ng/bounds-observer'; import TooltipRenderedOnPageBody from './TooltipRenderedOnPageBody'; import Hoverable from '../Hoverable'; import withWindowDimensions from '../withWindowDimensions'; -import {defaultProps, propTypes} from './tooltipPropTypes'; +import * as tooltipPropTypes from './tooltipPropTypes'; import TooltipSense from './TooltipSense'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; @@ -38,6 +38,11 @@ class Tooltip extends PureComponent { this.updateBounds = this.updateBounds.bind(this); } + /** + * Update the tooltip bounding rectangle + * + * @param {DOMRectReadOnly} bounds - updated bounds + */ updateBounds(bounds) { this.setState({ wrapperWidth: bounds.width, @@ -173,6 +178,6 @@ class Tooltip extends PureComponent { } } -Tooltip.propTypes = propTypes; -Tooltip.defaultProps = defaultProps; +Tooltip.propTypes = tooltipPropTypes.propTypes; +Tooltip.defaultProps = tooltipPropTypes.defaultProps; export default withWindowDimensions(Tooltip); diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 1150c07dd2d..36a7871f065 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -66,7 +66,7 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { const element = document.elementFromPoint(xOffset, yOffset); - // Ensure it's not itself + // Ensure it's not the previous rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself if (!element || element === wrapper) { return false; } From 99faa6cf6459b11cd1ea4d0ce9351bf894bdad57 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 16:17:37 +0200 Subject: [PATCH 05/16] Ignore all tooltip's elements during the overlap check --- src/libs/DomUtils/index.js | 30 ++++++++++++++++++++++++++++++ src/styles/getTooltipStyles.js | 7 +++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/libs/DomUtils/index.js b/src/libs/DomUtils/index.js index 4e58ea3a08f..8d3c379710a 100644 --- a/src/libs/DomUtils/index.js +++ b/src/libs/DomUtils/index.js @@ -2,6 +2,36 @@ function blurActiveElement() { document.activeElement.blur(); } +/** + * Check whether the given element is in the DOM subtree with the given root + * + * @param {Element} element - element to check + * @param {Element} root - root of the subtree + * @return {Boolean} - whether the given element is in the subtree + */ +function isInSubtree(element, root) { + if (!element || !root) { + return false; + } + + if (element === root) { + return true; + } + + let currentNode = element.parentNode; + + while (currentNode !== null) { + if (currentNode === root) { + return true; + } + + currentNode = currentNode.parentNode; + } + + return false; +} + export default { blurActiveElement, + isInSubtree, }; diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 36a7871f065..78d75942ee6 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -5,6 +5,7 @@ import themeColors from './themes/default'; import fontFamily from './fontFamily'; import variables from './variables'; import roundToNearestMultipleOfFour from './roundToNearestMultipleOfFour'; +import DomUtils from '../DomUtils'; // This defines the proximity with the edge of the window in which tooltips should not be displayed. // If a tooltip is too close to the edge of the screen, we'll shift it towards the center. @@ -66,8 +67,10 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { const element = document.elementFromPoint(xOffset, yOffset); - // Ensure it's not the previous rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself - if (!element || element === wrapper) { + const isElementPartOfTooltip = DomUtils.isInSubtree(element, wrapper); + + // Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself + if (!element || isElementPartOfTooltip) { return false; } From e85f3adf7ce82287b2851ed576a6744f87a2b6d1 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 16:27:33 +0200 Subject: [PATCH 06/16] Fix DomUtils import --- src/styles/getTooltipStyles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 78d75942ee6..af58d86c2ca 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -5,7 +5,7 @@ import themeColors from './themes/default'; import fontFamily from './fontFamily'; import variables from './variables'; import roundToNearestMultipleOfFour from './roundToNearestMultipleOfFour'; -import DomUtils from '../DomUtils'; +import DomUtils from '../libs/DomUtils'; // This defines the proximity with the edge of the window in which tooltips should not be displayed. // If a tooltip is too close to the edge of the screen, we'll shift it towards the center. From 1e6a83a862848469614c25575529a7433e89dcf1 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 17:07:47 +0200 Subject: [PATCH 07/16] Tooltip: Introduce "isVisible" flag --- src/components/Tooltip/index.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 4f3ff89ac36..4b2a5f963bf 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -14,9 +14,12 @@ class Tooltip extends PureComponent { super(props); this.state = { - // Is tooltip rendered? + // Is tooltip already rendered on the page's body? This happens once. isRendered: false, + // Is the tooltip currently visible? + isVisible: false, + // The distance between the left side of the wrapper view and the left side of the window xOffset: 0, @@ -60,6 +63,8 @@ class Tooltip extends PureComponent { this.setState({isRendered: true}); } + this.setState({isVisible: true}); + this.animation.stopAnimation(); // When TooltipSense is active, immediately show the tooltip @@ -97,7 +102,7 @@ class Tooltip extends PureComponent { TooltipSense.deactivate(); - this.setState({isRendered: false}); + this.setState({isVisible: false}); } render() { @@ -161,7 +166,7 @@ class Tooltip extends PureComponent { /> )} Date: Thu, 18 May 2023 19:45:46 +0200 Subject: [PATCH 08/16] Use `Node.contains` instead of a custom utils --- src/components/Tooltip/index.js | 2 +- src/libs/DomUtils/index.js | 30 ------------------------------ src/styles/getTooltipStyles.js | 5 +---- 3 files changed, 2 insertions(+), 35 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 4b2a5f963bf..e1ccf056c8e 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -44,7 +44,7 @@ class Tooltip extends PureComponent { /** * Update the tooltip bounding rectangle * - * @param {DOMRectReadOnly} bounds - updated bounds + * @param {Object} bounds - updated bounds */ updateBounds(bounds) { this.setState({ diff --git a/src/libs/DomUtils/index.js b/src/libs/DomUtils/index.js index 8d3c379710a..4e58ea3a08f 100644 --- a/src/libs/DomUtils/index.js +++ b/src/libs/DomUtils/index.js @@ -2,36 +2,6 @@ function blurActiveElement() { document.activeElement.blur(); } -/** - * Check whether the given element is in the DOM subtree with the given root - * - * @param {Element} element - element to check - * @param {Element} root - root of the subtree - * @return {Boolean} - whether the given element is in the subtree - */ -function isInSubtree(element, root) { - if (!element || !root) { - return false; - } - - if (element === root) { - return true; - } - - let currentNode = element.parentNode; - - while (currentNode !== null) { - if (currentNode === root) { - return true; - } - - currentNode = currentNode.parentNode; - } - - return false; -} - export default { blurActiveElement, - isInSubtree, }; diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index af58d86c2ca..5e53b9708a4 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -5,7 +5,6 @@ import themeColors from './themes/default'; import fontFamily from './fontFamily'; import variables from './variables'; import roundToNearestMultipleOfFour from './roundToNearestMultipleOfFour'; -import DomUtils from '../libs/DomUtils'; // This defines the proximity with the edge of the window in which tooltips should not be displayed. // If a tooltip is too close to the edge of the screen, we'll shift it towards the center. @@ -67,10 +66,8 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { const element = document.elementFromPoint(xOffset, yOffset); - const isElementPartOfTooltip = DomUtils.isInSubtree(element, wrapper); - // Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself - if (!element || isElementPartOfTooltip) { + if (!element || wrapper.contains(element)) { return false; } From d50aa61a32f425de80f21a9a2600c1e8962ccd3e Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 20:15:12 +0200 Subject: [PATCH 09/16] Clean up tooltip-related naming --- .../Tooltip/TooltipRenderedOnPageBody.js | 48 +++++++------- src/components/Tooltip/index.js | 6 +- src/styles/getTooltipStyles.js | 64 +++++++++---------- 3 files changed, 60 insertions(+), 58 deletions(-) diff --git a/src/components/Tooltip/TooltipRenderedOnPageBody.js b/src/components/Tooltip/TooltipRenderedOnPageBody.js index cca9d66e4e8..e1ca222156f 100644 --- a/src/components/Tooltip/TooltipRenderedOnPageBody.js +++ b/src/components/Tooltip/TooltipRenderedOnPageBody.js @@ -20,11 +20,11 @@ const propTypes = { /** The distance between the top of the wrapper view and the top of the window */ yOffset: PropTypes.number.isRequired, - /** The width of the tooltip wrapper */ - wrapperWidth: PropTypes.number.isRequired, + /** The width of the tooltip's target wrapper */ + targetWrapperWidth: PropTypes.number.isRequired, - /** The Height of the tooltip wrapper */ - wrapperHeight: PropTypes.number.isRequired, + /** The height of the tooltip's target wrapper */ + targetWrapperHeight: PropTypes.number.isRequired, /** Any additional amount to manually adjust the horizontal position of the tooltip. A positive value shifts the tooltip to the right, and a negative value shifts it to the left. */ @@ -63,9 +63,9 @@ const TooltipRenderedOnPageBody = (props) => { // The width of tooltip's inner content. Has to be undefined in the beginning // as a width of 0 will cause the content to be rendered of a width of 0, // which prevents us from measuring it correctly. - const [tooltipContentWidth, setTooltipContentWidth] = useState(undefined); - const [tooltipWidth, setTooltipWidth] = useState(0); - const [tooltipHeight, setTooltipHeight] = useState(0); + const [contentMeasuredWidth, setContentMeasuredWidth] = useState(undefined); + const [wrapperMeasuredWidth, setWrapperMeasuredWidth] = useState(0); + const [wrapperMeasuredHeight, setWrapperMeasuredHeight] = useState(0); const contentRef = useRef(); const wrapper = useRef(); @@ -81,24 +81,24 @@ const TooltipRenderedOnPageBody = (props) => { // because of the late update of the width and the height from onLayout. const rect = wrapper.current.getBoundingClientRect(); - setTooltipWidth(rect.width); - setTooltipHeight(rect.height); - setTooltipContentWidth(contentRef.current.offsetWidth); + setWrapperMeasuredWidth(rect.width); + setWrapperMeasuredHeight(rect.height); + setContentMeasuredWidth(contentRef.current.offsetWidth); }, []); - const {animationStyle, tooltipWrapperStyle, tooltipTextStyle, pointerWrapperStyle, pointerStyle} = useMemo( + const {animationStyle, wrapperStyle, textStyle, pointerWrapperStyle, pointerStyle} = useMemo( () => getTooltipStyles( props.animation, props.windowWidth, props.xOffset, props.yOffset, - props.wrapperWidth, - props.wrapperHeight, + props.targetWrapperWidth, + props.targetWrapperHeight, props.maxWidth, - tooltipWidth, - tooltipHeight, - tooltipContentWidth, + wrapperMeasuredWidth, + wrapperMeasuredHeight, + contentMeasuredWidth, props.shiftHorizontal, props.shiftVertical, wrapper.current, @@ -108,12 +108,12 @@ const TooltipRenderedOnPageBody = (props) => { props.windowWidth, props.xOffset, props.yOffset, - props.wrapperWidth, - props.wrapperHeight, + props.targetWrapperWidth, + props.targetWrapperHeight, props.maxWidth, - tooltipWidth, - tooltipHeight, - tooltipContentWidth, + wrapperMeasuredWidth, + wrapperMeasuredHeight, + contentMeasuredWidth, props.shiftHorizontal, props.shiftVertical, ], @@ -126,10 +126,10 @@ const TooltipRenderedOnPageBody = (props) => { content = ( {props.text} @@ -141,7 +141,7 @@ const TooltipRenderedOnPageBody = (props) => { return ReactDOM.createPortal( {content} diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index e1ccf056c8e..06559d1abd1 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -9,6 +9,8 @@ import * as tooltipPropTypes from './tooltipPropTypes'; import TooltipSense from './TooltipSense'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; +// A "target" for the tooltip, i.e. an element that, when hovered over, triggers the tooltip to appear. The tooltip will +// point towards this target. class Tooltip extends PureComponent { constructor(props) { super(props); @@ -152,8 +154,8 @@ class Tooltip extends PureComponent { windowWidth={this.props.windowWidth} xOffset={this.state.xOffset} yOffset={this.state.yOffset} - wrapperWidth={this.state.wrapperWidth} - wrapperHeight={this.state.wrapperHeight} + targetWrapperWidth={this.state.wrapperWidth} + targetWrapperHeight={this.state.wrapperHeight} shiftHorizontal={_.result(this.props, 'shiftHorizontal')} shiftVertical={_.result(this.props, 'shiftVertical')} text={this.props.text} diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 5e53b9708a4..46f2d35c6db 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -87,12 +87,12 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { * and the left edge of the wrapped component. * @param {Number} yOffset - The distance between the top edge of the window * and the top edge of the wrapped component. - * @param {Number} componentWidth - The width of the wrapped component. - * @param {Number} componentHeight - The height of the wrapped component. + * @param {Number} targetWrapperWidth - The width of the tooltip target's wrapper. + * @param {Number} targetWrapperHeight - The height of the tooltip target's wrapper. * @param {Number} maxWidth - The tooltip's max width. - * @param {Number} tooltipWidth - The width of the tooltip itself. - * @param {Number} tooltipHeight - The height of the tooltip itself. - * @param {Number} tooltipContentWidth - The tooltip's inner content width. + * @param {Number} wrapperMeasuredWidth - The measured width of the tooltip target's wrapper. + * @param {Number} wrapperMeasuredHeight - The measured height of the tooltip target's wrapper. + * @param {Number} contentMeasuredWidth - The tooltip's inner content measured width. * @param {Number} [manualShiftHorizontal] - Any additional amount to manually shift the tooltip to the left or right. * A positive value shifts it to the right, * and a negative value shifts it to the left. @@ -106,12 +106,12 @@ export default function getTooltipStyles( windowWidth, xOffset, yOffset, - componentWidth, - componentHeight, + targetWrapperWidth, + targetWrapperHeight, maxWidth, - tooltipWidth, - tooltipHeight, - tooltipContentWidth, + wrapperMeasuredWidth, + wrapperMeasuredHeight, + contentMeasuredWidth, manualShiftHorizontal = 0, manualShiftVertical = 0, wrapper, @@ -120,38 +120,38 @@ export default function getTooltipStyles( // 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 // we'll display it beneath its wrapped component rather than above it as usual. - const shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, wrapper); + const shouldShowBelow = yOffset - wrapperMeasuredHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, wrapper); // Determine if we need to shift the tooltip horizontally to prevent it // from displaying too near to the edge of the screen. - const horizontalShift = computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWidth, manualShiftHorizontal); + const horizontalShift = computeHorizontalShift(windowWidth, xOffset, targetWrapperWidth, wrapperNominalWidth, manualShiftHorizontal); // Determine if we need to shift the pointer horizontally to prevent it from being too near to the edge of the tooltip // We shift it to the right a bit if the tooltip is positioned on the extreme left // and shift it to left a bit if the tooltip is positioned on the extreme right. const horizontalShiftPointer = horizontalShift > 0 - ? Math.max(-horizontalShift, -(tooltipWidth / 2) + POINTER_WIDTH / 2 + variables.componentBorderRadiusSmall) - : Math.min(-horizontalShift, tooltipWidth / 2 - POINTER_WIDTH / 2 - variables.componentBorderRadiusSmall); + ? Math.max(-horizontalShift, -(wrapperMeasuredWidth / 2) + POINTER_WIDTH / 2 + variables.componentBorderRadiusSmall) + : Math.min(-horizontalShift, wrapperMeasuredWidth / 2 - POINTER_WIDTH / 2 - variables.componentBorderRadiusSmall); const tooltipVerticalPadding = spacing.pv1; const tooltipFontSize = variables.fontSizeSmall; - // We get wrapper width based on the tooltip's inner text width so the wrapper is just big enough to fit text and prevent white space. + // We calculate wrapper width based on the tooltip's inner text width so the wrapper is just big enough to fit text and prevent white space. // If the text width is less than the maximum available width, add horizontal padding. - // Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. - const wrapperWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1; + // Note: contentMeasuredWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. + const wrapperNominalWidth = contentMeasuredWidth && contentMeasuredWidth + spacing.ph2.paddingHorizontal * 2 + 1; - const isTooltipSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0; + const isWrapperSizeReady = wrapperMeasuredWidth !== 0 && wrapperMeasuredHeight !== 0; // Hide the tooltip entirely if it's size hasn't finished measuring yet. This prevents UI jank where the tooltip flashes close to its expected position. - const opacity = isTooltipSizeReady ? 1 : 0; + const opacity = isWrapperSizeReady ? 1 : 0; - const scale = !isTooltipSizeReady ? 1 : currentSize; + const scale = !isWrapperSizeReady ? 1 : currentSize; let wrapperTop = 0; let wrapperLeft = 0; - if (isTooltipSizeReady) { + if (isWrapperSizeReady) { // Because it uses fixed positioning, the top-left corner of the tooltip is aligned // with the top-left corner of the window by default. // we will use yOffset to position the tooltip relative to the Wrapped Component @@ -162,9 +162,9 @@ export default function getTooltipStyles( // To shift the tooltip up, we'll give `top` a negative value. wrapperTop = shouldShowBelow ? // We need to shift the tooltip down below the component. So shift the tooltip down (+) by... - yOffset + componentHeight + POINTER_HEIGHT + manualShiftVertical + yOffset + targetWrapperHeight + POINTER_HEIGHT + manualShiftVertical : // We need to shift the tooltip up above the component. So shift the tooltip up (-) by... - yOffset - (tooltipHeight + POINTER_HEIGHT) + manualShiftVertical; + yOffset - (wrapperMeasuredHeight + POINTER_HEIGHT) + manualShiftVertical; // Next, we'll position it horizontally. // we will use xOffset to position the tooltip relative to the Wrapped Component @@ -178,7 +178,7 @@ export default function getTooltipStyles( // so the tooltip's center lines up with the center of the wrapped component. // 3) Add the horizontal shift (left or right) computed above to keep it out of the gutters. // 4) Lastly, add the manual horizontal shift passed in as a parameter. - wrapperLeft = xOffset + (componentWidth / 2 - tooltipWidth / 2) + horizontalShift + manualShiftHorizontal; + wrapperLeft = xOffset + (targetWrapperWidth / 2 - wrapperMeasuredWidth / 2) + horizontalShift + manualShiftHorizontal; } return { @@ -188,14 +188,14 @@ export default function getTooltipStyles( // so Position fixed children will be relative to this new Local cordinate system transform: [{scale}], }, - tooltipWrapperStyle: { + wrapperStyle: { position: 'fixed', backgroundColor: themeColors.heading, borderRadius: variables.componentBorderRadiusSmall, ...tooltipVerticalPadding, ...spacing.ph2, zIndex: variables.tooltipzIndex, - width: wrapperWidth, + width: wrapperNominalWidth, maxWidth, top: wrapperTop, left: wrapperLeft, @@ -204,7 +204,7 @@ export default function getTooltipStyles( // We are adding this to prevent the tooltip text from being selected and copied on CTRL + A. ...styles.userSelectNone, }, - tooltipTextStyle: { + textStyle: { color: themeColors.textReversed, fontFamily: fontFamily.EXP_NEUE, fontSize: tooltipFontSize, @@ -222,16 +222,16 @@ export default function getTooltipStyles( // // OR if the pointer should be above the tooltip wrapper, then the pointer up (-) by the pointer's height // so that the bottom of the pointer lines up with the top of the tooltip - top: shouldShowBelow ? -POINTER_HEIGHT : tooltipHeight, + top: shouldShowBelow ? -POINTER_HEIGHT : wrapperMeasuredHeight, // To align it horizontally, we'll: - // 1) Shift the pointer to the right (+) by the half the tooltipWidth's width, - // so the left edge of the pointer lines up with the tooltipWidth's center. + // 1) Shift the pointer to the right (+) by the half the wrapperNominalWidth's width, + // so the left edge of the pointer lines up with the wrapperNominalWidth's center. // 2) To the left (-) by half the pointer's width, - // so the pointer's center lines up with the tooltipWidth's center. + // so the pointer's center lines up with the wrapperNominalWidth's center. // 3) Due to the tip start from the left edge of wrapper Tooltip so we have to remove the // horizontalShift which is added to adjust it into the Window - left: horizontalShiftPointer + (tooltipWidth / 2 - POINTER_WIDTH / 2), + left: horizontalShiftPointer + (wrapperNominalWidth / 2 - POINTER_WIDTH / 2), opacity, }, From 7748d2d409d33d12abb2c43f64e011c8e6b8c7ee Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 21:18:31 +0200 Subject: [PATCH 10/16] Fix wrapperNominalWidth used instead of wrapperMeasuredWidth --- src/styles/getTooltipStyles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 46f2d35c6db..41bf0854ce4 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -124,7 +124,7 @@ export default function getTooltipStyles( // Determine if we need to shift the tooltip horizontally to prevent it // from displaying too near to the edge of the screen. - const horizontalShift = computeHorizontalShift(windowWidth, xOffset, targetWrapperWidth, wrapperNominalWidth, manualShiftHorizontal); + const horizontalShift = computeHorizontalShift(windowWidth, xOffset, targetWrapperWidth, wrapperMeasuredWidth, manualShiftHorizontal); // Determine if we need to shift the pointer horizontally to prevent it from being too near to the edge of the tooltip // We shift it to the right a bit if the tooltip is positioned on the extreme left From e56ce29792d7948e0dd73d39d42257a07a879319 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:02:27 +0200 Subject: [PATCH 11/16] Clean up tooltip-related naming, round 2 --- .../Tooltip/TooltipRenderedOnPageBody.js | 16 +++--- src/components/Tooltip/index.js | 4 +- src/styles/getTooltipStyles.js | 56 +++++++++---------- 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/components/Tooltip/TooltipRenderedOnPageBody.js b/src/components/Tooltip/TooltipRenderedOnPageBody.js index e1ca222156f..68a1152987b 100644 --- a/src/components/Tooltip/TooltipRenderedOnPageBody.js +++ b/src/components/Tooltip/TooltipRenderedOnPageBody.js @@ -20,11 +20,11 @@ const propTypes = { /** The distance between the top of the wrapper view and the top of the window */ yOffset: PropTypes.number.isRequired, - /** The width of the tooltip's target wrapper */ - targetWrapperWidth: PropTypes.number.isRequired, + /** The width of the tooltip's target */ + targetWidth: PropTypes.number.isRequired, - /** The height of the tooltip's target wrapper */ - targetWrapperHeight: PropTypes.number.isRequired, + /** The height of the tooltip's target */ + targetHeight: PropTypes.number.isRequired, /** Any additional amount to manually adjust the horizontal position of the tooltip. A positive value shifts the tooltip to the right, and a negative value shifts it to the left. */ @@ -93,8 +93,8 @@ const TooltipRenderedOnPageBody = (props) => { props.windowWidth, props.xOffset, props.yOffset, - props.targetWrapperWidth, - props.targetWrapperHeight, + props.targetWidth, + props.targetHeight, props.maxWidth, wrapperMeasuredWidth, wrapperMeasuredHeight, @@ -108,8 +108,8 @@ const TooltipRenderedOnPageBody = (props) => { props.windowWidth, props.xOffset, props.yOffset, - props.targetWrapperWidth, - props.targetWrapperHeight, + props.targetWidth, + props.targetHeight, props.maxWidth, wrapperMeasuredWidth, wrapperMeasuredHeight, diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 06559d1abd1..f18bc803aa4 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -154,8 +154,8 @@ class Tooltip extends PureComponent { windowWidth={this.props.windowWidth} xOffset={this.state.xOffset} yOffset={this.state.yOffset} - targetWrapperWidth={this.state.wrapperWidth} - targetWrapperHeight={this.state.wrapperHeight} + targetWidth={this.state.wrapperWidth} + targetHeight={this.state.wrapperHeight} shiftHorizontal={_.result(this.props, 'shiftHorizontal')} shiftVertical={_.result(this.props, 'shiftVertical')} text={this.props.text} diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 41bf0854ce4..d6a25b55641 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -87,18 +87,18 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { * and the left edge of the wrapped component. * @param {Number} yOffset - The distance between the top edge of the window * and the top edge of the wrapped component. - * @param {Number} targetWrapperWidth - The width of the tooltip target's wrapper. - * @param {Number} targetWrapperHeight - The height of the tooltip target's wrapper. + * @param {Number} tooltipTargetWidth - The width of the tooltip's target + * @param {Number} tooltipTargetHeight - The height of the tooltip's target * @param {Number} maxWidth - The tooltip's max width. - * @param {Number} wrapperMeasuredWidth - The measured width of the tooltip target's wrapper. - * @param {Number} wrapperMeasuredHeight - The measured height of the tooltip target's wrapper. - * @param {Number} contentMeasuredWidth - The tooltip's inner content measured width. + * @param {Number} tooltipWidth - The measured width of the tooltip + * @param {Number} tooltipHeight - The measured height of the tooltip + * @param {Number} tooltipContentWidth - The tooltip's inner content measured width. * @param {Number} [manualShiftHorizontal] - Any additional amount to manually shift the tooltip to the left or right. * A positive value shifts it to the right, * and a negative value shifts it to the left. * @param {Number} [manualShiftVertical] - Any additional amount to manually shift the tooltip up or down. * A positive value shifts it down, and a negative value shifts it up. - * @param {Element} wrapper - The reference to the tooltip wrapper + * @param {Element} tooltip - The reference to the tooltip's root element * @returns {Object} */ export default function getTooltipStyles( @@ -106,43 +106,43 @@ export default function getTooltipStyles( windowWidth, xOffset, yOffset, - targetWrapperWidth, - targetWrapperHeight, + tooltipTargetWidth, + tooltipTargetHeight, maxWidth, - wrapperMeasuredWidth, - wrapperMeasuredHeight, - contentMeasuredWidth, + tooltipWidth, + tooltipHeight, + tooltipContentWidth, manualShiftHorizontal = 0, manualShiftVertical = 0, - wrapper, + tooltip, ) { // 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 // we'll display it beneath its wrapped component rather than above it as usual. - const shouldShowBelow = yOffset - wrapperMeasuredHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, wrapper); + const shouldShowBelow = yOffset - tooltipHeight < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset, tooltip); // Determine if we need to shift the tooltip horizontally to prevent it // from displaying too near to the edge of the screen. - const horizontalShift = computeHorizontalShift(windowWidth, xOffset, targetWrapperWidth, wrapperMeasuredWidth, manualShiftHorizontal); + const horizontalShift = computeHorizontalShift(windowWidth, xOffset, tooltipTargetWidth, tooltipWidth, manualShiftHorizontal); // Determine if we need to shift the pointer horizontally to prevent it from being too near to the edge of the tooltip // We shift it to the right a bit if the tooltip is positioned on the extreme left // and shift it to left a bit if the tooltip is positioned on the extreme right. const horizontalShiftPointer = horizontalShift > 0 - ? Math.max(-horizontalShift, -(wrapperMeasuredWidth / 2) + POINTER_WIDTH / 2 + variables.componentBorderRadiusSmall) - : Math.min(-horizontalShift, wrapperMeasuredWidth / 2 - POINTER_WIDTH / 2 - variables.componentBorderRadiusSmall); + ? Math.max(-horizontalShift, -(tooltipWidth / 2) + POINTER_WIDTH / 2 + variables.componentBorderRadiusSmall) + : Math.min(-horizontalShift, tooltipWidth / 2 - POINTER_WIDTH / 2 - variables.componentBorderRadiusSmall); const tooltipVerticalPadding = spacing.pv1; const tooltipFontSize = variables.fontSizeSmall; - // We calculate wrapper width based on the tooltip's inner text width so the wrapper is just big enough to fit text and prevent white space. + // We calculate tooltip width based on the tooltip's inner text width so the tooltip is just big enough to fit text and prevent white space. // If the text width is less than the maximum available width, add horizontal padding. - // Note: contentMeasuredWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. - const wrapperNominalWidth = contentMeasuredWidth && contentMeasuredWidth + spacing.ph2.paddingHorizontal * 2 + 1; + // Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. + const wrapperNominalWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1; - const isWrapperSizeReady = wrapperMeasuredWidth !== 0 && wrapperMeasuredHeight !== 0; + const isWrapperSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0; // Hide the tooltip entirely if it's size hasn't finished measuring yet. This prevents UI jank where the tooltip flashes close to its expected position. const opacity = isWrapperSizeReady ? 1 : 0; @@ -162,9 +162,9 @@ export default function getTooltipStyles( // To shift the tooltip up, we'll give `top` a negative value. wrapperTop = shouldShowBelow ? // We need to shift the tooltip down below the component. So shift the tooltip down (+) by... - yOffset + targetWrapperHeight + POINTER_HEIGHT + manualShiftVertical + yOffset + tooltipTargetHeight + POINTER_HEIGHT + manualShiftVertical : // We need to shift the tooltip up above the component. So shift the tooltip up (-) by... - yOffset - (wrapperMeasuredHeight + POINTER_HEIGHT) + manualShiftVertical; + yOffset - (tooltipHeight + POINTER_HEIGHT) + manualShiftVertical; // Next, we'll position it horizontally. // we will use xOffset to position the tooltip relative to the Wrapped Component @@ -178,7 +178,7 @@ export default function getTooltipStyles( // so the tooltip's center lines up with the center of the wrapped component. // 3) Add the horizontal shift (left or right) computed above to keep it out of the gutters. // 4) Lastly, add the manual horizontal shift passed in as a parameter. - wrapperLeft = xOffset + (targetWrapperWidth / 2 - wrapperMeasuredWidth / 2) + horizontalShift + manualShiftHorizontal; + wrapperLeft = xOffset + (tooltipTargetWidth / 2 - tooltipWidth / 2) + horizontalShift + manualShiftHorizontal; } return { @@ -214,22 +214,22 @@ export default function getTooltipStyles( pointerWrapperStyle: { position: 'fixed', - // By default, the pointer's top-left will align with the top-left of the tooltip wrapper. + // By default, the pointer's top-left will align with the top-left of the tooltip tooltip. // // To align it vertically, we'll: - // If the pointer should be below the tooltip wrapper, shift the pointer down (+) by the tooltip height, + // If the pointer should be below the tooltip tooltip, shift the pointer down (+) by the tooltip height, // so that the top of the pointer lines up with the bottom of the tooltip // - // OR if the pointer should be above the tooltip wrapper, then the pointer up (-) by the pointer's height + // OR if the pointer should be above the tooltip tooltip, then the pointer up (-) by the pointer's height // so that the bottom of the pointer lines up with the top of the tooltip - top: shouldShowBelow ? -POINTER_HEIGHT : wrapperMeasuredHeight, + top: shouldShowBelow ? -POINTER_HEIGHT : tooltipHeight, // To align it horizontally, we'll: // 1) Shift the pointer to the right (+) by the half the wrapperNominalWidth's width, // so the left edge of the pointer lines up with the wrapperNominalWidth's center. // 2) To the left (-) by half the pointer's width, // so the pointer's center lines up with the wrapperNominalWidth's center. - // 3) Due to the tip start from the left edge of wrapper Tooltip so we have to remove the + // 3) Due to the tip start from the left edge of tooltip Tooltip so we have to remove the // horizontalShift which is added to adjust it into the Window left: horizontalShiftPointer + (wrapperNominalWidth / 2 - POINTER_WIDTH / 2), From 76f6c127a061d11144982afbd76e74b27aa88c14 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:13:22 +0200 Subject: [PATCH 12/16] Clean up tooltip-related naming, round 3 --- src/components/Tooltip/TooltipRenderedOnPageBody.js | 12 ++++++------ src/styles/getTooltipStyles.js | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/components/Tooltip/TooltipRenderedOnPageBody.js b/src/components/Tooltip/TooltipRenderedOnPageBody.js index 68a1152987b..d22bf2ba65c 100644 --- a/src/components/Tooltip/TooltipRenderedOnPageBody.js +++ b/src/components/Tooltip/TooltipRenderedOnPageBody.js @@ -67,7 +67,7 @@ const TooltipRenderedOnPageBody = (props) => { const [wrapperMeasuredWidth, setWrapperMeasuredWidth] = useState(0); const [wrapperMeasuredHeight, setWrapperMeasuredHeight] = useState(0); const contentRef = useRef(); - const wrapper = useRef(); + const rootWrapper = useRef(); useEffect(() => { if (!props.renderTooltipContent || !props.text) { @@ -79,14 +79,14 @@ const TooltipRenderedOnPageBody = (props) => { useLayoutEffect(() => { // Calculate the tooltip width and height before the browser repaints the screen to prevent flicker // because of the late update of the width and the height from onLayout. - const rect = wrapper.current.getBoundingClientRect(); + const rect = rootWrapper.current.getBoundingClientRect(); setWrapperMeasuredWidth(rect.width); setWrapperMeasuredHeight(rect.height); setContentMeasuredWidth(contentRef.current.offsetWidth); }, []); - const {animationStyle, wrapperStyle, textStyle, pointerWrapperStyle, pointerStyle} = useMemo( + const {animationStyle, rootWrapperStyle, textStyle, pointerWrapperStyle, pointerStyle} = useMemo( () => getTooltipStyles( props.animation, @@ -101,7 +101,7 @@ const TooltipRenderedOnPageBody = (props) => { contentMeasuredWidth, props.shiftHorizontal, props.shiftVertical, - wrapper.current, + rootWrapper.current, ), [ props.animation, @@ -140,8 +140,8 @@ const TooltipRenderedOnPageBody = (props) => { return ReactDOM.createPortal( {content} diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index d6a25b55641..ffd6976f686 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -188,7 +188,7 @@ export default function getTooltipStyles( // so Position fixed children will be relative to this new Local cordinate system transform: [{scale}], }, - wrapperStyle: { + rootWrapperStyle: { position: 'fixed', backgroundColor: themeColors.heading, borderRadius: variables.componentBorderRadiusSmall, From 8f26c99161db0217cd21f6fceae6e29af304acd0 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:18:03 +0200 Subject: [PATCH 13/16] isOverlappingAtTop: Update JSDoc to match the most recent naming --- src/styles/getTooltipStyles.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index ffd6976f686..2314f0aba1f 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -56,10 +56,10 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid * and the left edge of the wrapped component. * @param {Number} yOffset - The distance between the top edge of the window * and the top edge of the wrapped component. - * @param {Element} wrapper - The reference to the tooltip wrapper + * @param {Element} tooltip - The reference to the tooltip's root element * @returns {Boolean} */ -function isOverlappingAtTop(xOffset, yOffset, wrapper) { +function isOverlappingAtTop(xOffset, yOffset, tooltip) { if (typeof document.elementFromPoint !== 'function') { return false; } @@ -67,7 +67,7 @@ function isOverlappingAtTop(xOffset, yOffset, wrapper) { const element = document.elementFromPoint(xOffset, yOffset); // Ensure it's not the already rendered element of this very tooltip, so the tooltip doesn't try to "avoid" itself - if (!element || wrapper.contains(element)) { + if (!element || tooltip.contains(element)) { return false; } From b2179332f10489aa8c98e8653686d53bb6681542 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:20:30 +0200 Subject: [PATCH 14/16] Clean up tooltip-related naming, round 4 --- src/styles/getTooltipStyles.js | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 2314f0aba1f..7a5f591e4ab 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -140,18 +140,18 @@ export default function getTooltipStyles( // We calculate tooltip width based on the tooltip's inner text width so the tooltip is just big enough to fit text and prevent white space. // If the text width is less than the maximum available width, add horizontal padding. // Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. - const wrapperNominalWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1; + const rootWrapperWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1; - const isWrapperSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0; + const isTooltipSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0; // Hide the tooltip entirely if it's size hasn't finished measuring yet. This prevents UI jank where the tooltip flashes close to its expected position. - const opacity = isWrapperSizeReady ? 1 : 0; + const opacity = isTooltipSizeReady ? 1 : 0; - const scale = !isWrapperSizeReady ? 1 : currentSize; - let wrapperTop = 0; - let wrapperLeft = 0; + const scale = !isTooltipSizeReady ? 1 : currentSize; + let rootWrapperTop = 0; + let rootWrapperLeft = 0; - if (isWrapperSizeReady) { + if (isTooltipSizeReady) { // Because it uses fixed positioning, the top-left corner of the tooltip is aligned // with the top-left corner of the window by default. // we will use yOffset to position the tooltip relative to the Wrapped Component @@ -160,7 +160,7 @@ export default function getTooltipStyles( // First, we'll position it vertically. // To shift the tooltip down, we'll give `top` a positive value. // To shift the tooltip up, we'll give `top` a negative value. - wrapperTop = shouldShowBelow + rootWrapperTop = shouldShowBelow ? // We need to shift the tooltip down below the component. So shift the tooltip down (+) by... yOffset + tooltipTargetHeight + POINTER_HEIGHT + manualShiftVertical : // We need to shift the tooltip up above the component. So shift the tooltip up (-) by... @@ -178,7 +178,7 @@ export default function getTooltipStyles( // so the tooltip's center lines up with the center of the wrapped component. // 3) Add the horizontal shift (left or right) computed above to keep it out of the gutters. // 4) Lastly, add the manual horizontal shift passed in as a parameter. - wrapperLeft = xOffset + (tooltipTargetWidth / 2 - tooltipWidth / 2) + horizontalShift + manualShiftHorizontal; + rootWrapperLeft = xOffset + (tooltipTargetWidth / 2 - tooltipWidth / 2) + horizontalShift + manualShiftHorizontal; } return { @@ -195,10 +195,10 @@ export default function getTooltipStyles( ...tooltipVerticalPadding, ...spacing.ph2, zIndex: variables.tooltipzIndex, - width: wrapperNominalWidth, + width: rootWrapperWidth, maxWidth, - top: wrapperTop, - left: wrapperLeft, + top: rootWrapperTop, + left: rootWrapperLeft, opacity, // We are adding this to prevent the tooltip text from being selected and copied on CTRL + A. @@ -225,13 +225,13 @@ export default function getTooltipStyles( top: shouldShowBelow ? -POINTER_HEIGHT : tooltipHeight, // To align it horizontally, we'll: - // 1) Shift the pointer to the right (+) by the half the wrapperNominalWidth's width, - // so the left edge of the pointer lines up with the wrapperNominalWidth's center. + // 1) Shift the pointer to the right (+) by the half the rootWrapperWidth's width, + // so the left edge of the pointer lines up with the rootWrapperWidth's center. // 2) To the left (-) by half the pointer's width, - // so the pointer's center lines up with the wrapperNominalWidth's center. + // so the pointer's center lines up with the rootWrapperWidth's center. // 3) Due to the tip start from the left edge of tooltip Tooltip so we have to remove the // horizontalShift which is added to adjust it into the Window - left: horizontalShiftPointer + (wrapperNominalWidth / 2 - POINTER_WIDTH / 2), + left: horizontalShiftPointer + (rootWrapperWidth / 2 - POINTER_WIDTH / 2), opacity, }, From bdb79c7d7cdf28330288b4d322fc8d8b7fff7c6c Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:22:01 +0200 Subject: [PATCH 15/16] getTooltipStyles: Fix rootWrapperWidth used instead of tooltipWidth --- src/styles/getTooltipStyles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 7a5f591e4ab..0098cf50fd0 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -231,7 +231,7 @@ export default function getTooltipStyles( // so the pointer's center lines up with the rootWrapperWidth's center. // 3) Due to the tip start from the left edge of tooltip Tooltip so we have to remove the // horizontalShift which is added to adjust it into the Window - left: horizontalShiftPointer + (rootWrapperWidth / 2 - POINTER_WIDTH / 2), + left: horizontalShiftPointer + (tooltipWidth / 2 - POINTER_WIDTH / 2), opacity, }, From cd2520fb8da8d35fcbf98a4e94158d1925803913 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Thu, 18 May 2023 22:26:11 +0200 Subject: [PATCH 16/16] getTooltipStyles: Fix unnecessary comment change --- src/styles/getTooltipStyles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/styles/getTooltipStyles.js b/src/styles/getTooltipStyles.js index 0098cf50fd0..985eddbd3de 100644 --- a/src/styles/getTooltipStyles.js +++ b/src/styles/getTooltipStyles.js @@ -137,7 +137,7 @@ export default function getTooltipStyles( const tooltipVerticalPadding = spacing.pv1; const tooltipFontSize = variables.fontSizeSmall; - // We calculate tooltip width based on the tooltip's inner text width so the tooltip is just big enough to fit text and prevent white space. + // We calculate wrapper width based on the tooltip's inner text width so the wrapper is just big enough to fit text and prevent white space. // If the text width is less than the maximum available width, add horizontal padding. // Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly. const rootWrapperWidth = tooltipContentWidth && tooltipContentWidth + spacing.ph2.paddingHorizontal * 2 + 1;