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

Global Styles: consider loading user styles after theme styles #34141

Closed
jffng opened this issue Aug 18, 2021 · 18 comments
Closed

Global Styles: consider loading user styles after theme styles #34141

jffng opened this issue Aug 18, 2021 · 18 comments
Assignees
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress

Comments

@jffng
Copy link
Contributor

jffng commented Aug 18, 2021

Description

Currently styles load in the following order:

  1. Block library styles
  2. Global styles — core + theme.json + user styles set via the site editor. All of these are combined into global-styles-inline-css
  3. Theme styles

For example, this is from emptytheme:

Screen Shot 2021-08-18 at 10 41 12 AM

This loading order can cause unexpected behavior when the theme wants to specify some defaults that are not entirely possible via theme.json, but still allow for user styles to take precedence. For an example of where this can create unexpected behavior, see Automattic/themes#4435

What is your proposed solution?

Within global styles, it could be useful to separate user styles from the core + theme.json styles. This way, the loading order could be:

  1. Block library styles
  2. Global styles — core + theme.json
  3. Theme styles
  4. Global styles — user styles
@jffng jffng added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 18, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Aug 18, 2021
@creativecoder
Copy link
Contributor

Potential fix for this issue in #34147

@oandregal
Copy link
Member

oandregal commented Aug 19, 2021

👋 Would like to understand what's the issue you're experiencing.

The styles are enqueued as they are expected: first the core styles (block-library, global styles), then the theme styles. This has been always like this and the introduction of global styles respected this order. It's intentional, so the theme can overwrite the core styles just by being enqueued later (no extra specificity required).

@oandregal
Copy link
Member

oandregal commented Aug 19, 2021

It'd be helpful if "how to reproduce" instructions were posted in this issue for people that are not aware of how to work with a theme like BlockBase. I've struggled a bit to make it work. For reference, this is what I've done after some trial&error:

Now, the testing instructions:

  • Go to the site editor.
  • Add a button block.
  • Via the global styles sidebar, select text & background colors for the button (via "By Block Type" menu tab, "Button" section).

The expected result is that user-provided styles should be reflected in the site editor & front-end. They're not.


This is what I see: the reason why the user styles are not applied is that the theme overrides them. This is what BlockBase is doing:

  1. Uses theme.json to provide button styles. These will be used by the site editor to consolidate with user styles, if there's some, only outputting theme or user styles.
  2. The theme enqueues a stylesheet that overwrites the same colors provided via global styles, including future user choices as well:
.wp-block-button.wp-block-button__link, .wp-block-button .wp-block-button__link {
  color: var(--wp--custom--button--color--text);
  background-color: var(--wp--custom--button--color--background);
}

Why does BlockBase need to provide a stylesheet that overrides the styles already set via its own theme.json?

@jffng
Copy link
Contributor Author

jffng commented Aug 19, 2021

@nosolosw thank you for the thoughtful reply. Apologies for not providing better testing instructions or rationale for why this change could be useful. My general assumption was that if a user makes a customization via global styles UI, it should always win / override the theme's preferences.

Why does BlockBase need to provide a stylesheet that overrides the styles already set via its own theme.json?

The short answer is, we shouldn't be. The principle behind blockbase is that CSS should only exist where theme.json does not supply a way to style the block or element. But in the case of the button block, there are a few places that rule falls apart a bit:

  • Not all buttons are button blocks e.g. comments, forms, search block, file block. In Blockbase and its children, we sought to provide a simple way to customize the color of all buttons using the existing design tools. To do this, we used a semantic color system that maps a color preset to a custom variable that is used in both theme.json and CSS where there is not yet an existing mechanism to target the button. That way, if the user changes the primary color of the palette, buttons everywhere would update accordingly. We applied this patch CSS to button blocks too, but as demonstrated in Blockbase: Ponyfill css overrides block-level global styles for Button block Automattic/themes#4435, we need to be more selective about this.
  • To provide button hover styles according to a theme design target we're aiming to achieve, we need more specific control over and access to the padding and border values (since the design of the button hover states use an outline / border effect, we can't just rely on the theme.json padding value otherwise the size of the button will look different between hover/non-hover states).

All that said, I think we can close this issue; it can be resolved mostly in Blockbase by removing the theme.json configurable CSS for the button block, and I don't think the use cases described above justify changing the loading order.

@jffng jffng closed this as completed Aug 19, 2021
@oandregal
Copy link
Member

Thanks for sharing the extra context, it's very valuable to me: it shows me practical cases in which the theme.json is not yet fully functional.

This is what I hear:

  1. There're components of the design for which we don't have a way to change all "components of a type" at once. Example: the button block, the button within search, etc. should be addressable at once.

  2. There're components of the design that can't be addressed yet. Example: the button in the search block.

  3. Some designs require coordination across properties for different states (rest vs hover). Example: maintaining spacing while changing padding&border values.

I don't have answers yet, but distilling this feedback for our future selves will help us get there.

@oandregal
Copy link
Member

I keep thinking about the order of styles. I do think the current order of styles is correct as far as core and theme styles go:

  1. Block styles: block-library, 3rd party block styles, global-styles-inline-css (core and theme).
  2. Theme stylesheets.

However, there's the gray area of user styles that will be introduced with the site editor. I've been operating under the assumption that user styles provided via the GS sidebar would also be part of the global-styles-inline-css stylesheet, as they work today. However, it can be argued that it should work like this:

  1. Block styles: block-library, 3rd party block styles, global-styles-inline-css (core and theme).
  2. Theme stylesheets.
  3. User styles: global-styles-user-inline-css.

@jffng Would this approach help address the issue with padding & border? Or would it be more problematic as user styles would have higher priority by being later in the chain?

@oandregal
Copy link
Member

At #34196 I've shared an idea that could be helpful for the issue with buttons.

@MaggieCabrera
Copy link
Contributor

However, it can be argued that it should work like this:

1. Block styles: block-library, 3rd party block styles, `global-styles-inline-css` (core and theme).

2. Theme stylesheets.

3. User styles: `global-styles-user-inline-css`.

Yes! I think this is what was happening exactly in this case. Whatever the user changes should take precedence over what the theme initially offers.

@jffng jffng changed the title Wrong stylesheet loading order on front-end Global Styles: consider loading user styles after theme styles Aug 20, 2021
@jffng
Copy link
Contributor Author

jffng commented Aug 20, 2021

Would this approach help address the issue with padding & border? Or would it be more problematic as user styles would have higher priority by being later in the chain?

Yes I think so. We want to provide a default for the button padding that combines a border and padding value, which we need to do via CSS. But we can't lower the specificity of the theme CSS so that global styles inline CSS will win if the user has set a preference here.

It gets a bit complicated by the "outline" block style variation, but maybe that's a separate issue.

Going to re-open this for a bit in case other folks want to add to the discussion.

@jffng jffng reopened this Aug 20, 2021
@oandregal
Copy link
Member

Makes sense to me, thanks for reopening. Would you mind also updating the issue description to reflect the idea of separating global styles (core, theme) and global styles (user)?

@uniquesolution
Copy link

I had posted on wordpress forum regarding this issue https://wordpress.org/support/topic/how-to-enqueue-stylesheet-before-block-library-style/ , enqueue and dequeue not work with style tags added by wordpress, so I can't replace it after my bootstrap.css file, I want to keep bootstrap.css load first then the wodpress style tags, so that theme.json default css overwrite bootstrap, currently no way to change order of css loading, or may be there is some way but I am not aware about that ?

@oandregal
Copy link
Member

Hey @jffng and @MaggieCabrera I've looked at this and wanted to share my findings.

So, as I see this, the theme wants to maintain linked CSS values, which is challenging upon user changes. For example: how to maintain the height of the button (padding+border) for different statuses (rest state, hover state).

Let's run with some code. This is the setup: a post with a button (default state, no style variation attached, which is a different issue). In the hover state, the design needs to change colors and the button gains a border it does not have on the rest state.

I've tested with the empty theme. Added these styles to its theme.json:

    "styles": {
        "blocks": {
            "core/button": {
                "color": {
                    "background": "hotpink",
                    "text": "yellow"
                },
                "spacing": {
                    "padding": "1em"
                }
            }
        }
    }

And then to target the hover state, added the following CSS to its stylesheet:

.wp-block-button__link:hover {
  background-color: yellow;
  color: hotpink;
  padding: .8em; /* Reduced padding from the rest state */
  border: .2em hotpink solid; /* It gains a border it didn't have */
}

This works fine: the button keeps the height on hover.

What happens when the user changes the padding via the global styles sidebar? The height of the button in the rest and hover states will be different. The reason is that the hover state (custom CSS) doesn't have access to the new height target. This can be tested by going to the site editor and changing the padding of the button block.

Would this be solved if we split user styles to a different stylesheet?

No, the issue will be the same (height is not maintained).

Potential fix

What if the theme made the link between those values (padding & border) explicit? Something like this would work:

  1. Change the custom CSS to this:
.wp-block-button__link:hover {
  color: hotpink;
  background-color: yellow;
  padding: calc(var(--wp--theme--button-padding)*0.8);
  border: calc(var(--wp--theme--button-padding)*0.2) hotpink solid;}

Note the new --wp--theme-button-padding custom property.

  1. --wp--theme--button-padding is a custom property that the theme takes care of enqueuing itself. Its value is the padding value coming from theme.json (either the theme or user).

We want this result:

:root {
  --wp--theme--buton-padding: <this comes from the theme.json>
}

Which can be achieved by adding a few lines of code to the existing function to enqueue the custom styles:

function emptytheme_scripts() {
    // Enqueue theme stylesheet, that contains the hover status styles.
    wp_register_style( 'emptytheme-style', get_template_directory_uri() . '/style.css', array(), wp_get_theme()->get( 'Version' ) );

    // THE NEW CODE.
    // Take the value of the target height and inline it as --wp--theme--button-padding.
    $value  = gutenberg_get_global_styles( array( 'spacing', 'padding' ), 'core/button' );
    if ( is_array( $value ) ) { // User values come as an array.
        $value = $value['top'];
    }
    $output = ':root { --wp--theme--button-padding: ' . $value . '; }';
    wp_add_inline_style( 'emptytheme-style', $output );
   // END OF NEW CODE.

    wp_enqueue_style( 'emptytheme-style'  );
}
add_action( 'wp_enqueue_scripts', 'emptytheme_scripts' );

This is the gist of it. Note that this is a simplification, as the padding values (top, right, bottom, left) can be different, etc. It's just a sample to show the idea.

@oandregal
Copy link
Member

I'm inclined to close this issue. I don't see uses cases that would be fixed from splitting global styles into theme and user.

The fact there are some states of the block that aren't absorbed yet via theme.json and the global styles UI is a known challenge. This happens at least for style variations and hover/active/etc statuses, but there may be more. The fix seems to actually be able to target those states via theme.json and the global styles UI. I've started to look into style variations in theme.json at #33232 but had to stop due to other priorities. I'd like to resume that work post-5.9.

Note that there may be other issues that can be fixable right now. For example, I've just run into a specificity issue in which the outline variation styles override theme.json styles that I'm fixing at #35968 I'd appreciate some help to land that one.

Thoughts?

@MaggieCabrera
Copy link
Contributor

@oandregal on a quick look that looks like the best solution, we are all in for having a CSS variable that we can play with that keeps the user changes. But I'm thinking this will break if those changes are done on a per-block basis instead of in the global styles.

@oandregal
Copy link
Member

But I'm thinking this will break if those changes are done on a per-block basis instead of in the global styles.

Would you expand a bit on what you mean by this? I'm not sure I follow.

@MaggieCabrera
Copy link
Contributor

But I'm thinking this will break if those changes are done on a per-block basis instead of in the global styles.

Would you expand a bit on what you mean by this? I'm not sure I follow.

if I user changes the padding of a specific button (instead of all of them in global styles), the hover effect of that button will not respect the height.

@jffng
Copy link
Contributor Author

jffng commented Oct 27, 2021

if I user changes the padding of a specific button (instead of all of them in global styles), the hover effect of that button will not respect the height.

In this case, won't the hover effect be ignored because the padding values are in-lined and will override any padding values of the hover CSS, so it's not an issue?

I'm inclined to close this issue. I don't see uses cases that would be fixed from splitting global styles into theme and user.

👍 I think we can close this one too, thanks for explaining and providing some sample code. I understand your suggestion, although I do wish the theme wouldn't have to add such code to enable coordination among style values.

If we encounter some use cases where it is useful to separate the user styles from global, we can re-open.

@jffng jffng closed this as completed Oct 27, 2021
@via-lars
Copy link

via-lars commented Aug 19, 2022

this should have been no discussion and is nothing to not understand. user customisation always last.

I see a question is "what is a user customisation?":

eg. Theme Builder adjustments by "user" (/editor) should override theme pre-styling.

but Site Editor "customisation" is basically building the site styling, that can all be before theme styling.
Post Editor style adjustments however are "user customisation".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants