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

Popover: Fix iframe story and add test #53182

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Jul 31, 2023

What?

Fixes the "With Slot Outside Iframe" story of the Popover component in Storybook.

I also added a unit test to ensure that the cross-document rendering is working as expected. This test is also a good first step in unblocking our version-pinned floating-ui dependency (#48402).

Why?

This story was originally (#42903) written using the Iframe component in @wordpress/block-editor. In #46706, there was an implementation change in this Iframe component that introduced some conditional rendering logic that relied on the block editor store. This change broke the isolated example in Storybook, and made the Iframe component unsuitable for non-WP usage.

How?

I replaced the Iframe component with a generic iframe component that is suitable for isolated testing.

Testing Instructions

✅ Tests pass

  1. npm run storybook:dev
  2. Go to the Popover "With Slot Outside Iframe" story in Storybook and see that it works again.

Screenshots or screencast

Expected story

CleanShot.2023-08-05.at.02.57.04.mp4

@mirka mirka added [Package] Components /packages/components Storybook Storybook and its stories for components labels Jul 31, 2023
@mirka mirka requested review from chad1008 and brookewp July 31, 2023 14:23
@mirka mirka self-assigned this Jul 31, 2023
@mirka mirka requested a review from ajitbohra as a code owner July 31, 2023 14:23
@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Jul 31, 2023
@mirka mirka added [Type] Developer Documentation Documentation for developers and removed [Type] Bug An existing feature does not function as intended labels Jul 31, 2023
Copy link
Contributor

@brookewp brookewp left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! I saw a few odd things happening in my testing.

In Chrome, I see that the popover disappears as soon as I start to scroll within the container:

chrome

But I believe it should stay open until dismissed? And in Firefox, it starts out of view, and the popover can be scrolled, but the container cannot:

Screenshot 2023-08-01 at 11 16 06 AM

The container iframe titled 'My iframe' is empty in FF (it doesn't contain any elements in the body):

Screenshot 2023-08-01 at 11 32 54 AM

Vs Chrome:

Screenshot 2023-08-01 at 11 33 01 AM

The max-height of the popover also varies between them (30px in FF, 178px in Chrome)

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Flaky tests detected in 3ec5c25.
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/5739400949
📝 Reported issues:

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Testing this was odd for me. In the PR's current state (after the FF fix in 3ec5c25), Firefox looks great, but Chrome now fails to render any content inside the iframe for me. It looks very similar to what @brookewp reported in FF before that fix was added:
Screenshot 2023-08-03 at 10 56 29
Chrome, with the FF fix applied

Screenshot 2023-08-03 at 10 57 24
Chrome markup with the FF fix applied

If I drop that one commit, Chrome works again, (FF of course doesn't). I did notice the popover overlays the container's border when scrolling:
Screenshot 2023-08-03 at 10 49 08
Scrolling within the container on Chrome, WITHOUT the Firefox fix

I've tested multiple times, but it feels weird to me that the FF fix would break Chrome... it's possible there's something funky happening locally for me if this isn't reproducible by others 🤔

Copy link
Contributor

@stokesman stokesman left a comment

Choose a reason for hiding this comment

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

I tried this in Chrome and Orion and saw the same as Chad reported. I took a guess that adding a srcDoc might somehow help. That seemed to do the trick and it started working in those browsers. Firefox remained okay too.

@mirka
Copy link
Member Author

mirka commented Aug 4, 2023

Thanks for testing everyone! And especially @stokesman for figuring out a fix 🙏 I actually couldn't reproduce this Chrome issue even with a hard refresh, but an Incognito tab somehow made it happen 😅 Should be working in all three browsers now.

I did notice the popover overlays the container's border when scrolling

I believe this is the expected behavior for the original story, because the popover content is being rendered in a slot outside of the purple border iframe 👍

Copy link
Contributor

@stokesman stokesman left a comment

Choose a reason for hiding this comment

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

Looks 🚢 shape to me.

Copy link
Contributor

@stokesman stokesman left a comment

Choose a reason for hiding this comment

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

😆

@mirka mirka merged commit 054344c into trunk Aug 9, 2023
50 checks passed
@mirka mirka deleted the fix/popover-iframe-story branch August 9, 2023 21:44
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components Storybook Storybook and its stories for components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants