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

NavigatorProvider: Exclude size value from contain CSS rule #57498

Merged
merged 2 commits into from Jan 3, 2024

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Jan 3, 2024

Fixes #57494
Related to #56909

What?

This PR excludes the size value by changing the value of the contain CSS property applied to the NavigatorProvider component from strict (equivalent to size layout paint style) to content (equivalent to layout paint style).

As a result, this PR solves the following three problems as far as I've researched.

The Preferences modal content does not display at all in the mobile viewport

Reported by #57494. The Preferences modal component uses the NavigatorProvider when it's a mobile viewport.

Global style sidebar layout

  • Go to the Site Editor and go to Global Styles > Blocks
  • Fixes unintended changes to sidebar width and scrollbar edge position
trunk this PR
image image

Content of one story is not displayed in Storybook

When you access the URL below, nothing should be displayed.

https://wordpress.github.io/gutenberg/?path=/story/components-experimental-navigator--skip-focus

Why?

contain: strict is equivalent to contain: size layout paint style. According to MDN, if a value contains size, it appears to have a size of zero if no size is explicitly specified for that element.

If you turn on contain: size you need to also specify the size of the element you have applied this to using contain-intrinsic-size (or the equivalent longhand properties). It will end up being zero-sized in most cases, if you don't manually give it a size.

It's not a problem if the element has an explicit height or flex-glow:1 etc., but if not, problems like those reported by #57494 will occur.

How?

We can probably fix the problem by leaving it as contain: strict and adjusting the CSS that is causing the problem. However, I think we should change from strict to content to avoid such issues at the component level, to avoid impacting consumers and forcing them to set an explicit height. There may be a performance drop compared to strict, but I personally think this is acceptable.

Testing Instructions

Please confirm that the three issues mentioned at the beginning of the explanation have been resolved.

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jan 3, 2024
@t-hamano t-hamano self-assigned this Jan 3, 2024
@t-hamano t-hamano force-pushed the navigator/fix-elemeht-height branch from f146311 to 8a8114d Compare January 3, 2024 07:38
@t-hamano t-hamano requested a review from a team January 3, 2024 08:07
@t-hamano t-hamano marked this pull request as ready for review January 3, 2024 08:07
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.

Thank you for working on the fix and for the detailed PR description 🚀

@ciampo ciampo merged commit 2114141 into trunk Jan 3, 2024
59 checks passed
@ciampo ciampo deleted the navigator/fix-elemeht-height branch January 3, 2024 16:39
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preferences Modal: No content is displayed in mobile viewport
2 participants