WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.3k stars 4.11k forks source link

Typography: numerical font size presets break FontSizePicker #44857

Open ramonjd opened 1 year ago

ramonjd commented 1 year ago

Description

Setting the size property of a font size preset item to a number causes the FontSizePicker component to stop working for newly-inserted blocks.

{
    "version": 2,
    "settings": {
        "appearanceTools": true,
        "typography": {
            "fontSizes": [
                {
                    "size": 12,
                    "slug": "small"
                }
            ],
                         ...
        }
    }
}

The schema for theme.json defines "size" as being of type 'string', however the FontSizePicker does support numbers.

So question is whether we should support numbers as font sizes across the board (and coerce them to px values?).

From @noisysocks

probably what’s happening is that FontSizePicker supports it fine but then the onChange callback doesn’t maybe we need to do a more wholistic audit of setting size to be a number

Step-by-step reproduction instructions

  1. Add a fontSize preset with a number value for "size". Make sure you have more than 3 but no more than 5 presets.
  2. Open the post editor and add a text block, e.g., paragraph or heading
  3. Attempt to apply a font size preset to the newly-inserted block
  4. Observe that the preset is not applied.

Screenshots, screen recording, code snippet

2022-10-11 11 44 42

Environment info

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

noisysocks commented 1 year ago

The schema for theme.json defines "size" as being of type 'string', however the FontSizePicker does support numbers.

How about let's lean into what the schema says and require strings? If a number is specified then automatically switch it to a ${ number }px string at the theme.json level and output a warning.

ramonjd commented 1 year ago

I spent some time trying to resolve this and I think the most appropriate place to coerce a font size, where it is a number, is in gutenberg_get_typography_font_size_value, something like this:


    /*
     * JSON schema requires the value of "size" to be a string.
     */
    if ( ! is_string( $preset_size ) ) {
        _doing_it_wrong(
            __FUNCTION__,
            __( 'Font-size value must be a string, including units.', 'gutenberg' ),
            '6.3.0'
        );
    }

    // Tries to cast numeric values to a string and add a 'px' unit as fallback.
    if ( is_numeric( $preset_size ) ) {
        $preset_size = $preset_size . 'px';
    }

We're already coercing to "string" + px for fluid font sizes, so it makes sense to do it this way I believe.

Not too sold on the _doing_it_wrong error since it breaks all the tests! 😄 Anyone know a way around it?