WordPress / gutenberg

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

Units block support in block.json does not work #47519

Open carolinan opened 1 year ago

carolinan commented 1 year ago

Description

Three core blocks currently has a block support in block.json called units: gallery, navigation, and social links. The purpose of this block support is undocumented, and it is not in the schema for block.json.

Assuming that the intention was to set the default units that the block uses in different controls, the support is not working. It is possible that this was working when it was implemented, but has regressed.

For example the navigation block declares:

"spacing": {
            "blockGap": true,
            "units": [ "px", "em", "rem", "vh", "vw" ],
            "__experimentalDefaultControls": {
                "blockGap": true
            }
        },

But the block spacing control has the units px, em and rem.

In the gallery block, units are directly under supports, not under spacing.

    "supports": {
        ...
        "units": [ "px", "em", "rem", "vh", "vw" ],

But only the spacing control uses units, and they are px, em and rem.

If I update these blocks to for example only use the px unit, there is no change in the editor controls. If spacing units are also added in theme.json, then the theme.json values are used.

Step-by-step reproduction instructions

  1. Activate a theme that does not add spacing units, for example emptytheme.
  2. Add a gallery block.
  3. In the Dimensions panel, block spacing, select "Set custom size".
  4. In the control, select the button with the text "px". This opens the unit options. Confirm that only px, rem and em are available.

Screenshots, screen recording, code snippet

No response

Environment info

WP 6.2-alpha-55147 Gutenberg trunk

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

skorasaurus commented 8 months ago

I'm unable to reproduce this in Gutenberg 17 and 6.4; I can use vh and vw in the gallery block as indicated below.

However, I wonder if we should leave this open because I do notice that the gallery block's support has supports.units

but the navigation block has supports.spacing.units

Is that intentional?

my theme.json 
      "spacing":{
         "padding":true,
         "margin":true,
         "units":[
            "px",
            "em",
            "rem",
            "vh",
            "vw",
            "ch"
         ]
      }

For example:

image

image

carolinan commented 8 months ago

theme.json values work, but block.json does not.

t-hamano commented 8 months ago

I've tested it in all major versions since WP6.0, when the gallery block first supported gap. Overriding units in block.json does not originally work, and it does not appear to be a bug or regression. Therefore, I will label this issue as Enhancement.

carolinan commented 7 months ago

Perhaps @glendaviesnz or @ramonjd knows the purpose of these block supports or if they can even be removed from the block.json in the respective blocks.

ramonjd commented 7 months ago

Thanks for the ping @carolinan, and for testing @t-hamano

I can confirm @carolinan's observation: setting spacing.units in block.json has no effect. Only in theme.json can I set units that affect UI controls:

        "spacing": {
            "units": true // default is [ "px", "em", "rem", "vh", "vw", "%" ]
        }

Border units are also taken from spacing and therefore controlled by theme.json.

And the unit control component has the same default (uses spacing units): https://github.com/WordPress/gutenberg/blob/84715c01148344c318344978508b5d442526ed0c/packages/block-editor/src/components/unit-control/index.js#L17-L17

I'd lean on @glendaviesnz's knowledge in relation to spacing units.

As for others, typography font size units are hard-coded it appears: https://github.com/WordPress/gutenberg/blob/84715c01148344c318344978508b5d442526ed0c/packages/components/src/font-size-picker/index.tsx#L69-L69

To remain consistent with other settings, should block.json units override theme.json settings?

Looking at the code, I suppose it would be possible to grab the block.json overrides where it makes sense:

        // For units that use spacing.units
    const [ availableUnits ] = useSettings( 'spacing.units' ); // theme.json
        const blockUnits = getBlockSupport( blockName, 'spacing.units' ); // block.json
    const units = useCustomUnits( {
        availableUnits: availableUnits || blockUnits || [ 'px', 'em', 'rem' ],
    } );

Or maybe the better place would be useSettingsForBlockElement, which states to be a "React hook that overrides a global settings object with block and element specific settings."

We'd probably want to audit the places we use custom units to ensure values make sense, e.g., would vh and vw be appropriate for font sizes? We could just leave it up to block consumers to decide as well.

carolinan commented 7 months ago

Shouldn't block.json provide defaults, and theme.json override them?

ramonjd commented 7 months ago

To remain consistent with other settings, should block.json units override theme.json settings?

Sorry, that came out completely wrong. 😄 🙃

I meant block.json supports settings could be the place where a block defines whether it supports something, e.g., units. And you're right, theme.json overrides values of supported settings.

t-hamano commented 7 months ago

I'm a bit confused, is the unit property something that should be defined in block.json ? In other words, defining units is entirely the responsibility of theme.json, and core blocks might not need to provide default units via block.json ?

For example, if we want to change only the unit of a specific block, we can define it in theme.json as follows. I have confirmed that this definition actually works as expected.

{
    "$schema": "https://schemas.wp.org/trunk/theme.json",
    "version": 2,
    "settings": {
        "appearanceTools": true,
        "spacing": {
            "units": [ "px", "%" ]
        },
        "blocks": {
            "core/group": {
                "spacing": {
                    "units": [ "vh", "vw" ]
                }
            }
        }
    }
}

If this assumption is correct, is the unit defined in block.json some kind of mistake and should we just delete it?

ramonjd commented 7 months ago

I'm a bit confused, is the unit property something that should be defined in block.json ? In other words, defining units is entirely the responsibility of theme.json, and core blocks might not need to provide default units via block.json ?

It's a good question.

I think a blocks' block.json could still define defaults if it makes sense in context. Maybe the % or vh might not make good defaults for some block types, e.g., text blocks. Now, all this assumes that the functionality to make this work will be built 😄

Another example, I suspect the gallery block defined "units": [ "px", "em", "rem", "vh", "vw" ], (even though it has no effect) to provide specific defaults for block gap in https://github.com/WordPress/gutenberg/pull/38164

@aaronrobertshaw will also have some insight into supports.

aaronrobertshaw commented 7 months ago

I think a blocks' block.json could still define defaults if it makes sense in context. Maybe the % or vh might not make good defaults for some block types, e.g., text blocks.

My take on this is the block.json might define more the "available" units rather than defaults. So as you say, the block could cull units that do not make sense for it at all. Theme.json can then still define which units are actually used or shown.

https://github.com/WordPress/gutenberg/pull/31822 might provide some further context around units.

Another example, I suspect the gallery block defined "units": [ "px", "em", "rem", "vh", "vw" ], (even though it has no effect) to provide specific defaults for block gap in https://github.com/WordPress/gutenberg/pull/38164

At a quick glance of that PR and some of the other gallery code at the time of that commit, I couldn't see where the units defined in block.json were actually used. @glendaviesnz might have some extra details buried deep within his memory 🤞

carolinan commented 2 months ago

This is also relevant if we want to add units that are specific to font sizes. @t-hamano