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

Typography: theme.json "layout" settings cannot be custom css variables or references, must be explicit values #55070

Open
ouw-jvt opened this issue Oct 5, 2023 · 5 comments
Labels
[Feature] Typography Font and typography-related issues and PRs [Status] Needs More Info Follow-up required in order to be actionable. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Enhancement A suggestion for improvement.

Comments

@ouw-jvt
Copy link

ouw-jvt commented Oct 5, 2023

Description

If you attempt to set the contentSize or wideSize property of settings.layout to a custom variable defined in settings.custom, the PHP portion of the style generation does not properly interpret the variable and map it to the real value. This causes some features to stop working, such as fluid typography.

In order to fix this, wp_get_typography_value_and_unit() from typography.php (or the methods calling it) would need to detect the presence of and interpolate the variable to its value by fetching it from wp_get_global_settings() (in the case of the below example's suggested fix, that would be $global_settings['custom'][layout][wideSize]).

While this may not be intended functionality, every other theme.json setting (that I have tried) seems to work when set to a variable. It's also useful for child theme inheritance and SASS workflows. Additional use cases are mentioned in #53525. However, if this will not be supported, a note in documentation demonstrating the formatting requirements of this particular setting would suffice.

Step-by-step reproduction instructions

In a block theme, using this example theme.json causes the fluid type (expected on the h1 style) to fail and revert to the size key. Key lines are "contentSize": "var(--wp--custom--layout--content-size)", and
"wideSize": "var(--wp--custom--layout--wide-size)".
CleanShot 2023-10-04 at 23 44 35@2x

{
  "$schema": "https://schemas.wp.org/trunk/theme.json",
  "version": 2,
  "settings": {
    "custom": {
      "layout": {
        "contentSize": "1692px",
        "wideSize": "1980px"
      }
    },
    "appearanceTools": true,
    "layout": {
      "contentSize": "var(--wp--custom--layout--content-size)",
      "wideSize": "var(--wp--custom--layout--wide-size)"
    },
    "typography": {
      "fluid": true,
      "fontSizes": [
        {
          "fluid": {
            "min": "1rem",
            "max": "3rem"
          },
          "name": "Large",
          "size": "3rem",
          "slug": "large"
        }
      ]
    },
    "useRootPaddingAwareAlignments": true
  },
  "styles": {
    "elements": {
      "h1": {
        "typography": {
          "fontSize": "var(--wp--preset--font-size-large)"
        }
      }
    }
  }
}

By switching back to a pixel value, things work as expected
CleanShot 2023-10-04 at 23 43 13@2x

{
  "$schema": "https://schemas.wp.org/trunk/theme.json",
  "version": 2,
  "settings": {
    "custom": {
      "layout": {
        "contentSize": "1692px",
        "wideSize": "1980px"
      }
    },
    "appearanceTools": true,
    "layout": {
      "contentSize": "1692px",
      "wideSize": "1980px"
    },
    "typography": {
      "fluid": true,
      "fontSizes": [
        {
          "fluid": {
            "min": "1rem",
            "max": "3rem"
          },
          "name": "Large",
          "size": "3rem",
          "slug": "large"
        }
      ]
    },
    "useRootPaddingAwareAlignments": true
  },
  "styles": {
    "elements": {
      "h1": {
        "typography": {
          "fontSize": "var(--wp--preset--font-size-large)"
        }
      }
    }
  }
}

Potential solutions

theme.json variable detection and lookup

In typography.php wp_get_typography_value_and_unit() after the is_numeric() check (link to source here):

  1. Check the string ($raw_value) for the text "var(--wp--)"
  2. If matched, parse the variable contents by use of explode() or regex part matching.
  3. Attempt to locate the variable's value in the wp_get_global_settings() array.

A similar method could be used to handle ref entries, as well (mentioned in #53525)

Pitfalls

Could be expensive, performance-wise, to traverse the entire theme.json checking for a match (possibly multiple times)

Documentation and/or json schema update

If we can't implement this, we should note it in the layout setting's documentation and the schema description for the field. It may be appropriate to do this anywhere in which a value is expected in a specific format literally/directly by the style engine PHP.

Environment info and related issues

  • WordPress version 6.1 to 6.3.1
  • Not browser-specific
  • Not theme-specific
  • Not device-specific

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

Related issue

Suggested tags

  • Global Styles
  • [Type] Bug
  • [Type] Enhancement
  • [Type] Developer Documentation
@ouw-jvt ouw-jvt changed the title theme.json "layout" settings cannot use custom css variables, must be explicit values theme.json: "layout" settings cannot use custom css variables, must be explicit values Oct 5, 2023
@ouw-jvt ouw-jvt changed the title theme.json: "layout" settings cannot use custom css variables, must be explicit values Typography: theme.json "layout" settings cannot be custom css variables or references, must be explicit values Oct 5, 2023
@t-hamano
Copy link
Contributor

t-hamano commented Oct 5, 2023

Hi @ouw-jvt,

This issue does not seem to reproduce in WordPress 6.4 Beta, and with the latest Gutenberg plugin.

  • ❌ WordPress 6.3.1: Reproduced
  • ✅ WordPress 6.3.1 with the latest Gutenberg plugin enabled: Not reproduced
  • ✅ WordPress 6.4 Beta 2: Not reproduced
  • ✅ WordPress 6.4 Beta 2 with the latest Gutenberg plugin enabled: Not reproduced

Is it possible to use something like the Beta Tester plugin to check if this problem occurs even with the latest WordPress core?

@t-hamano t-hamano added [Status] Needs More Info Follow-up required in order to be actionable. [Feature] Typography Font and typography-related issues and PRs labels Oct 5, 2023
@ramonjd ramonjd added the [Type] Enhancement A suggestion for improvement. label Oct 6, 2023
@ramonjd
Copy link
Member

ramonjd commented Oct 6, 2023

Thanks for creating this issue.

What I think is happening is, since Wordpress 6.3.x, specifically #53551, the typography block supports will fallback to $default_maximum_viewport_width if settings.layout.wideSize in theme.json cannot be correctly parsed to retrieve a valid value + unit, e.g., 12px or 5.4rem etc.

The fallback value is set as 1600px. Because this kicks in, we're seeing valid clamp() values despite the CSS vars.
Why do we need the actual value? Because we need numerical values to calculate the correct clamp() values.

The workaround for now could be to hardcode your values into maxViewportWidth, e.g.,

		"typography": {
			"fluid": {
				"maxViewportWidth": "1980px",
			},
                  }

This was introduced in Wordpress 6.4 in #53081 (it's been in the plugin since 16.5)

So CSS vars are not resolved. This is equally true of "ref" values, as mentioned in #53525. Exploring "ref" resolution would be my preference in the first instance to make things consistent with how other styles values work in theme.json. As you say, it would be important to measure before and after performance.

Another idea might be to introduce filters so theme developers could use their own clamp formula using incoming values. I haven't explored that idea fully yet, but all ideas are welcome 😄

If we can't implement this, we should note it in the layout setting's documentation and the schema description for the field. It may be appropriate to do this anywhere in which a value is expected in a specific format literally/directly by the style engine PHP.

This is a very good suggestion, thank you! I can update the docs with the current reality as well as pragmatic workaround.

@ramonjd
Copy link
Member

ramonjd commented Oct 6, 2023

I can update the docs with the current reality as well as pragmatic workaround.

Actually I don't have access :) It looks like the docs are getting an overhaul anyway: https://github.com/WordPress/Documentation-Issue-Tracker/issues?q=is%3Aissue+is%3Aopen+theme.json

In the meantime, the most trustworthy reference is the living theme.json schema, which should always reflect the latest changes in Gutenberg:

"description": "Sets the max-width of wide (`.alignwide`) content. Also used as the maximum viewport when calculating fluid font sizes",

@ouw-jvt
Copy link
Author

ouw-jvt commented Oct 12, 2023

@ramonjd
Thanks for the replies! Documentation and updates to the json schema description denoting it does not accept references seems sufficient for me. Good to know this has been partially addressed in newer Gutenberg packages, too.

I'll see if I can get involved in that documentation rewrite!

@github-actions
Copy link

Help us move this issue forward. This issue is being marked stale since it has no activity after 15 days of requesting more information. Please add info requested so we can help move the issue forward. Note: The triage policy is to close stale issues that need more info and no response after 2 weeks.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Status] Needs More Info Follow-up required in order to be actionable. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

3 participants