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

Use a var for user presets to avoid having to use ! important to enforce them #40335

Closed
wants to merge 12 commits into from

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Apr 14, 2022

What?

Removes the ! important that is being applied to user applied style presets. This was added here in order to fix an issue with theme style settings overriding user selected settings.

Why?

The application of the ! important across all the predefined classes has been a breaking change across a range of themes (#38252, #34575, #37590) as it overrides the valid use of some of these presets, and in some cases in a way which can't be worked around at the theme level, particularly with responsive typography.

While it is important to ensure that the styles applied by the user in the editor take priority over global styles, and theme styles, I think it is worth reconsidering the use of ! important to achieve this for the following reasons:

  • As mentioned already it is a breaking change for many Classic themes, many of which may no longer be maintained so can't be updated to account for this change, or can't be fixed due to lack of support for responsive typography in theme.json at this point
  • While it works in many cases to achieve the desired aim, it is not a guarantee that the user selected styles will be applied, eg. if a theme adds a higher specificity selector with ! important applied it will still override the user selection
  • The initial case for which it was introduced no longer needs the ! important in order to work due to the removal of a wrapper around the Post Title block. While it will still be an issue with any blocks applying these styles with a selector of specificity higher than (0,1,0) this can potentially be solve by appending the preset classes to these custom selectors

While this change may make it easier for theme styles to override, either accidentally or intentionally, user applied styles, clear documentation for theme authors on how to prevent this with the use of lower specificity selectors may be a better long term solution than trying to enforce it with ! important.

How?

  • Removes the automatic appending of !important from the preset classes
  • Adds css vars for the user presets, and uses these vars as the primary value for the presets, falling back to the other preset values, which means the preset will take priority even if a theme.json, etc. setting has a higher specificity, eg. .wp-block-columns h2 { background-color: var(--wp--user--preset--background-color,black);
  • Separates out the loading of the user presets from other presets so loading of them can be deferred so hopefully loaded after most theme styles.

N.B. This approach works as expected across the various levels of style application in the likes of TT2 theme, but it does mean that themes or plugins could override user presets if a higher specificity is used - but they can already override them by adding a higher specificity and their own ! important

Testing Instructions

  • Add a table and set the background and text color in global styles
  • Add a post with two tables, and select user defined colors on one, and make sure both display as expected in frontend
  • Via theme.json target the post title for color styles by adding:
"core/post-title": {
    "color": {
        "text": "green",
        "background": "yellow"
    }
}
  • Go to the editor, add three post title blocks:

    • Select text & background preset colors (not custom) for the first.
    • Select text & background custom colors (not preset) for the second.
    • Leave the third untouched.
  • Publish the post and go to the front end.

  • Add some theme.json element settings:

// under the styles.elements section
			"h2": {
				"color": {
					"text": "purple",
					"background": "pink"
				},
				"typography": {
					"fontSize": "var(--wp--preset--font-size--extra-large)"
				}
			}

//under the styles.blocks section
			"core/columns": {
				"elements": {
					"h2": {
						"color": {
							"text": "red",
							"background": "black"
						},
						"typography": {
							"fontSize": "var(--wp--preset--font-size--small)"
						}
					}
				}
			}
  • Add an H2 heading block out of a column and within a column block and make sure the above colors apply as expected.
  • Add additional H2 heading in and out of column block and select predefined text/background colors and font size and check that they appear as expected

The expected behavior would be that the user choices are respected, both in the editor and front-end.

@glendaviesnz glendaviesnz added [Status] In Progress Tracking issues with work in progress Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 14, 2022
@glendaviesnz glendaviesnz self-assigned this Apr 14, 2022
@github-actions
Copy link

github-actions bot commented Apr 14, 2022

Size Change: +196 B (0%)

Total Size: 1.24 MB

Filename Size Change
build/block-library/blocks/cover/style-rtl.css 1.56 kB +31 B (+2%)
build/block-library/blocks/cover/style.css 1.56 kB +30 B (+2%)
build/block-library/common-rtl.css 1.02 kB +23 B (+2%)
build/block-library/common.css 1.01 kB +22 B (+2%)
build/block-library/style-rtl.css 11.6 kB +45 B (0%)
build/block-library/style.css 11.6 kB +45 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.77 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 487 B
build/block-directory/index.min.js 6.51 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 150 kB
build/block-editor/style-rtl.css 14.5 kB
build/block-editor/style.css 14.5 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 59 B
build/block-library/blocks/avatar/style.css 59 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 445 B
build/block-library/blocks/button/editor.css 445 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 103 B
build/block-library/blocks/code/style.css 103 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 95 B
build/block-library/blocks/comments/editor.css 95 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 353 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 961 B
build/block-library/blocks/gallery/editor.css 964 B
build/block-library/blocks/gallery/style-rtl.css 1.51 kB
build/block-library/blocks/gallery/style.css 1.51 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 333 B
build/block-library/blocks/group/editor.css 333 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 738 B
build/block-library/blocks/image/editor.css 737 B
build/block-library/blocks/image/style-rtl.css 529 B
build/block-library/blocks/image/style.css 535 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 199 B
build/block-library/blocks/latest-posts/editor.css 198 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 708 B
build/block-library/blocks/navigation-link/editor.css 706 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 375 B
build/block-library/blocks/navigation/editor-rtl.css 2.03 kB
build/block-library/blocks/navigation/editor.css 2.04 kB
build/block-library/blocks/navigation/style-rtl.css 1.95 kB
build/block-library/blocks/navigation/style.css 1.94 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 395 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 260 B
build/block-library/blocks/paragraph/style.css 260 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 69 B
build/block-library/blocks/post-comments-form/editor.css 69 B
build/block-library/blocks/post-comments-form/style-rtl.css 495 B
build/block-library/blocks/post-comments-form/style.css 495 B
build/block-library/blocks/post-comments/editor-rtl.css 77 B
build/block-library/blocks/post-comments/editor.css 77 B
build/block-library/blocks/post-comments/style-rtl.css 628 B
build/block-library/blocks/post-comments/style.css 628 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 323 B
build/block-library/blocks/post-template/style.css 323 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 370 B
build/block-library/blocks/pullquote/style.css 370 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 369 B
build/block-library/blocks/query/editor.css 369 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 233 B
build/block-library/blocks/separator/style.css 233 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 759 B
build/block-library/blocks/site-logo/editor.css 759 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.37 kB
build/block-library/blocks/social-links/style.css 1.36 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 504 B
build/block-library/blocks/table/editor.css 504 B
build/block-library/blocks/table/style-rtl.css 625 B
build/block-library/blocks/table/style.css 625 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 226 B
build/block-library/blocks/tag-cloud/style.css 227 B
build/block-library/blocks/template-part/editor-rtl.css 149 B
build/block-library/blocks/template-part/editor.css 149 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 561 B
build/block-library/blocks/video/editor.css 563 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/editor-rtl.css 10.2 kB
build/block-library/editor.css 10.3 kB
build/block-library/index.min.js 181 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/theme-rtl.css 689 B
build/block-library/theme.css 694 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 47.1 kB
build/components/index.min.js 227 kB
build/components/style-rtl.css 14.5 kB
build/components/style.css 14.5 kB
build/compose/index.min.js 11.7 kB
build/core-data/index.min.js 14.6 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.39 kB
build/customize-widgets/style.css 1.39 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 7.98 kB
build/date/index.min.js 32 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.69 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 4.05 kB
build/edit-navigation/style.css 4.05 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 30.4 kB
build/edit-post/style-rtl.css 7.08 kB
build/edit-post/style.css 7.08 kB
build/edit-site/index.min.js 47.9 kB
build/edit-site/style-rtl.css 7.73 kB
build/edit-site/style.css 7.71 kB
build/edit-widgets/index.min.js 16.4 kB
build/edit-widgets/style-rtl.css 4.4 kB
build/edit-widgets/style.css 4.4 kB
build/editor/index.min.js 38.5 kB
build/editor/style-rtl.css 3.71 kB
build/editor/style.css 3.7 kB
build/element/index.min.js 4.3 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 6.62 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.83 kB
build/keycodes/index.min.js 1.41 kB
build/list-reusable-blocks/index.min.js 1.75 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.9 kB
build/notices/index.min.js 957 B
build/nux/index.min.js 2.1 kB
build/nux/style-rtl.css 744 B
build/nux/style.css 741 B
build/plugins/index.min.js 1.98 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.32 kB
build/primitives/index.min.js 949 B
build/priority-queue/index.min.js 628 B
build/react-i18n/index.min.js 704 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.69 kB
build/reusable-blocks/index.min.js 2.24 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11.2 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.52 kB
build/token-list/index.min.js 668 B
build/url/index.min.js 1.99 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.2 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.07 kB

compressed-size-action

@cbirdsong
Copy link

cbirdsong commented Apr 14, 2022

I can't figure out how to get this running locally to check it out firsthand, but based on the code change it looks like you're outputting the color both ways, like this?

.has-magenta-background-color {
  background-color: magenta;
  --wp--user--preset--background-color: magenta;
}

In my themes I only output the custom property and assign the background color on .has-background, which greatly reduces the amount of code generated:

.has-magenta-background-color {
  --wp--user--preset--background-color: magenta;
}
.has-background {
  background-color: var(--wp--user--preset--background-color);
}

Aside: This also makes it easier to assign a complementary text color using .has-background:not(.has-text-color), though I'm not sure how you'd grab the right shade with PHP (I use a Sass function.) This idea was discussed in #34717, alongside a suggestion to move to using custom properties in this way.

@glendaviesnz
Copy link
Contributor Author

based on the code change it looks like you're outputting the color both ways

This was a quick hack yesterday to see if this concept worked as it was really easy to slot this into the same spot where the var was being added. Next week I will take a closer look at whether it can be added under the .has-background selector instead as you suggest.

@glendaviesnz
Copy link
Contributor Author

One issue with this approach is that the theme css is loaded after the global css, so there is potential for theme styles to override the user applied styles. One possibility would be to load the theme styles first, but that was discussed here, and is not possible as it could cause breakages in themes that are expecting to be able to override other global styles via load order only.

Taking a look at whether we can decouple the user presets from the global styles and load them in the footer to try and make it more likely that they will take precedence of theme styles.

@glendaviesnz
Copy link
Contributor Author

Taking a look at whether we can decouple the user presets from the global styles and load them in the footer to try and make it more likely that they will take precedence of theme styles.

Have separated out the loading of the user preset css and enqueued with a lower priority, so in most cases should load after theme css - we could look at loading in footer instead.

@cbirdsong
Copy link

Loading in the footer would cause weird render jank since the styles wouldn’t be parsed until well after content was rendered, like this bug that was just filed on trac.

@glendaviesnz
Copy link
Contributor Author

Loading in the footer would cause weird render jank since the styles wouldn’t be parsed until well after content was rendered, like this bug that was just filed on trac.

yeh, I don't think it is an option at all in this case - as some of these styles are critical to the look of the page - thanks for the link to the related bug, always good to have supporting reasons for a decision.

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Apr 19, 2022

@cbirdsong you can download a copy of this PR as a plugin build from here for testing.

This approach seems to work, except in cases where a theme has a setting with a higher specificity for something, eg. if a theme had something like

.wp-block-group p {
    color: green;
}

it would need to be updated to

.wp-block-group p:not(.has-text-color) {
    color: green;
}

or

.wp-block-group p {
    color: var(--wp--user--preset--background-color, green);
}

to prevent this theme css overriding the user preset.

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.

Nice work navigating all the nuances around this issue @glendaviesnz 👍

Given the issue that this PR aims to address, I believe the tradeoff of requiring some themes to update a few styles is acceptable. So long as we continue to communicate with the wider community about the issue we're tackling and how/why we've settled upon this approach.

In general, the code changes look pretty good although I did hit one issue while testing.

Add a post with two tables, and select user defined colors on one, and make sure both display as expected in frontend

I added a gradient background to table blocks via global styles. This wasn't being overridden by user-selected flat colors in both the block editor and frontend.

  • Selecting new preset or custom gradients did work
  • Selecting custom or preset flat colors failed to take precedence
Video of issue
Screen.Recording.2022-04-20.at.7.07.18.pm.mp4

Edit: Looks like the issue I encountered is also present on trunk as well.

@cbirdsong
Copy link

This approach seems to work, except in cases where a theme has a setting with a higher specificity for something, eg. TwentyTwentyOne blocks has

.wp-block-group p {
    color: inherit;
}

I don't see that style in either version of Twenty Twenty-One in the theme directory, but it would be better to discourage targeting specific elements for color instead of inheriting from the parent container?


One other problem I see with this as it is: the cover block does not use .has-background and instead uses .has-background-dim.

<div class="wp-block-cover is-light">
  <span aria-hidden="true" class="wp-block-cover__background has-pale-cyan-blue-background-color has-background-dim-20 has-background-dim"></span>
  <div class="wp-block-cover__inner-container">
  </div>
</div>

The cover block CSS is a real mess, but if it can't be switched use .has-background then it seems like it'd be pretty easy to add --wp--user--preset--background-color into the stock styles on .has-background-dim.


  • Selecting custom or preset flat colors failed to take precedence

Edit: Looks like the issue I encountered is also present on trunk as well.

The table block seems to have a lot of issues around color inheritance. The per-block setting doesn't cascade into the figcaption in any scenario, and once I set dark/light green custom block text/background colors in the site editor it looked like this in the editor, but was unchanged in the front end:

Table block color issues

@glendaviesnz
Copy link
Contributor Author

I don't see that style in either version of Twenty Twenty-One in the theme directory

🤦 My local dev env was loading an old version of the theme that I forgot to revert! Sorry to have wasted your time looking for that.

@glendaviesnz
Copy link
Contributor Author

One other problem I see with this as it is: the cover block does not use .has-background and instead uses .has-background-dim

Nice catch! I would like to refactor that Cover block completely! ... but for the sake of an easy fix for this PR I have added a background setting to the cover block css.

@glendaviesnz
Copy link
Contributor Author

I added a gradient background to table blocks via global styles. This wasn't being overridden by user-selected flat colors in both the block editor and frontend.

Thanks @aaronrobertshaw, this was caused by the fact that background was used for gradient and not background-color. It should be fixed by this commit.

@glendaviesnz
Copy link
Contributor Author

once I set dark/light green custom block text/background colors in the site editor it looked like this in the editor, but was unchanged in the front end

This seemed to work for me, this is the frontend view:
Screen Shot 2022-04-21 at 12 22 21 PM

What theme are you using when testing this?

@glendaviesnz glendaviesnz force-pushed the try/use-css-var-for-user-presets branch from c25f18d to d06cf34 Compare April 21, 2022 00:30
@oandregal
Copy link
Member

oandregal commented Apr 21, 2022

in some cases in a way which can't be worked around at the theme level, particularly with responsive typography.

I'd like to clarify this point, which is not true with my current understanding. I'll be happy to update my understanding. I'd like us to grow a common ground of the problem to solve, otherwise is difficult to compare trade-offs and make a decision. Let me share what I know and, please, correct me if I'm missing something.

Using !important is only problematic for the subset of themes that change the font size value of the preset class using a media query. Is this correct? Are there any other use cases we aim to solve?

Can these themes set the value they want depending on media queries? Yes, they can:

@media (min-width: 600px) {
  body { --wp--preset--font-size--normal: <new_value>; }
}

That's all the CSS they need. Allowing this use case is precisely the reason we introduced CSS Custom Properties in preset classes.

Based on this, what I see is that the approach proposed in this PR forces all themes and all blocks to potentially update their CSS to make sure they don't overwrite the user presets. So, instead of asking the subset of themes that have the issue to update, this PR proposes we ask a much larger community. I'm invested in finding a fix for the subset of themes that have the issue, but I'm not convinced that asking everyone else to update is a good trade-off.

(I just wanted to share my high-level view before looking at the implementation).

@oandregal
Copy link
Member

oandregal commented Apr 21, 2022

I've looked at implementation and this is what I see.

  1. The preset classes are removed from the "global styles" stylesheet and enqueued in a new "use preset" stylesheet that we try to load after the theme CSS, to override the same classes defined by the theme.
  2. The preset classes are updated to having an intermediate generic CSS var that we set to the actual value provided via theme.json and the !important is gone:
.has-black-color {
  --wp--user--preset--color: var(--wp--preset--color--black);
  color: var(--wp--user--preset--color);
}
  1. Some blocks use the new generic CSS vars in their CSS:
.wp-block-cover [class*="-background-color"] {
	background-color: var(--wp--user--preset--background-color);
}

If the problem to solve is responsive typography for themes that enqueue their own classes, I understand how removing the !important in the preset classes and asking everyone else to update resolves the issue.

The part I don't understand of this proposal is why we need to create new generic CSS Custom Properties and promote them as public API for themes & blocks to use in their stylesheets. The direction so far has been the opposite: instead of leaking implementation details (the CSS Custom Properties) to the blocks or themes, try to absorb the block styles into the theme.json algorithm.

$presets_stylesheet = gutenberg_get_global_stylesheet( array( 'presets' ) );

wp_register_style( 'use-preset-styles', false, array(), true, true );
wp_add_inline_style( 'use-preset-styles', '.has-background { background-color: var(--wp--user--preset--background-color);} .has-text-color {color: var(--wp--user--preset--color);}' . $presets_stylesheet );
Copy link
Member

Choose a reason for hiding this comment

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

I understand why we do:

.has-text-color {
  color: var(--wp--user--preset--color);
}
.has-black-color {
  --wp--user--preset--color: var(--wp--preset--color--black);
}

Instead of:

.has-black-color {
  --wp--user--preset--color: var(--wp--preset--color--black);
  color: var(--wp--user--preset--color);
}

Though I think is a premature micro-optimization that makes everything more complex (more effort to maintain and future bugs).

To improve size performance of the global styles stylesheet, I think we'd be better off exploring some of these paths:

Performance wise, it's also worth noting this approach means 28k of CSS (global styles + use presets stylesheet) vs the 18k in trunk (global styles stylesheet).

Choose a reason for hiding this comment

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

With this, .has-text-color and .has-background could easily move back into a cacheable file and don't need to be injected as an inline style, leaving only the actual color classes in a style tag.

Also, looking at the code generated, we should not need to add the background: declaration to the .has-<name>-background-color and .has-<name>-background-gradient classes. It should just look like:

.has-background {
  background-color: var(--wp--user--preset--background-color);
  background-image: var(--wp--user--preset--background-gradient);
}

Then the individual .has- classes then just set those properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though I think is a premature micro-optimization that makes everything more complex

Yep, definitely worth reviewing the optimization versus complexity here if we move forward with this concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved the .has-background definition from inline style to static css file, and also integrated the gradient backgrounds into it which removed approx 6kb from the inline css

@@ -50,6 +50,10 @@
background-color: $black;
}

[class*="-background-color"] {
background-color: var(--wp--user--preset--background-color);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we do this if the cover block doesn't have any block supports related to this (color, background)?

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 just a side effect of the potentially premature optimisation of .has-background class, ie. Cover uses the likes of the has-white-background-color preset classes, but doesn't assign the corresponding .has-background class. Adding this style just avoids a deprecation to add that class, but better options are probably to rethink that optimisation as you note, or to refactor the cover block to use the color supports and add the associated classes.

@cbirdsong
Copy link

Based on this, what I see is that the approach proposed in this PR forces all themes and all blocks to potentially update their CSS to make sure they don't overwrite the user presets. So, instead of asking the subset of themes that have the issue to update, this PR proposes we ask a much larger community. I'm invested in finding a fix for this subset of themes, but I'm not convinced that asking everyone else to update is a good trade-off.

I don't really see how this change affects a much larger community. Removing !important would only affect themes built assuming !important is present, which would be only themes built with theme.json under 5.9.

However, adding !important in 5.9 broke responsive typography in every theme that used add_theme_support("editor-font-sizes") or even just overrode the default small/large ones with their own CSS.

Can these themes set the value they want depending on media queries? Yes, they can:

@media (min-width: 600px) {
  body { --wp--preset--font-size--normal: <new_value>; }
}

That's all the CSS they need. Allowing this use case is precisely the reason we introduced CSS Custom Properties in preset classes.

  1. This method only allows changes to font sizes globally, meaning you couldn't increase the small/medium/large font size within a hero block or decrease them in a sidebar.

  2. Authors that implemented these without theme.json might not be comfortable using custom properties due to browser support.

  3. Not every theme is has someone actively maintaining it to make this sort of update.

Using !important is only problematic for the subset of themes that change the font size value of the preset class using a media query. Is this correct? Are there any other use cases we aim to solve?

Having said all that, I think !important is far more problematic for colors than font sizes, as it makes it impossible to do super basic stuff like hover/active states.

@cbirdsong
Copy link

  1. Some blocks use the new generic CSS vars in their CSS:
.wp-block-cover [class*="-background-color"] {
	background-color: var(--wp--user--preset--background-color);
}

If the problem to solve is responsive typography for themes that enqueue their own classes, I understand how removing the !important in the preset classes and asking everyone else to update resolves the issue.

The part I don't understand of this proposal is why we need to create new generic CSS Custom Properties and promote them as public API for themes & blocks to use in their stylesheets. The direction so far has been the opposite: instead of leaking implementation details (the CSS Custom Properties) to the bolcks, try to absorb the block styles into the theme.json algorithm.

I don't think you can anticipate every possible situation where a color might need to be applied by a theme/block style, and using custom properties like this allows for a theme/block dev to take user preferences into account while also leaving flexibility for how CSS can be written compared to forcing it on with !important.

These custom properties would only need to be used in situations where a block needs to use the custom color in an unexpected way. In the case of the cover block, this is only required because it doesn't use the standard .has-background class when setting a color, but the original impetus was the table block's "striped" style, which we looked at here and there is no easy fix for. Custom properties provide an elegant solution.

@oandregal
Copy link
Member

oandregal commented Apr 21, 2022

These custom properties would only need to be used in situations where a block needs to use the custom color in an unexpected way. In the case of the cover block, this is only required because it doesn't use the standard .has-background class when setting a color, but the original impetus was the table block's "striped" style, which we looked at #40121 (comment) and there is no easy fix for. Custom properties provide an elegant solution.

I find this concerning for a few reasons:

We're trying to circumvent the existing issues of the block supports mechanism with a limited fix instead of addressing them. One example: how the global styles sidebar can expose an UI for the user to change the cover block color with this approach?

We actually used something like this in the first iterations. See the PR that introduced them in the button block and the PR were it was reverted. I've documented the struggles we ran into in this issue: essentially, 1) nested content was problematic (inner blocks, patterns) and 2) undefined css vars made css properties that don't inherit (background-color) to inherit.

@glendaviesnz
Copy link
Contributor Author

Using !important is only problematic for the subset of themes that change the font size value of the preset class using a media query. Is this correct? Are there any other use cases we aim to solve?

Can these themes set the value they want depending on media queries? Yes, they can:

@media (min-width: 600px) {
body { --wp--preset--font-size--normal: <new_value>; }
}
That's all the CSS they need.

@webmandesign can you please confirm if the above approach, noted here, would resolve your issue with the responsive typography caused by the addition the !important.

Having said all that, I think !important is far more problematic for colors than font sizes, as it makes it impossible to do super basic stuff like hover/active states

@cbirdsong are you able to provide a concrete example of where the use of !important is being a hindrance in this regard, please? I could contrive one, but better to see actual cases you are dealing with if possible so we are all on the same page.

@cbirdsong
Copy link

We actually used something like this in the first iterations. See the PR that introduced them in the button block and the PR were it was reverted. I've documented the struggles we ran into in this issue: essentially, 1) nested content was problematic (inner blocks, patterns) and 2) undefined css vars made css properties that don't inherit (background-color) to inherit.

Reading through those, I can see (and have personally experienced) how custom properties applied that way can cascade unexpectedly, but limiting the application of the --wp--user--preset properties to .has-background and .has-text-color eliminates the inheritance problems1, since those classes should always appear alongside another class that defines the associated custom property.

There will certainly be cases where a CSS author will have to apply a .has-<whatever> or :not(.has-<whatever>) to avoid accidentally overriding something, but before !important was added in 5.9 that often needed to be considered anyway.

With !important you just have to make different considerations on how to work around it, only with far fewer options. You can, of course, just add more !important), and if you do that sloppily then the user’s choices are no longer being respected and we’re back where we started.

!important or not, there will always have to be a convention that theme/block authors need to work within while writing CSS, and consequently there will always be a chance of that getting screwed it up.

Footnotes

  1. Except possibly with gradients, depending on how they're applied.

@cbirdsong
Copy link

cbirdsong commented Apr 22, 2022

@webmandesign can you please confirm if the above approach, noted here, would resolve your issue with the responsive typography caused by the addition the !important.

This may be a valid way to accomplish responsive typography within the constraints of !important, but it is an inflexible path that cuts off many options for theme authors, and it will not fix any sites with non-theme.json themes that used other methods.


@cbirdsong are you able to provide a concrete example of where the use of !important is being a hindrance in this regard, please? I could contrive one, but better to see actual cases you are dealing with if possible so we are all on the same page.

The most common scenario is buttons, which feel broken without hover/active states. It is difficult to meet color contrast standards when adding them using opacity/filter, so I use a Sass function to automatically choose complementary active shades for each color. Often it's just darkening/lightening the color by a percentage, but on others I've had them hand-curated by the designer or automatically calculated them from a list of potential shades in the site's palette. Simplified example:

@each $name, $color in $colors {
	$color-active: color-shift($color, 33%); // darkens or lightens the color by 33%, depending on if it's a light or dark color

	.has-#{$name}-background-color {
		--button-color: #{$color};
		--button-color-active: #{$color-active}; 
		--button-color-inverse: #{color-contrast($color)}; // chooses white or black text, depending on if it's a light or dark color
		--button-color-inverse-active: #{color-contrast($color-active)}; 
	}
	.has-#{$name}-color { 
		--button-color-inverse: #{$color}; 
		--button-color-inverse-active: #{$color-active}; 
		// these come after the ones in the background color class so setting a custom text color is still possible
	}
}

