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

Tooltip: Add test for classname leakage #58182

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Tooltip: Add test for classname leakage #58182

merged 1 commit into from
Jan 24, 2024

Conversation

mirka
Copy link
Member

@mirka mirka commented Jan 24, 2024

Follow-up to #57878

What?

Adds a unit test for the Tooltip component to assert that it doesn't leak the components-tooltip CSS classname onto the anchor element when tooltips are nested.

Why?

Although Tooltip will in fact leak props to its anchor element in the nested case, it will likely not be much of a problem because consumers usually have no reason to set arbitrary props on Tooltip itself.

The test is specifically to guard against future devs from unwittingly connecting Tooltip to the Component Context System, which will automatically add and leak the components-tooltip class, causing visual breakage.

Testing Instructions

Connect Tooltip to the Component Context System. Test should fail.

Diff (Don't mind the TS errors)
diff --git a/packages/components/src/tooltip/index.tsx b/packages/components/src/tooltip/index.tsx
index 87c7506a9c..7dbba65c3d 100644
--- a/packages/components/src/tooltip/index.tsx
+++ b/packages/components/src/tooltip/index.tsx
@@ -8,12 +8,7 @@ import * as Ariakit from '@ariakit/react';
  * WordPress dependencies
  */
 import { useInstanceId } from '@wordpress/compose';
-import {
-	Children,
-	useContext,
-	createContext,
-	forwardRef,
-} from '@wordpress/element';
+import { Children, useContext, createContext } from '@wordpress/element';
 import deprecated from '@wordpress/deprecated';
 
 /**
@@ -25,6 +20,7 @@ import type {
 } from './types';
 import Shortcut from '../shortcut';
 import { positionToPlacement } from '../popover/utils';
+import { contextConnect, useContextSystem } from '../context';
 
 const TooltipInternalContext = createContext< TooltipInternalContextType >( {
 	isNestedInTooltip: false,
@@ -53,7 +49,7 @@ function UnforwardedTooltip(
 		text,
 
 		...restProps
-	} = props;
+	} = useContextSystem( props, 'Tooltip' );
 
 	const { isNestedInTooltip } = useContext( TooltipInternalContext );
 
@@ -138,6 +134,6 @@ function UnforwardedTooltip(
 	);
 }
 
-export const Tooltip = forwardRef( UnforwardedTooltip );
+export const Tooltip = contextConnect( UnforwardedTooltip, 'Tooltip' );
 
 export default Tooltip;

@mirka mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Jan 24, 2024
@mirka mirka requested a review from a team January 24, 2024 13:04
@mirka mirka self-assigned this Jan 24, 2024
@mirka mirka requested a review from ajitbohra as a code owner January 24, 2024 13:04
Copy link

Flaky tests detected in 50da201.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7640593098
📝 Reported issues:

@mirka mirka enabled auto-merge (squash) January 24, 2024 13:52
@mirka mirka merged commit 6f269a8 into trunk Jan 24, 2024
60 checks passed
@mirka mirka deleted the add/tooltip-class-test branch January 24, 2024 14:18
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants