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

Regression: Columns CSS affects readability in WP 6.0 #40952

Closed
peterwilsoncc opened this issue May 10, 2022 · 19 comments
Closed

Regression: Columns CSS affects readability in WP 6.0 #40952

peterwilsoncc opened this issue May 10, 2022 · 19 comments
Labels
[Block] Columns Affects the Columns Block

Comments

@peterwilsoncc
Copy link
Contributor

Description

This was reported on WordPress trac by the WordPress.org user marctison75. The original ticket was WP#55703.

The first part of the issue with the narrow gap seems more important to my mind.

I've tested multiple sites that I managed with WordPress 6.0 RC, and I see that it changes and breaks the layout of the columns block on the front-end.

The gutter between the columns is now very small.

nTk1bvl

The columns are now stacked on tablet while the setting says "Stack on mobile". With WP 5.9, the columns are 50% 50% on tablet (iPad portrait 768px). With WP 6.0 they are 100% which breaks the design of my websites.

6YUe9n8

You can reproduce this issue with the Twenty Twenty-One or the Twenty Twenty theme, Kadence, and Blocksy.

With Twenty Twenty-Two, the gutter stays the same, but there is the same issue with the columns stacking on tablet.

Step-by-step reproduction instructions

Non provided in the original ticket but I'm guessing

  1. Install WP 5.9 with 2020 default theme
  2. Create some content with columns
  3. Update to WP 6.0 RC
  4. Observe changes to columns.

Screenshots, screen recording, code snippet

See above.

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@peterwilsoncc peterwilsoncc added the [Block] Columns Affects the Columns Block label May 10, 2022
@gziolo gziolo added the Needs Testing Needs further testing to be confirmed. label May 10, 2022
@ndiego
Copy link
Member

ndiego commented May 10, 2022

I have confirmed. This was introduced a long time ago in Gutenberg: #37436. If a theme supports blockGap then the gap between columns will default to the blockGap setting in theme.json. Otherwise, it defaults to 0.5em. It used to default to 2em.

@skorasaurus skorasaurus removed the Needs Testing Needs further testing to be confirmed. label May 10, 2022
@ramonjd
Copy link
Member

ramonjd commented May 16, 2022

Hi!

Just to add some context.

#37436 was a necessary step to avoid bugs associated with #37360 (Avoid using CSS variables for block gap styles).

#37360 removed the use of --wp--style--block-gap for all styles other than top-level global styles (styles in the body tag).

0.5em currently the fallback for all blocks that have layout support. The intention was for such blocks to have blockGap support so that the gap can be modified in the editor.

This doesn't do much to help classic themes.

The obvious approach is to bump 0.5em to 2em in layout, but this would affect all blocks that use layout such as the Group, Navigation and Social Icons blocks.

Perhaps our best bet here is to fix a long-standing bug that prevents us from defining blockGap at the block-level.

It's something that was needed to be fixed after #37360. Various people have been trying different ways of achieving it.

So right now, for block themes, this theme.json style doesn't do anything:

	"styles": {
		"blocks": {
			"core/column": {
				"spacing": {
					"blockGap": "2em"
				}
			}
		}
	}

Once we get that working, I think the general approach would be to define a default blockGap value for the Columns block (let's set it back to 2em). That would fix the regression in block themes.

As for classic themes, I'm not sure yet.

This PR:

allowed presets to be printed out in the source for classic themes.

But blockGap is not among them, unless we make a change to add it or alternatively settings.layout values to presets (and add a layout property of blockGap).

In the meantime, we could restore the default in the theme style sheets (for every one over in the themes directory)

.wp-block-columns {
	gap: 2em;
}

That is something we could do straight away. What do folks think?

cc @aaronrobertshaw and @andrewserong who helped write this comment

@ndiego
Copy link
Member

ndiego commented May 16, 2022

Thanks for the context @ramonjd. I think the easiest and quickest fix is to implement the CSS change that you mentioned. Plus those themes already have massive stylesheets.

However, I just tested with Twenty Twenty One and simply using the class .wp-block-columns does not appear to be specific enough to override the container styles. This should work though:

div.wp-block-columns {
	gap: 2em;
}

@ramonjd
Copy link
Member

ramonjd commented May 16, 2022

I just tested with Twenty Twenty One and simply using the class .wp-block-columns does not appear to be specific enough to override the container styles.

Thanks for testing that out @ndiego ! 🙇

I feared we'd have to get specific 😄 but it looks like a pretty safe change here.

I'll add to/test with the other themes over at https://github.com/WordPress/wordpress-develop and if it seems to be kosher will reopen the trac ticket WP#55703

@peterwilsoncc
Copy link
Contributor Author

In the meantime, we could restore the default in the theme style sheets (for every one over in the themes directory)

.wp-block-columns {
	gap: 2em;
}

That is something we could do straight away. What do folks think?

I'm not overly keen on modifying the default themes as a solution to this.

It still leaves the regression for the majority of sites, those that are using custom themes.

@ramonjd
Copy link
Member

ramonjd commented May 17, 2022

I'm not overly keen on modifying the default themes as a solution to this.

It still leaves the regression for the majority of sites, those that are using custom themes.

All good. I'm looking at an alternative.

Since the change in #34439, there has been a fallback value for the -wp--style--block-gap CSS variable for themes that do "not register its own editor style, a theme.json file, or has toggled off "Use theme styles" in preferences."

--wp--style--block-gap: 2em;

But there is no corresponding default -wp--style--block-gap value for block themes.

Rather we've opted to hardcode 0.5em into places such as the layout implementation.

Why don't we:

  1. define a default value of -wp--style--block-gap: 0.5em in the common block library styles, which are loaded across the editor and frontend
  2. Remove the hardcoded 0.5em in Gutenberg and refer only to var( --wp--style--block-gap ).
  3. Then, define --wp--style--block-gap as we wish in the Columns block styles for the frontend and editor.

I think points 1 and 2 should be done anyway.

For example:

:root {
	...
	// Global default for block gap.
	--wp--style--block-gap: 0.5em;
}

[...swap 0.5em with var( --wp--style--block-gap ) ⏲️ ]

And, in columns/styles.scss and editor.scss:

.wp-block-columns {
        ...
        /* The Columns block should have a default block gap of 2em. */
	--wp--style--block-gap: 2em;

I'm still testing, but it seems to work fine.

Screen Shot 2022-05-17 at 11 36 47 am

@andrewserong
Copy link
Contributor

I'm still testing, but it seems to work fine.

If we do that change, would that then set the fallback for all children of the Columns block to 2em, too, which would mean that social icons blocks within the column would then have a fallback of 2em? From memory, I think that was part of the reason for #37360 to resolve #36521.

@ramonjd
Copy link
Member

ramonjd commented May 17, 2022

which would mean that social icons blocks within the column would then have a fallback of 2em?

That's the catch. Getting around inheritance. Looking at it now, but looks pretty glum.

@ramonjd
Copy link
Member

ramonjd commented May 17, 2022

That's the catch. Getting around inheritance. Looking at it now, but looks pretty glum.

I haven't found a reasonable way around this yet.

The conundrum appears to be that we can set a default gap value for columns, but the layout implementation via its .wp-container-* classes will overwrite it every time.

So .wp-block-columns is overwritten by .wp-container-1 because the latter is loaded last.

To make things more fun, in the editor we bump up the specificity: .editor-styles-wrapper .wp-container-1

This is actually desirable to avoid inheritance, say, for when a child block of columns sets its own gap value.

But it's a hindrance for blocks wishing to set their own low-level specificity gap value.

Conclusion: The layout implementation needs to be aware of a specific block's fallback value, if any.

Right now a bug prevents us from doing that in block themes, and it wouldn't work for classic themes anyway.

The only thing I've found I can count on is that classic blocks do not have access to --wp--style--block-gap because it's set based on theme.json values.

I see the following potential options (I'm listing everything I've considered regardless of how silly it might sound, but it's in vague order of reasonableness, that is, from reasonable to absolutely fruity):

  1. Update the default theme.json shipped with Gutenberg and allow layout to check any block-level style values. That is what Theme JSON: use block global style for block gap where no gap value exists in layout.php #39870 is doing and it both fixes the existing bug and allows classic themes to default to a specific blockGap value. See the changes in 88d1f54 The WIP alternative in Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json #40875 might also allow this.
  2. Promote --wp--style--block-gap or an equivalent to become a preset (available to classic themes since Make global styles available to all themes #34334), and then set a default value for columns blocks. We'd then use this as the fallback in the layout implementation. This relies on us fixing the block-level styles bug since we'd need to be able to target the columns block.
  3. Update theme styles as already mentioned. Not desirable as it will not apply to custom themes.
  4. Load a classic-theme specific frontend stylesheet, as we do for the editor see: Remove the default block margins from themes with theme.json file #30375
  5. Use classic theme specific CSS selectors (entry-content or whatever) to deal with classic themes while we work out the block-level styles stuff. Unfortunately unprecedented and probably not-realistic.
  6. Roll back layout support for columns completely (I'm just adding this here despite the assumption that we shouldn't since it's already out there)

Point 1 seems like the most promising so far.

@ramonjd
Copy link
Member

ramonjd commented May 17, 2022

I'm not overly keen on modifying the default themes as a solution to this.

@peterwilsoncc Do you see any problems with updating the core styles anyway?

Fishing for more ideas, but I'm not seeing a universal, stable and/or non-hacky fix for this particular regression otherwise until we work out how to get those pesky block-level blockGap styles to work.

Edit: Or there's this 3-line hack in layout.php :trollface:

	if ( ! $gap_value && ! WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() && 'core/columns' === $block['blockName'] ) {
		$gap_value             = '2em';
		$has_block_gap_support = true;
	}

@andrewserong
Copy link
Contributor

After chatting with @ramonjd, I've put together a potential fix for the gap issue in #41098 if folks would like to have a look / add any feedback. The idea here is to:

  • Add a property to block.json in the spacing block support, where we can declaratively provide a fallback value for blockGap to use in place of the hard-coded 0.5em value.
  • In the layout support, in the editor and on the site frontend, look up the block's settings to see if it provides that fallback value, and use that if available. If it isn't available, then fall back to 0.5em.

It seems to work so far in the very minimal testing I've done. Happy for any feedback, or to shelve it if anyone comes up with a better solution. At the very least, it seems like it didn't require many lines of code to get working 🙂

@ramonjd
Copy link
Member

ramonjd commented May 17, 2022

The columns are now stacked on tablet while the setting says "Stack on mobile". With WP 5.9, the columns are 50% 50% on tablet (iPad portrait 768px). With WP 6.0 they are 100% which breaks the design of my websites.

I'll take a look at this tomorrow, but I suspect it be just as finicky.

Looking at the change PR comments we debated the removal of the tablet-styles. If we'd done nothing they would have been broken anyway:

Update "tablet" viewport rules to allow the columns to grow/shrink to smooth over the removal of the hard-coded 50% width at that breakpoint (the 50% rule had to be removed as we can't calculate the appropriate gap without a CSS variable. See: #37436 (comment))

It's a hard one to balance. The changes were trying to make the layout system more consistent overall, but there's an element of relying on themes to update their CSS overrides to accommodate.

cc some of the folks that were involved @jasmussen @scruffian @youknowriad in case they have some thoughts 🙇

@dianeco
Copy link

dianeco commented May 17, 2022

the 50% rule had to be removed as we can't calculate the appropriate gap without a CSS variable

A possible way to apply the 50% rule without knowing the gap is to switch the columns display to grid instead of flex:

@media (min-width: 600px) and (max-width:781px) {
	.wp-block-columns:not(.is-not-stacked-on-mobile) {
		display: grid;
		grid-template-columns: 1fr 1fr;
	}
}

@cbirdsong
Copy link

This issue is another good argument for the standardized spacing scale proposed in #38998:

The default value for the space between columns should be much larger than the default value for space between paragraphs. A single "gap" value was never going to be enough.

@ramonjd
Copy link
Member

ramonjd commented May 18, 2022

The columns are now stacked on tablet while the setting says "Stack on mobile". With WP 5.9, the columns are 50% 50% on tablet (iPad portrait 768px). With WP 6.0 they are 100% which breaks the design of my websites.

I've tried to reinstate this, and it works to a certain extent but not perfectly. I thought I'd throw it up anyway:

As mentioned in the PR

There is a huge and probably prohibitive gotcha: if a columns block has a style.spacing.blockGap value greater than var(--wp--style--block-gap, 2em) the two column effect will not show.

What would make this particular definition more robust is if we could do the following dynamically: ( var(--wp--style--block-gap, 2em) + style.spacing.blockGap

This only affects block themes as far as I can tell. Classic themes with no theme.json that switching on blockGap are fine.

@peterwilsoncc
Copy link
Contributor Author

@peterwilsoncc Do you see any problems with updating the core styles anyway?

It's a little close to the release so I'd prefer not too but if it's unavoidable then it can be done. Would it need to be for all bundled themes (2010 - 2022) or a subset only?

Sorry, I forgot to answer this earlier today.

@ramonjd
Copy link
Member

ramonjd commented May 18, 2022

Would it need to be for all bundled themes (2010 - 2022) or a subset only?

All bundled themes :(

I think our time is better spent trying to kick #41098 over the line then.

Thanks!

@gziolo
Copy link
Member

gziolo commented May 20, 2022

It should be fixed with #41098. I will prepare all the necessary changes to bring that to WordPress core for 6.0 RC4.

@gziolo gziolo closed this as completed May 20, 2022
@marctison75
Copy link

#41098 fixes the gap issue, but there's still the issue with the columns stacking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block
Projects
No open projects
Archived in project
Development

No branches or pull requests

9 participants