On occasion I've also created lightened/transparent versions of colors for the background of the outline button style.

@each $name, $color in $colors {
	.has-#{$name}-background-color.is-style-outline {
		--button-color: #{transparentize($color, 0.3)}; // the color at 30% opacity
		--button-color-active: #{transparentize($color, 0.15)}; // the color at 15% opacity
		--button-color-inverse: #{$black}; // the text color is always black, in this case
		--button-color-inverse-active: #{$color}; // then switching to the full opacity color on hover
	}
}

These can both be applied using the same CSS1:

.wp-block-button > .wp-block-button__link {
	color: var(--button-color-inverse);
	background-color: var(--button-color);

	&:hover, &:focus, &:active {
		color: var(--button-color-inverse-active, var(--button-color-inverse));
		background-color: var(--button-color-active);
	}
}

Outside of buttons there are simply scenarios where it might make more sense to apply a text/background color elsewhere within the block. One recent theme I worked on had a block style for the Media & Text block where the content element overlapped on the image, like so:

Media + Text block with content overlapping the image

Because my .has-<color> classes only provided a custom property, I was able to easily apply it to the overlapping content element instead of the wrapper:

.wp-block-media-text.is-style-overlapping {
    .wp-block-media-text__content {
        background-color: white;
        transform: translateX(-8%);
        border-radius: 1.5rem / 0.25rem;
        box-shadow: 0 0 0 0.2em black;
    }
}

.wp-block-media-text.is-style-overlapping.has-background {
    background-color: transparent;
    
    .wp-block-media-text__content {
        background-color: var(--wp-background-color);
    }
}

Again, I could do all of this on a stock 5.9 install by just using !important to override the generated background-color: whatever !important; styles as needed. The editor does not have a monopoly on the use of !important in Wordpress, so it will always be possible for me to screw this up and prevent a user's color choice from working as expected. However, if that happens, I will want to fix it, because if I didn't want users to be able to select colors then I wouldn't have registered any.

Footnotes

  1. My actual Sass is more complicated than this to avoid generating too much CSS once compiled, so this is simplified for the sake of brevity/comprehensibility.

@glendaviesnz
Copy link
Contributor Author

@cbirdsong thanks for all your input - your comments and examples are really helpful in building up the full picture of the issues at play here.

@glendaviesnz
Copy link
Contributor Author

We actually used something like this in the first iterations. See the PR that introduced them in the button block and the PR were it was reverted. I've documented the struggles we ran into in this issue: essentially, 1) nested content was problematic (inner blocks, patterns) and 2) undefined css vars made css properties that don't inherit (background-color) to inherit.

Thanks for the extra context @oandregal, will take a look.

@oandregal
Copy link
Member

oandregal commented Apr 22, 2022

re: hover/active statuses with colors

Thanks for sharing. Let's say the theme wanted yellow to be the hover color:

.wp-block-button:hover {
  --wp--preset--color--black: yellow;
  --wp--preset--color--cyan-bluish-gray: yellow;
  --wp--preset--color--white: yellow;
  --wp--preset--color--pale-pink: yellow;
  --wp--preset--color--vivid-red: yellow;
  --wp--preset--color--luminous-vivid-orange: yellow;
  --wp--preset--color--luminous-vivid-amber: yellow;
  --wp--preset--color--light-green-cyan: yellow;
  --wp--preset--color--vivid-green-cyan: yellow;
  --wp--preset--color--pale-cyan-blue: yellow;
  --wp--preset--color--vivid-cyan-blue: yellow;
  --wp--preset--color--vivid-purple: yellow;
  /* add another custom property for every color defined by the theme as well */
}

It can be done and it is ugly.

I understand why indirection through a generic custom property in the preset classes would make this particular use case easier to achieve. Though, it is not necessary if we remove the !important from the preset classes as well, and it doesn't solve the issue with hover/focus/active status in our current systems.

I feel that we're trying too many things at once in this PR: either we propose a solution that addresses the hover/focus/active statuses and/or responsive/intrinsic web design across all layers (user block styles, user global styles, theme global styles) or we focus on a proposal that helps themes for which !important has proven problematic.

How can me make progress in the second problem?

  • This PR and 40121 are the most impactul. They both have some technically interesting ideas that can be useful later (browser support is larger for :where) or to solve a different problem.

  • 39660 (remove !important from themes with no theme.json) respects the existing code for themes without theme.json that use hover statuses and/or different font size for viewports. It does so at the cost of making user presets potentially not work with the new things we keep adding (new blocks, patterns, new nested contexts such as gallery, quote, list, etc) for every theme with no theme.json.

Unless there's any other thing we can try, in my view, we're stuck at either asking these subset of themes to update their hover/viewport code (current approach) or asking every theme with no theme.json to update their preset classes to work with the new things as we keep adding them (the proposal at 39660). I find the current approach less impactful, given the importance of what we continue to ship, the number of themes impacted is smaller, and the use cases are constrained (color for hover status + font sizes for viewports). I understand how some people can prefer the opposite.

(Sorry for the long comment, I wanted to braindump all I could before I start a long off-work period where I won't be able to participate in these conversations. I hope it has been useful and, please, know that I appreciate a lot the drive to keep looking for solutions to our current struggles.)

@cbirdsong
Copy link

cbirdsong commented Apr 22, 2022

I feel that we're trying too many things at once in this PR: either we propose a solution that addresses the hover/focus/active statuses and/or responsive/intrinsic web design across all layers (user block styles, user global styles, theme global styles) or we focus on a proposal that helps themes for which !important has proven problematic.

To be clear, I'm not trying to solve hover/active styles or any other larger thing with this, just offering examples where !important causes problems that can only be solved with more !important.

Unless there's any other thing we can try, in my view, we're stuck at either asking these subset of themes to update their hover/viewport code (current approach) or asking every theme with no theme.json to update their preset classes to work with the new things as we keep adding them (the proposal at #39660). I find the current approach less impactful, given the importance of what we continue to ship, the number of themes impacted is smaller, and the use cases are constrained (color for hover status + font sizes for viewports). I understand how some people can prefer the opposite.

I don't understand why the universal removal of !important is not one of the options here. It was only needed as a hack to get around specificity problems, and it does not actually guarantee user choices are respected simply because of how CSS works. The correct choice is removing !important, fixing the specificity problems in the core CSS that necessitate it, and creating guidelines and best practices that everyone writing CSS around the editor can use moving forward.

@glendaviesnz
Copy link
Contributor Author

Sorry about the delay in further comment on this, I got tied up with some other things, will hopefully get back to it tomorrow.

@scruffian
Copy link
Contributor

I wonder if this PR will help to solve this in a different way. By specifying the block CSS in block.json, we can ensure that the correct order of precedence for settings is always adhered to (user > theme > core) without needing to use CSS tricks like strong selectors or !important.

@scruffian scruffian deleted the try/use-css-var-for-user-presets branch November 9, 2022 18:30
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