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

Apply ! important to user preset styles for theme.json themes only #39660

Closed

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Mar 22, 2022

This is a proposed interim solution to the issues outlined here, with the view to exploring removal of the ! important from user presets across all themes.

What?

Limits the addition of ! important to user preset property values to themes with a theme.json file. The problems outlined by theme authors were all related to Classic themes (that we are aware of), so this change puts Classic themes back to the pre 5.9 state. A Classic theme can still opt in to have the ! important applied to user presets by adding a theme.json file to the theme.

Why?

The blanket addition of ! important to these property values causes some issues for theme authors, particularly with the likes of responsive font sizing.

How?

Checks for the existence of a theme.json file before applying ! important

Testing Instructions

  • With WordPress 5.9.2 install Michelle 1.2.0.
  • Create a page and add a content from this gist.
  • Publish the page and check how it looks on frontend. The font size in both columns should be equal but if you reduce the screen size the font sizes between columns become uneven because in the left column the ! important in the GB presets is overriding the responsive typography CSS
  • Now run the same with this branch checked out and built and the fonts should resize in both columns as expected.
  • Select TT2 theme and add a paragraph and set the typography size. Check frontend and make sure the font size class has ! important set, eg.
has-large-font-size {
    font-size: var(--wp--preset--font-size--large)!important;
}

@oandregal
Copy link
Member

Hey, just commenting on direction, not implementation: if we make it optional, how do we make sure users can style blocks whose selector has a higher specificity? Example.

Personally, I feel that our goal should be to explore whether we can remove !important in user presets for all the themes.

Two potential approaches we could explore are:

  • Going block by block and reduce its specificity (using :where, only use single classes or less for specificity, etc) and make sure the existing preset classes work when applied to every state (with and without style variations, etc).
  • Advance the work to generate all block styles in the server so we can make smarter decisions depending on the block's content (perhaps that involves outputing .wp-block-ID { color: value } instead of presets, etc).

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Mar 28, 2022

if we make it optional, how do we make sure users can style blocks whose selector has a higher specificity? Example.

@oandregal The theory was to have it turned on by default for theme.json themes so we can at least ensure that the styles will be applied for these users, and if a theme author decides to disable it, then the burden is on them to ensure that their styles are of the correct specificity to not override the user applied ones. With Classic themes, although they don't get the enforced application by default, they can continue to use their responsive typography CSS, etc. and can manually fix any specificity issues with other styles, or just add the theme.json if they want the ! important application by default.

I think it would be good if we can see the above as a short term temporary fix to get around the issues that ! important is causing classic theme authors, while one or more of the options you mention below, which will likely take a little longer, are worked on - or if you think we can get one of the below solutions out the door quick then we could ditch this idea.

  • Going block by block and reduce its specificity (using :where, only use single classes or less for specificity, etc) and make sure the existing preset classes work when applied to every state (with and without style variations, etc).

This would be a useful exercise I think - only fixes things for core blocks of course, but we could provide guidance to 3rd party block authors on how to ensure the user applied block supports are applied correctly.

  • Advance the work to generate all block styles in the server so we can make smarter decisions depending on the block's content (perhaps that involves outputing .wp-block-ID { color: value } instead of presets, etc).

👍

@glendaviesnz glendaviesnz marked this pull request as ready for review March 29, 2022 22:49
@glendaviesnz glendaviesnz added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json and removed [Status] In Progress Tracking issues with work in progress labels Mar 29, 2022
@glendaviesnz glendaviesnz changed the title Try - Apply ! important to user preset styles for theme.json themes only and provide opt-out Apply ! important to user preset styles for theme.json themes only and provide opt-out Mar 29, 2022
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

I tested in Michelle, TT1, and TwentyTwentyTwo. Adding a preset font size adds ! important to the class rule in the block and site editors and frontend.

Here's what I'm seeing:

Before

Screen Shot 2022-03-30 at 11 27 51 am

Screen Shot 2022-03-30 at 11 29 50 am

After

Screen Shot 2022-03-30 at 11 30 12 am

Screen Shot 2022-03-30 at 11 30 07 am

@glendaviesnz
Copy link
Contributor Author

I have removed the prioritiseUserPresets theme.json flag as it seemed that if we were going to aim to try and remove the application of the ! important across all themes adding this flag could just leave us with a messy backwards compatibility issue if we then want to disable the toggling of this option.

As it stands now the PR provides a quick fix to remove the issues that Classic themes are experiencing with the addition of !important on user presets, but allows them the option to turn it back on of wanted/needed by adding a theme.json file. If we manage to remove the use of !important across all themes it is then just a matter of updating this one line.

@glendaviesnz glendaviesnz changed the title Apply ! important to user preset styles for theme.json themes only and provide opt-out Apply ! important to user preset styles for theme.json themes only Mar 30, 2022
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw 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 all the efforts in refining a short term solution for this issue @glendaviesnz 🙇

I believe the current approach that avoids adding a specific theme.json flag makes sense. Given the direction we appear to be heading in removing all !important rules, I agree the new flag would likely have just added to our backwards compatibility concerns.

✅ This PR tests as advertised for me.
✅ The compute_preset_classes function migrated to 6.0 compat folder follows 5.9 but with desired tweak
🟠 @ramonjd's suggestion to tweak prioritise to prioritize is still relevant
🔴 WP_Theme_JSON_Gutenberg_Test needs updating

@cbirdsong
Copy link

Going block by block and reduce its specificity (using :where, only use single classes or less for specificity, etc) and make sure the existing preset classes work when applied to every state (with and without style variations, etc).

I think this is the best direction, though there is some concern about browser support and excessive use of :where. It would be better to figure out more well-supported solutions like this inherit pattern, which works for the post title example:

.wp-block-post-title {
  color: green;
}
.wp-block-post-title h1 {
  color: inherit;
}

.has-red-color {
  color: red;
}

@glendaviesnz
Copy link
Contributor Author

🟠 @ramonjd's suggestion to tweak prioritise to prioritize is still relevant
🔴 WP_Theme_JSON_Gutenberg_Test needs updating

@aaronrobertshaw have fixed the above

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw 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 those fixes @glendaviesnz 👍

✅ All the unit tests are now passing
✅ PR still tests as advertised
!important is included/excluded from styles appropriately

@glendaviesnz
Copy link
Contributor Author

@oandregal what are your thoughts on the approach of just disabling this by default for non theme.json themes in 6.0, so at least Classic themes go back to how they were before this change, with a view to following up as quickly as possible with an option to remove it for all themes if feasible. I removed the toggle in theme.json because of potential issues with backwards compat, etc. especially if this is only a temporary solution.

foreach ( $slugs as $slug ) {
$css_var = static::replace_slug_in_string( $preset_metadata['css_vars'], $slug );
$class_name = static::replace_slug_in_string( $class, $slug );
$prioritize_user_presets = ( WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() ) ? ' !important' : '';
Copy link
Member

@oandregal oandregal Apr 4, 2022

Choose a reason for hiding this comment

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

So far the WP_Theme_JSON class has stayed theme-agnostic and it was pure (it only used the input data passed to it to compute the output, nothing from the surrounding environment). The WP_Theme_JSON_Resolver was the one that picked up environment data. I think we should keep these concerns separated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good point

@oandregal
Copy link
Member

oandregal commented Apr 4, 2022

what are your thoughts on the approach of just disabling this by default for non theme.json themes in 6.0, so at least Classic themes go back to how they were before this change, with a view to following up as quickly as possible with an option to remove it for all themes if feasible. I removed the toggle in theme.json because of potential issues with backwards compat, etc. especially if this is only a temporary solution.

I stand by #39660 (comment) This logic was introduced for user styles to have higher priority than theme styles. Changing the behavior introduces some other issues (classic themes being able to override user styles).

You may want to engage others as well: cc @youknowriad @mtias @mcsf @jorgefilipecosta

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Apr 4, 2022

This logic was introduced for user styles to have higher priority than theme styles.

Yep, I understand and agree with the need for this. The use of ! important to enforce this has been a breaking change for theme authors though, so was just exploring some quick fixes to try and alleviate some of those issues while options for the removal of ! important completely are explored.

@glendaviesnz
Copy link
Contributor Author

I think we will struggle to get an agreement that a temporary fix like this should be included, and the time is probably better spent looking for a permanent fix that allows the removal of ! important for all themes, so going to close for now. Further updates about this ! important issue can be found here.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants