Tooltip: forward and merge inner tooltip props correctly

[ciampo]

What?

Following the work done in #57202 to support nesting Tooltip components in the React tree, this PR improves how forwarded props are merged / passed to the children of the inner tooltip.

Why?

As discussed in #56490 (comment), the approach from #57202 didn't fully consider how props are passed / forwarded / merged to the children of nested Tooltip components.

This change is necessary for allowing nested Tooltips and their children to work as expected.

How?

As suggested by @diegohaz , using Ariakit.Role takes care of merging props correctly.

The other important part was to remove some of the props that are added by the internal context system, which are not intended for children of nested tooltips.

Testing Instructions

Play around with the "Nested Tooltip" storybook example by passing extra styles / classname / event handlers, make sure that they are forward correctly to the children of the inner-most Tooltip component.

Make sure unit tests pass.

[mirka]

I noticed that Tooltip was just recently connected to the Context System (#57202), specifically to deal with this nested tooltip problem.

Have we considered passing isNestedInTooltip via standard React context, rather than the wp-components Context System? Seems like it would make things simpler if that's possible. Also, if some random props added by the system can mess up the nested tooltip case, it might not be a good idea for Tooltip to be connected to the "public" Context System in the first place, since that could be a footgun for consumers that fail to consider the nested case.

[ciampo]

Have we considered passing isNestedInTooltip via standard React context, rather than the wp-components Context System?

That was how the first implementation of #57202 worked, but I seem to remember getting some feedback about using the context system. You comment makes sense though, as it would simplify the implementation and avoid the need for the custom removeExtraPropsAddedByContext utility.

I went ahead and applied the suggested changes, let me know what you think.

[mirka]

Here are some suggested tests to add in relation to this code change:

diff --git a/packages/components/src/tooltip/test/index.tsx b/packages/components/src/tooltip/test/index.tsx
index ed6f7b5f7b..9f7cba6eeb 100644
--- a/packages/components/src/tooltip/test/index.tsx
+++ b/packages/components/src/tooltip/test/index.tsx
@@ -102,6 +102,17 @@ describe( 'Tooltip', () => {
 				screen.queryByRole( 'button', { description: 'tooltip text' } )
 			).not.toBeInTheDocument();
 		} );
+
+		it( 'should not leak Tooltip props to the anchor element', () => {
+			render(
+				<Tooltip data-foo>
+					<button>Anchor</button>
+				</Tooltip>
+			);
+			expect(
+				screen.getByRole( 'button', { name: 'Anchor' } )
+			).not.toHaveAttribute( 'data-foo' );
+		} );
 	} );
 
 	describe( 'keyboard focus', () => {
@@ -481,5 +492,33 @@ describe( 'Tooltip', () => {
 				).not.toBeInTheDocument()
 			);
 		} );
+
+		it( 'should not leak Tooltip classname to the anchor element', () => {
+			render(
+				<Tooltip>
+					<Tooltip>
+						<button>Anchor</button>
+					</Tooltip>
+				</Tooltip>
+			);
+			expect(
+				screen.getByRole( 'button', { name: 'Anchor' } )
+			).not.toHaveClass( 'components-tooltip' );
+		} );
+
+		it( 'should not leak Tooltip props to the anchor element', () => {
+			render(
+				<Tooltip data-outer>
+					<Tooltip data-inner>
+						<button>Anchor</button>
+					</Tooltip>
+				</Tooltip>
+			);
+			const anchor = screen.getByRole( 'button', {
+				name: 'Anchor',
+			} );
+			expect( anchor ).not.toHaveAttribute( 'data-outer' );
+			expect( anchor ).not.toHaveAttribute( 'data-inner' );
+		} );
 	} );
 } );

I'm ok with merging this PR because it is an improvement over trunk (i.e. the classname leakage is fixed), but note that the inner Tooltip props in the nested case is still leaking onto the anchor.

Also, something I noticed in this test file is that it mostly tests Tooltip in combination with Button, which is fine when we're intentionally testing that specific coupling, but may not be ideal in terms of isolation when we consider the particular tooltip implementation that Button has under the hood. Perhaps something to keep in mind as we work on #56490!

[ciampo]

Here are some suggested tests to add in relation to this code change:

Thank you, just pushed to the branch

note that the inner Tooltip props in the nested case is still leaking onto the anchor

The thing is, some props (added by ariakit) need to be forwarded to the anchor — event listeners, for example — but those are implementation details, and therefore it felt wrong to whitelist only certain props. Do you have any ideas in mind?

I noticed in this test file is that it mostly tests Tooltip in combination with Button, which is fine when we're intentionally testing that specific coupling, but may not be ideal in terms of isolation when we consider the particular tooltip implementation that Button has under the hood.

I also noticed that, and I'll probably open a separate PR where:

• I'll tweak Tooltip tests to use vanilla button instead of Button
• if necessary, add a few explicit tests to the Button component in combination with Tooltip

Update: opened #57975

[mirka]

The thing is, some props (added by ariakit) need to be forwarded to the anchor — event listeners, for example — but those are implementation details, and therefore it felt wrong to whitelist only certain props. Do you have any ideas in mind?

Mmm, it should be fine most of the time, since all the explicit props of Tooltip are not forwarded as part of the rest props. And we never forwarded rest props until last week.

Unless we start running into actual problems, we might be able to get away with just reverting this to props: TooltipProps.

gutenberg/packages/components/src/tooltip/index.tsx

Line 44 in 82a5f73

props: WordPressComponentProps< TooltipProps, 'div', false >,

That would set the correct expectations, at least.

[ciampo]

That would set the correct expectations, at least.

Sounds good, opened #58125 to implement your suggestions.