Components: Fix Slot/Fill Emotion StyleProvider
#38237
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
force-pushed
the
fix/components-style-provider-slot-fill
branch
from
82e7b62
to
2879634
Compare
|
Size Change: -2 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
I also spent some time trying to create a Storybook example illustrating the issue that is PR aims to fix, but I couldn't come up with a good example — can someone else also help here? While doing so, I added one of the examples that @sarayourfriend described in #37551 (comment), which still appears broken (the black text is supposed to be blue):
Could this be related to a separate problem with |
ciampo
added
[Feature] Component System
youknowriad
reviewed
Jan 26, 2022
| <Slot name="test-slot" /> | ||
| </StyleProvider> | ||
| </IFrame> | ||
| <Slot name="outside-frame" /> |
There was a problem hiding this comment.
can you try bubblesVirtually prop here?
There was a problem hiding this comment.
Tried, but it doesn't seem to make a difference
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
IFramecomponent with theIframecomponent from@wordpress/block-editor - I added
bubblesVirtuallyto 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 |
|---|---|---|
 |
 |
 |
A couple more notes:
- changing the
iframecomponent seems to be what "allowed the example to be broken" - adding
bubblesVirtuallyallowed theslotto receive the fix from this PR
I guess an interesting thing would be to understand what was the difference between the two IFrame components?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There was a problem hiding this comment.
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!
sarayourfriend
approved these changes
Jan 26, 2022
ciampo
force-pushed
the
fix/components-style-provider-slot-fill
branch
from
686e848
to
e15e9de
Compare
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR contains a fix proposed by @youknowriad in #37551 (comment) which fixes an issue related to Emotion's
StyleProviderwhen used in combination withSlot/Fill. In Riad's words:This PR also updated the
useCxStorybook examples to include a couple of examples that wouldn't work before the fix in this PR. The main thing there was to use theIFrameelement from@wordpress/block-editor, instead of a local replica.We also tried to add a unit test but didn't manage due to the complexity introduced of the aforementioned
IFramecomponent.Testing Instructions
Screenshots
Types of changes
Fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).