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

WIP: Check for Theme Support before adding style attributes in editor #28422

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Jan 22, 2021

Description

When the Block Editor handles blocks that support color, if the color
applied is a palette color the value of that color is still applied by
way of a style attribute. This is done to provide backward
compatability for themes that don't include the palette CSS in their
editor stylesheet. However, for themes that DO provide those classes,
and especially for those where the behavior isn't just "change the
color" (i.e. Spearhead 'dark' mode) this logic breaks expected behavior.

This is an attempt to allow a theme to express, by way of a theme
supports flag, that those style attributes should NOT be applied to the
block; the classes will suffice.

Fixes: #26506

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

When the Block Editor handles blocks that support color, if the color
applied is a palette color the value of that color is still applied by
way of a style attribute.  This is done to provide backward
compatability for themes that don't include the palette CSS in their
editor stylesheet.  However, for themes that DO provide those classes,
and especially for those where the behavior isn't just "change the
color" (i.e. Spearhead 'dark' mode) this logic breaks expected behavior.

This is an attempt to allow a theme to express, by way of a theme
supports flag, that those style attributes should NOT be applied to the
block; the classes will suffice.
@pbking pbking requested a review from ellatrix as a code owner January 22, 2021 14:36
@pbking pbking marked this pull request as draft January 22, 2021 14:36
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 22, 2021
@pbking
Copy link
Contributor Author

pbking commented Jan 22, 2021

I expected this to be relatively simple but I got really stuck trying to get an updated value of the theme supports configuration.

The value of themeSupports appears to be cached in a way that I can't bust. It is pre-loaded in a <script id='wp-api-fetch-js-after'> and the value doesn't reflect what is CURRENTLY set by a theme (by adjusting values set by add_theme_support in functions.php).

I have not been able to find the code that queues this 'wp-api-fetch-js-after' inline script to determine what's happening. Instead it appears to be some historical value. Even attempting this on a completely fresh WordPress installation didn't provide the values expected.

Also attempted was to bump the version in the theme I am testing with, but to no avail. The changed VERSION (via style.css) of the theme IS correctly expressed in that preloaded data...but the value of themeSupports is not.

@youknowriad
Copy link
Contributor

This is a very complex thing. I think personally we shouldn't be introducing a new theme support or config but we should instead do this:

  • Automatically generate the styles from the define palettes and inject it to the editor
  • Remove that fallback.

I actually thought the first step here was already implemented by @jorgefilipecosta at some point for Global styles. Maybe @nosolosw could confirm or deny that?

@pbking
Copy link
Contributor Author

pbking commented Jan 22, 2021

Hey @youknowriad if I understand correctly, that actually IS what is happening (by way of #22356). That is what I am attempting to UNdo (in a targeted way). In this instance, Spearhead has a dark mode, and when that is the case the palette color "Black" actually is a white color, which is correctly handled and expressed in the class applied to the block, but that value (in that scenario) is different than the "light" version that is expressed in the hex value in the color palette.

As was discussed in 22356 that it would be lovely if that fix (applying styles to the blocks) weren't necessary, but for regression reasons appears to be so. I tried to use some other logic to determine if those styles were actually necessary, but if there is a way to determine that then I don't know.

And you're absolutely right. It is a very complex thing. I fell down this rabbit hole for hours already. :P

@youknowriad
Copy link
Contributor

In this instance, Spearhead has a dark mode, and when that is the case the palette color "Black" actually is a white color,

How is this spearhead doing this? By using a global classname or something? How do you think such behavior should be handled with global styles in mind (no css basically)?

This seems to fall into the use-case of "nested context" maybe? this is: potentially being able to define styles for a specific context which in this situation would be a .wp-style-is-dark-mode container. And done this way, I assume global styles would generate the right CSS for it. We don't support nested contexts like that yet though. So I wonder if we should introduce a stopgap for now or not. (and maybe leaning towards the latter)

@pbking
Copy link
Contributor Author

pbking commented Jan 22, 2021

I would assume that Global Styles would always appropriately supply the classes to drive this scenario, so I would expect that if styles were being driven by theme.json then this flag should be defaulted to TRUE making the behavior of adding the colors to the style attributes a thing that only happens for "classic" themes (and then, only classic themes without the flag). At least that's the fix I was working towards. Any ideas that stop forcing the color into the style attribute would be a dandy fix.

Spearhead handles this pretty cleanly with a media query. If a user has a system dark mode set and a user uses a palette color then the class for that palette color is just re-defined as the inverse(ish). Something only do-able in CSS due to the media-query but still very do-able in combination with theme.json styling techniques.

Base automatically changed from master to trunk March 1, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
2 participants