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

Components: Fix Slot/Fill Emotion StyleProvider #38237

Merged
merged 5 commits into from
Jan 28, 2022
+86 −47
Merged

Components: Fix Slot/Fill Emotion StyleProvider #38237

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f6a5d1d
Fix Slot/Fill emotion style provider
ciampo Jan 26, 2022
2146940
Add slotfill storybook example to `useCx`
ciampo Jan 26, 2022
ceb99bb
Use iframe from `@wordpress/block-editor`, add `bubblesVirtually` prop
ciampo Jan 26, 2022
caac47f
Fix slotfill name
ciampo Jan 26, 2022
e15e9de
Add simpler slotfill example
ciampo Jan 26, 2022
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
Prev Previous commit
Next Next commit
Add slotfill storybook example to useCx
  • Loading branch information
@ciampo
ciampo committed Jan 27, 2022
commit 2146940b64554d0ecb370a5fe284bb46c19cba9a
59 changes: 54 additions & 5 deletions packages/components/src/utils/hooks/stories/use-cx.js
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
/**
* Internal dependencies
* External dependencies
*/
import { useCx } from '..';
import StyleProvider from '../../../style-provider';
import { css } from '@emotion/react';

/**
* WordPress dependencies
*/
import { useState, createPortal } from '@wordpress/element';

/**
* External dependencies
* Internal dependencies
*/
import { css } from '@emotion/react';
import { useCx } from '..';
import StyleProvider from '../../../style-provider';
import {
createSlotFill,
Provider as SlotFillProvider,
} from '../../../slot-fill';

export default {
title: 'Components (Experimental)/useCx',
@@ -65,6 +70,50 @@ const Example = ( { args, children } ) => {
return <span className={ classes }>{ children }</span>;
};

export const _slotfill = () => {
const { Fill, Slot } = createSlotFill( 'ToolsPanelSlot' );

const redText = css`
color: red;
`;
const blueText = css`
color: blue;
`;
const greenText = css`
color: green;
`;

return (
<SlotFillProvider>
<StyleProvider document={ document }>
<IFrame>
<IFrame>
<Example args={ [ redText ] }>
This text is inside an iframe and should be red
</Example>
<Fill name="test-slot">
<Example args={ [ blueText ] }>
This text is also inside the iframe, but is
relocated by a slot/fill and should be blue
</Example>
</Fill>
<Fill name="outside-frame">
<Example args={ [ greenText ] }>
This text is also inside the iframe, but is
relocated by a slot/fill and should be green
</Example>
</Fill>
</IFrame>
<StyleProvider document={ document }>
<Slot name="test-slot" />
</StyleProvider>
</IFrame>
<Slot name="outside-frame" />
Copy link
Contributor

Choose a reason for hiding this comment

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

can you try bubblesVirtually prop here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried, but it doesn't seem to make a difference

Screenshot 2022-01-26 at 12 26 48

Copy link
Contributor

Choose a reason for hiding this comment

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

My fix for sure is specific to "bubblesVirtually" implementation which is the one we prefer in Gutenberg in general. (the other is more "historic"). I don't know why it's not working though in storybook, the blue text is the exact same use case as spinner it seems 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This is slightly different though as in the story we've wrapped the Slot in a StyleProvider as well. Are we doing that in usage in Gutenberg?

This is why I was confused because I was actually able to solve the problem with the <StyleProvider><Slot /></StyleProvider> with the context based code that @ciampo and I came up with that I shared here: #37551 (comment)

But it didn't work to fix the spinner.

Which makes me thing that this story, as constructed currently, while it raises an edge case that we need to cover, is actually a red herring when compared to the spinner case (and probably the preview iframe issue you were having before @youknowriad).

In short: I don't believe we've been able to actually reproduce the issue that was being found in Gutenberg in the story yet.

If I'm understanding the fix for the GB issue correctly, the minimal reproduction would just be:

<Iframe>
  <Fill name="test"><Example /></Fill>
</Iframe>
<Slot name="test" />

And indeed, that is what appears to happen in Gutenberg. But in the story, when you try this, it worked perfectly fine.

It's just occurred to me that potentially it could be because the story is running in an iframe? I didn't get around to trying that minimal reproduction story popped out into it's own document so maybe that would make it possible to reproduce (though should theoretically not matter if we consider that any iframe can just be the "root" document of the page, it doesn't have to be the root document of the window).

Who knows!

Copy link
Contributor Author

@ciampo ciampo Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think I managed to work out a Storybook example which breaks before this fix, and works after this fix.

In an effort to replicate as closely as possible the issue that we're trying to reproduce:

  • I replaced the local IFrame component with the Iframe component from @wordpress/block-editor
  • I added bubblesVirtually to both <Slot>s
without the fix in this PR with the fix in this PR, but without bubblesVirtually with the fix in this PR, and with bubblesVirtually
Screenshot 2022-01-26 at 18 48 18 Screenshot 2022-01-26 at 18 55 54 Screenshot 2022-01-26 at 18 50 32

A couple more notes:

  • changing the iframe component seems to be what "allowed the example to be broken"
  • adding bubblesVirtually allowed the slot to receive the fix from this PR

I guess an interesting thing would be to understand what was the difference between the two IFrame components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to get around the IFrame loading with a bit of a hacky approach as nothing else seemed to work me, but the problem seems to be related to the contents of createPortal not being rendered (or maybe simply "seen"?) by Jest / RTL.

These are the changes that I made so far (I've also added some "debugging" spans to the Iframe component to help with troubleshooting):

Click to see diff 
diff --git a/packages/block-editor/src/components/iframe/index.js b/packages/block-editor/src/components/iframe/index.js
index b4e1e44f65..d9cc27a1be 100644
--- a/packages/block-editor/src/components/iframe/index.js
+++ b/packages/block-editor/src/components/iframe/index.js
@@ -267,23 +267,33 @@ function Iframe(
 				title={ __( 'Editor canvas' ) }
 			>
 				{ iframeDocument &&
-					createPortal(
-						<>
-							<head ref={ headRef }>{ head }</head>
-							<body
-								ref={ bodyRef }
-								className={ classnames(
-									BODY_CLASS_NAME,
-									...bodyClasses
+					( () => {
+						return (
+							<>
+								<span>Outside the portal</span>
+								{ createPortal(
+									<>
+										<head ref={ headRef }>{ head }</head>
+										<body
+											ref={ bodyRef }
+											className={ classnames(
+												BODY_CLASS_NAME,
+												...bodyClasses
+											) }
+										>
+											<StyleProvider
+												document={ iframeDocument }
+											>
+												<span>Inside the portal!</span>
+												{ children }
+											</StyleProvider>
+										</body>
+									</>,
+									iframeDocument.documentElement
 								) }
-							>
-								<StyleProvider document={ iframeDocument }>
-									{ children }
-								</StyleProvider>
-							</body>
-						</>,
-						iframeDocument.documentElement
-					) }
+							</>
+						);
+					} )() }
 			</iframe>
 			{ tabIndex >= 0 && after }
 		</>
diff --git a/packages/components/src/utils/hooks/test/use-cx.js b/packages/components/src/utils/hooks/test/use-cx.js
index 181e0beeca..947158f0ad 100644
--- a/packages/components/src/utils/hooks/test/use-cx.js
+++ b/packages/components/src/utils/hooks/test/use-cx.js
@@ -4,14 +4,29 @@
 // eslint-disable-next-line no-restricted-imports
 import { cx as innerCx } from '@emotion/css';
 import { insertStyles } from '@emotion/utils';
-import { render } from '@testing-library/react';
+import {
+	screen,
+	render,
+	waitFor,
+	waitForElementToBeRemoved,
+} from '@testing-library/react';
 import { css, CacheProvider } from '@emotion/react';
 import createCache from '@emotion/cache';
 
+/**
+ * WordPress dependencies
+ */
+import { __unstableIframe as Iframe } from '@wordpress/block-editor';
+import { useState } from '@wordpress/element';
+
 /**
  * Internal dependencies
  */
 import { useCx } from '..';
+import {
+	createSlotFill,
+	Provider as SlotFillProvider,
+} from '../../../slot-fill';
 
 jest.mock( '@emotion/css', () => ( {
 	cx: jest.fn(),
@@ -61,4 +76,65 @@ describe( 'useCx', () => {
 			false
 		);
 	} );
+
+	it( 'should work correctly when using slot/fill in combination with an IFrame', async () => {
+		// const { Fill, Slot } = createSlotFill( 'UseCxTest' );
+
+		// const redText = css`
+		// 	color: red;
+		// `;
+
+		// const TestComponent = () => {
+		// 	const [ iframeIsLoading, setIframeIsLoading ] = useState( true );
+
+		// 	return (
+		// 		<SlotFillProvider>
+		// 			{ iframeIsLoading && (
+		// 				<span data-testid="iframe-loading">Loading</span>
+		// 			) }
+		// 			<Iframe onLoad={ () => setIframeIsLoading( false ) }>
+		// 				<span data-testid="iframe-simple-content">
+		// 					Iframe Content
+		// 				</span>
+		// 				<Fill name="test-slot">
+		// 					<Example args={ [ redText ] }>
+		// 						This text should be red
+		// 					</Example>
+		// 				</Fill>
+		// 			</Iframe>
+		// 			<Slot bubblesVirtually name="test-slot" />
+		// 		</SlotFillProvider>
+		// 	);
+		// };
+
+		const SimplifiedTestComponent = () => {
+			const [ iframeIsLoading, setIframeIsLoading ] = useState( true );
+			return (
+				<div>
+					{ iframeIsLoading && (
+						<span data-testid="iframe-loading">Loading</span>
+					) }
+					<Iframe onLoad={ () => setIframeIsLoading( false ) }>
+						<span data-testid="iframe-simple-content">
+							Iframe Content
+						</span>
+					</Iframe>
+				</div>
+			);
+		};
+
+		const { debug } = render( <SimplifiedTestComponent /> );
+
+		let loadingSpy;
+		await waitFor( () => {
+			loadingSpy = screen.getByTestId( 'iframe-loading' );
+			expect( loadingSpy ).toBeInTheDocument();
+		} );
+
+		await waitFor( () => expect( loadingSpy ).not.toBeInTheDocument() );
+
+		// <span>Outside the portal</span> is printed out
+		// <span>Inside the portal!</span> is NOT printed out (together with the rest of the children)
+		debug();
+	} );
 } );

If anyone has the time to take a look as well it'd be great! Otherwise I plan on spending a little bit more time on it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking if you tried the suggestion in this issue? testing-library/react-testing-library#62

I never really ran into any problems testing portals as far as I can remember but I might have just gotten lucky and avoided testing them directly so far!

Seems like the trick is to pass an extra container: document.body argument to render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately adding { container: document.body } as an additional argument to render triggers a warning, and the renderIntoDocument method (also mentioned in that issue) is deprecated.

I've tried to get inspiration from this example but as soon as the portal doesn't render in the iframeDocument.documentElement, more errors appear (for example, ownerNode here becomes undefined).

Since I've almost spent 2 days on this without managing to get a working unit test, I'm going to merge this PR as-is in order to unblock #37551

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling that the fix from #37551 (comment) might be relevant to #35619 (comment) in a way that the fix in this current PR doesn't seem to be. I will try that out tomorrow.

Did you manage to have a look at this at all? If not, I can try to also look into it later today / on Monday.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was away from my computer yesterday so I didn't get to, but if I have time to look into it today I will!

</StyleProvider>
</SlotFillProvider>
);
};

export const _default = () => {
const redText = css`
color: red;