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

Only apply default margin from global styles if root body does not have margin set in theme.json #36958

Closed
wants to merge 1 commit into from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 29, 2021

Description

Note there is now also an alternative approach to this in #36960.

In #36147 we learn that the Theme JSON class automatically applies a margin: 0 to the body element. Unfortunately it does so in a way that means if you use theme.json to define a margin on the body element, the default margin: 0 will still take precedence.

For example applying this to theme.json will still leave you with the default margin: 0 on the body, rather than the expected value of margin: 0 5rem:

{
	"styles": {
		"spacing": {
			"margin": "0 5rem",
			"padding": "0"
		},
	}
}

This PR fixes this by only applying the default margin: 0 if the root spacing does not have a margin declaration.

Closes #36147

Question

Need to consider what happens how to handle things if the spacing -> margin is only set on a single axis (e.g. margin-left or margin-bottom.

An alternative implementation might be to simply allow the margin: 0 to be set but to ensure that any margin declarations coming from theme.json are applied afterwards rather than before as they are currently. This would allow the theme.json declarations to take precedence.

Update: I have this approach as a PR in #36960.

How has this been tested?

⚠️ ⚠️ ⚠️ ⚠️ You cannot currently test this because the class-wp-theme-json-gutenberg.php file is not loaded when WP 5.9 is active. Rather the Core version of the class is used. This PR now includes changes from #36957 which allow you to test this PR. They will be removed prior to merge You will need to use WordPress 5.8 (that is five point EIGHT not NINE) to test this PR. See #36957 (comment) for reasons. ⚠️ ⚠️ ⚠️

@oandregal is working on a fix for this at which point you will be able to follow the testing instructions below:

  1. Make sure you are running WP 5.8!
  2. Install and activate TT1 blocks (or block Theme or your choice). You should make this theme local in order that you can edit the theme.json of the Theme.
  3. Load the front of the site. Inspect the body element and notice the margin: 0 applied to the body element.
  4. Now edit theme.json and add the following under the styles section:
{
	"spacing": {
		"margin": "0 5rem",
		"padding": "0"
	},
}
  1. Reload the front of the site and inspect the body. Notice how the margin: 0 is no longer applied and instead the applied style of the body is margin: 0 5rem.

⚠️ Watch out: TT1 blocks applies margin: 0 to body in its style.css file. Don't get confused when verifying the results of this PR which have nothing to do with the manually created style.css file.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getdave getdave marked this pull request as ready for review November 29, 2021 13:27
@getdave getdave changed the title Only apply default margin if root does not have margin set Only apply default margin from global styles if root body does not have margin set in theme.json Nov 29, 2021
@getdave getdave added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended labels Nov 29, 2021
@getdave getdave self-assigned this Nov 29, 2021
@getdave
Copy link
Contributor Author

getdave commented Nov 29, 2021

cc @overclokk as you raised the original Issue.

Comment on lines 1064 to 1066
if ( ! isset( $node['spacing']['margin'] ) ) {
$block_rules .= 'body { margin: 0; }';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the changeset to focus on in this PR

Copy link

@overclokk overclokk Nov 29, 2021

Choose a reason for hiding this comment

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

I was thinking why not adding that value as default in theme.json in core, so the theme can override that vales without an if statement

Copy link
Member

Choose a reason for hiding this comment

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

Relevant to this fix is the original PR by @youknowriad and this comment #35421 (comment)

cc @ntsekouras

Copy link
Contributor

@ntsekouras ntsekouras Nov 30, 2021

Choose a reason for hiding this comment

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

I considered whether this should be applied in the default theme.json instead but decided against because while right now the default theme.json styles applies to body, I'm wondering if it's better if they apply to .wp-site-blocks because that's the actual container of everything (imagine defining a layout style at the root level, you want it to apply there and not on body)

@youknowriad's comment makes sense but I'm wondering if changing this now would cause some themes to break. Should we handle margin now in body and consider a change like this after 5.9 🤔 ?

Setting a default in theme.json sounds good, but we also need to handle both notations margin: 10px and margin:{top: 10px}.

--cc @jasmussen @kjellr

Copy link
Contributor Author

@getdave getdave Nov 30, 2021

Choose a reason for hiding this comment

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

My personal opinion is that this PR is a bug fix and we should strive to keep it as such.

I understand there are other things to explore, namely:

  1. Making this part of default theme.json as suggested in Only apply default margin from global styles if root body does not have margin set in theme.json #36958 (comment).
  2. Applying the margin: 0 to the .wp-site-blocks element as per Remove default padding/margin on the body of the page and editor canvas #35421 (comment).

...however the aim of this PR is to fix the bug ready for WP 5.9 Beta 2 (given that Beta 1 deadline has passed) and it seems to me - given we're in a "bug fix only" period - we should therefore keep the scope of fixes limited.

Personally I would prefer we pursued #36960 as this option retains the current margin: 0 on the body which is what Themes have come to expect, but it also allows Themes to overide that setting via theme.json. That seems to cover the use case stated in the original Issue #36147, namely:

If I add a margin property to the theme.json at the top level styles the style generated is overwritten by a style added by the gutenberg plugin.

Given the overhead of raising and testing Global Styles PRs (PRs in both Gutenberg and Core) it would be great if we could decide on the preferred approach in order that we can get this issues resolved with maximum efficiency. I appreciate your understanding.

The decisions to be made are:

  1. Do we agreed that we should proceed with this "limited" fix scope (i.e. not pursuing other explorations)?
  2. Which PR should we pursue?:

Thanks all 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we agreed that we should proceed with this "limited" fix scope (i.e. not pursuing other explorations)?

I personally agree with this.

Which PR should be pursue

I also prefer the selector ordering PR (#36960 )

Copy link
Contributor

Choose a reason for hiding this comment

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

comment makes sense but I'm wondering if changing this now would cause some themes to break. Should we handle margin now in body and consider a change like this after 5.9 🤔 ?

As a high level principle, I like the idea of serving out good defaults but giving full control to themers to trivially style everything with theme.json, style.css or otherwise.

In this case it sounds like the risk is themes assuming body { margin: 0; } to be present, and this PR then removing it, is that correct?

In that case, have we considered adding this rule to create a soft default?

:where(body) {
	margin: 0;
}

In theory that should output the existing rule but be trivially overridden by theme.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it sounds like the risk is themes assuming body { margin: 0; } to be present, and this PR then removing it, is that correct?

I think it's not only about margin, but all the other 'root' styles to be applied to .wp-site-blocks and not body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasmussen An alternative fix is #36960

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@getdave getdave 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 Nov 29, 2021
@getdave getdave force-pushed the fix/default-theme-json-body-margin branch from 61c8ea6 to a598ded Compare November 30, 2021 09:21
@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

In light of #36957 (comment) I've removed the fix I cherrypicked from #36957.

I also rebased to pick up recent changes in trunk.

You now need to test this PR against WordPress 5.8.

@getdave getdave force-pushed the fix/default-theme-json-body-margin branch from a598ded to aea070f Compare November 30, 2021 09:30
@overclokk
Copy link

Still I don't understand what is the issue of using the theme.json in core as default values instead of the if statement.

@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

I'm going to close this out to direct attention towards #36960.

If folks decide that's not the best option we can always reopen this one.

@getdave getdave closed this Nov 30, 2021
@getdave
Copy link
Contributor Author

getdave commented Nov 30, 2021

Still I don't understand what is the issue of using the theme.json in core as default values instead of the if statement.

@overclokk Let me spin up a PR as an alternative approach. But I think the problem will be that if the Theme uses the object notation for margin (e.g. margin-left: 0) then that will override the entirety of the margin: 0 reset in Core. I've listed several problems as I see them.~

Another possible reason for including the if() statement is to make it clear that this is a CSS reset rather than some abstract default provided by Core.

I've spun up #36996 as an alternative which includes your suggestion. I've listed several concerns I have.

@youknowriad youknowriad deleted the fix/default-theme-json-body-margin branch December 1, 2021 07:54
@noisysocks noisysocks 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 Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The margin added at the top level styles (for body) in theme.json is override by a margin added from gutenberg
6 participants