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 the styles for components that use emotion within popovers #54912

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

youknowriad
Copy link
Contributor

Fix regression introduced in #53889

What?

In #53889 default popovers moved from using a "Slot" to just rendering within the "body" of the page. But we missed the fact that when using a Slot, there's an "emotion" StyleProvider that gets automatically applied which ensures that any CSS in JS styles rendered within the Slot are rendered properly to the right document. Since we're now using createPortal directly, we need to ensure the right StyleProvider is applied which would ensure that the styles are applied properly within that particular popover.

Testing Instructions

1- A bit hard cause I'm not sure we have a use-case right now in Gutenberg. You'll have to render a <Popover> or <Dropdown> within a "block" (so within the iframe) and use an "emotion" based component within it. (I have been personally testing using a private plugin that I can't share publicly)

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system labels Sep 28, 2023
@youknowriad youknowriad requested review from ciampo and tyxla September 28, 2023 13:20
@youknowriad youknowriad self-assigned this Sep 28, 2023
@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 28, 2023
@georgestephanis
Copy link
Contributor

Tested and confirmed that this remedies the issue.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Let's add a CHANGELOG entry under Bug fix.

I will also add the label to backport this to the next WordPress 6.4 pre-release (edit: I see that Riad already added it 🎉 )

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks for the fix 👍

@github-actions
Copy link

Flaky tests detected in 71e3e64.
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/6348653055
📝 Reported issues:

@youknowriad youknowriad force-pushed the fix/fix-emotion-components-within-popovers branch from 71e3e64 to a180b6a Compare September 29, 2023 13:06
@youknowriad youknowriad enabled auto-merge (squash) September 29, 2023 13:07
@youknowriad youknowriad merged commit 45ea38f into trunk Sep 29, 2023
50 checks passed
@youknowriad youknowriad deleted the fix/fix-emotion-components-within-popovers branch September 29, 2023 13:48
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 29, 2023
mikachan pushed a commit that referenced this pull request Sep 29, 2023
…rs (#54912)

# Conflicts:
#	packages/components/CHANGELOG.md
@mikachan
Copy link
Member

I just cherry-picked this PR to the commits-for-6.4-beta2 branch to get it included in the next release: ccf7656

@mikachan mikachan removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 29, 2023
fluiddot pushed a commit that referenced this pull request Oct 2, 2023
* Fix the install of system fonts from a font collection (#54713)

* Fix set upload dir test (#54762)

* Site Editor: Prevent unintended actions on the classic theme (#54422)

* Add action and selector to track access to Patterns page

* Add a URL parameter to check if the Patterns page was accessed directly

* Revert lib changes

* Fix critical error in the Post Editor

* Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode`

* Fix critical error in the Post Editor

* Patterns: Fix back navigation after pattern creation (#54852)

* Patterns: Fix category control width in site editor (#54853)

* Patterns: Allow non-user patterns under Standard filter (#54756)

* Performance Tests: Separate page setup from test (#53808)

# Conflicts:
#	test/performance/specs/post-editor.spec.js

* Performance Tests: Support legacy selector for expanded elements (#54690)

* Paragraph: Make 'aria-label' consistent with other blocks (#54687)

* Paragraph: Make 'aria-label' consistent with other blocks

* Update tests

* Try using BC labels in performance tests

* Move lightbox render function to filter (#54670)

* syntax refactor repace strpos with str_contains (#54832)

* Font Library: avoid deprected error in test (#54802)

* fix deprecated call

* removing unwanted line

* Fix the ShortcutProvider usage (#54851)

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Image: Ensure `false` values are preserved in memory when defined in `theme.json` (#54639)

* Modify conditional when parsing config

* Only drop the value if it's undefined.

---------

Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com>

* Use "Not synced" in place of "Standard" nomenclature for patterns (#54839)

* Standard -> Not synced

* Fix broken test

---------

Co-authored-by: Glen Davies <glen.davies@automattic.com>

* Format Library: Try to fix highlight popover jumping (#54736)

* Move mime-type collection generation to a function that can be tested… (#54844)

* Move mime-type collection generation to a function that can be tested.  Refactored to use that function.

* linting changes

* Add unit tests to mime type getter

* Fixed linting errors

* test the entire output array and replace assertTrue by assertEquals

* fixing docs

---------

Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>

* Ensure lightbox toggle is shown if block-level setting exists (#54878)

* Block Editor: Update strings in blocks 'RenameModal' component (#54887)

* Footnotes: Add aria-label to return links (#54843)

* Add aria-label to footnotes front-end links.

* Add visual output. Fix PHPCS errors.

* Remove visual changes, fix in follow-up.

* Font Library: Changed the OTF mime type expected value to be what PHP returns (#54886)

* Changed the OTF mime type expected value to be what PHP returns

* add unit test for otf file installation

---------

Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com>

* Image: Fix layout shift when lightbox is opened and closed (#53026)

* Replace overflow: hidden with JavaScript callback to prevent scrolling

* Disable scroll callback on mobile; add comments; fix scrim styles

The page jumps around when trying to override the scroll behavior
on mobile, so I disabled it altogether. I've also added comments
to clarify this and other decisions made around the scroll handling.

While testing, I realized that the scrim was completely opaque during
the zoom animation, which does not match the design, so I added a new
animation specifically for the scrim to fix that.

* Add handling for horizontally oriented pages

* Move close button so that it's further from scrollbar on desktop

* Fix call to handleScroll() and add touch callback to new render method

* Improve lightbox experience on mobile

To ensure pinch to zoom works as expected when the lightbox
is open on mobile, I added logic to disable the scroll override
when touch is detected. Without this, the scroll override kicks
in whenever one tries to pinch to zoom, and it breaks the experience.

I also revised the styles for the scrim to make it opaque, as having
content visible outside of the lightbox is distracting when
pinching to zoom on a mobile device, and I think having a consistent
lightbox across devices will make for the best user experience.

* Fix spacing

* Add touch directives to block supports

* Fix spacing in block supports

* Rename attribute for clarity

* Update comment

* Update comments

* Fix spacing

---------

Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com>

* Font Library: move font uploads to a new tab (#54655)

* move font uploads to a new tab in the modal

* fix invalid success message and revert the dropzone to modal

* add success notice for font uploads

* make supported file formats message dynamic based on allowed extensions

* update hint text and clear successful message with a fresh upload

* Block custom CSS: Fix incorrect CSS when multiple root selectors (#53602)

* Block custom CSS: Fix incorrect CSS when multiple root selectors

* Fix PHP lint error

* Use `scope_selector` and `append_to_selector` method and update unit test

* Use `scopeSelector` and `appendToSelector` function and update JS unit test

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* Update packages/block-editor/src/components/global-styles/utils.js

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* re-trigger CI

---------

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

* Add new e2e test for creating a pattern (#54855)

* Use list role instead of listbox for patterns (#54884)

* Popover: Fix the styles for components that use emotion within popovers (#54912)

# Conflicts:
#	packages/components/CHANGELOG.md

* Footnotes: use core’s meta revisioning if available (#52988)

# Conflicts:
#	packages/block-library/src/footnotes/index.php

* Remove base url from link control search results  (#54553)

* Expose baseURL setting on Post and Site Editors via block settings

* Strip baseURL from rendered search results

* Only fetch baseURL once in top level component

* Simplify implementation to utilise URL parse functions

* Improve comment wording to avoid referencing undefined var

* Remove superfluous conditional

* Decode URL prior to operations

* Refactor for readability

* Fix where url is not defined

* Revert change to filter util

* Ensure that filterURLForDisplay always receives a string as an arg

* Make e2e test locator less strict

* Prefer pipe

* Force remove trailing slash

* [Site Editor]: Update copy of using  the default template in a page (#54728)

* Remove overflow: hidden from the entity title h1 in the site editor sidebar (#54769)

* Site Editor: Avoid same key warnings in template parts area listings (#54863)

* TemplateAreas use template part clientId as key
* HomeTemplateDetails use template part clientId as key
* Cleanup useSelect hook

* Fix ToolSelector popover variant (#54840)

* Font Library: refactor endpoint permissions (#54829)

* break the checking of user permission and file write permissions

* return error 500 if the request to the install fonts endpoint needs write permissions and wordpress doens't have write permission on the server

* do not ask file write permission on uninstall endpoint

* Standardize the output of install and uninstall fonts endpoints

Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>

* Handle standardized output from install and uninstall endpoints in the frontend

Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>

* Update the install and unintall fonts endpoints unit tests for the new standardized output format

Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>

* fix the refersh call for the library

Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>

---------

Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>

* Don’t use TypeScript files in scripts package (#54856)

* Fix Search Block not updating in Nav block (#54823)

* Avoid setState in render

* Attempt at test coverage

* Improve tests and make them work

* Remove word-wrap property (#54866)

---------

Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Artemio Morales <artemio.morales@a8c.com>
Co-authored-by: Riad Benguella <benguella@gmail.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com>
Co-authored-by: Rich Tabor <hi@richtabor.com>
Co-authored-by: Glen Davies <glen.davies@automattic.com>
Co-authored-by: Jason Crist <jcrist@pbking.com>
Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com>
Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com>
Co-authored-by: Carlos Bravo <37012961+c4rl0sbr4v0@users.noreply.github.com>
Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Adam Silverstein <adamjs@google.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com>
Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com>
Co-authored-by: Pascal Birchler <pascalb@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants