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

Global styles don't support multi-value shorthand padding #40132

Open
tellthemachines opened this issue Apr 7, 2022 · 1 comment
Open

Global styles don't support multi-value shorthand padding #40132

tellthemachines opened this issue Apr 7, 2022 · 1 comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@tellthemachines
Copy link
Contributor

Description

If a theme sets root padding in theme.json as a multi-value shorthand declaration, like so:

"styles": {
		"spacing": {
                        "padding": "68px 32px"
                }
	}

it gets added to the body as expected: padding: 68px 32px;.

But if a user then sets root padding in global styles, and if they don't provide a value for all sides (say they only want to change the left/right padding, and keep the theme's top/bottom value), the shorthand is interpreted as a single value and results in incorrect CSS:

padding-top: 68px 32px;
padding-right: 20px;
padding-bottom: 68px 32px;
padding-left: 20px;

Step-by-step reproduction instructions

  1. Activate Empty Theme, and replace the content of its theme.json with:
{
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"spacing": {
                        "padding": "68px 32px"
                }
	},
	"patterns": [ "short-text-surrounded-by-round-images", "partner-logos" ]
}
  1. In the Global Styles sidebar, under "Layout", change "Padding" by first clicking "Unlink sides" and then editing only one or two of the fields.
  2. Save and verify that incorrect CSS now appears attached to .editor-styles-wrapper in the editor, and body in the front end.

Screenshots, screen recording, code snippet

No response

Environment info

No response

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

@tellthemachines tellthemachines added [Type] Bug An existing feature does not function as intended [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 7, 2022
@andrewserong
Copy link
Contributor

Just copying over a code snippet from the discussion in #39926 (comment) — if we decide we'd like to parse out shorthand properties, one way to do it could be to use something like the following to grab each part of the CSS value that's separated by a space, but isn't contained within a set of brackets:

const inputString = 'var( --my-variable, calc(2px + 3px) ) 2px 3px 0px';

function parseShorthandProps( input ) {
  const values = [];
  
  let openBracket = 0;
  let closedBracket = 0;
  let startIndex = 0;
  let isCollecting = false;
  
  for ( let i = 0; i < input.length; i++ ) {
    if ( ! isCollecting && input[i] !== ' ' ) {
      isCollecting = true;
      startIndex = i;
    }
    
    if ( input[i] === ('(') ) {
      openBracket++;
    }
    
    if ( input[i] === (')') ) {
      closedBracket++;
    }

    if ( isCollecting && ( input[i] === ' ' && openBracket === closedBracket ) || i === input.length - 1 ) {
      isCollecting = false;
      const val = i === input.length -1 ? input.substring( startIndex ) : input.substring( startIndex, i );
      values.push( val );
      openBracket = 0;
      closedBracket = 0;
    }
  }
  
  return values;
}

console.log( parseShorthandProps( inputString ) );
// output: ["var( --my-variable, calc(2px + 3px) )","2px","3px","0px"]

Explicitly dealing with the fact that CSS values can contain CSS functions does open up a potential can of worms that @ramonjd mentioned, which is that we might be able to output values that include clamp(), var(), calc(), etc, but we don't (yet) have a means for a user to edit those kinds of values within the site editor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants