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.

Sign up for GitHub

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

Jump to bottom

Allow full width blocks to override parent or root container padding #39871

Closed
wants to merge 9 commits into from
Closed

Allow full width blocks to override parent or root container padding #39871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f93230f
Cherry pick relevant commits from #36214
Mar 30, 2022
e3a71f2
Fix attribute location
Mar 30, 2022
cc90ade
Revert cover style change
tellthemachines Apr 8, 2022
b7536bb
Only add negative margins is padding set to prevent invalid property …
Apr 10, 2022
cd5b314
Fix attributes path
tellthemachines Apr 11, 2022
505f87f
Fix double declaration of left menu
Apr 11, 2022
92f57fb
Fix front end styles
tellthemachines Apr 11, 2022
5b706aa
Try fullwidth when block padding is set
tellthemachines Apr 11, 2022
fe0801b
Check layout is actually used
tellthemachines Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ function gutenberg_register_layout_support( $block_type ) {
*
* @param string $selector CSS selector.
* @param array $layout Layout object. The one that is passed has already checked the existence of default block layout.
* @param array|null $padding Padding applied to the current block.
* @param boolean $has_block_gap_support Whether the theme has support for the block gap.
* @param string $gap_value The block gap value to apply.
* @param boolean $should_skip_gap_serialization Whether to skip applying the user-defined value set in the editor.
* @param string $fallback_gap_value The block gap value to apply.
*
* @return string CSS style.
*/
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false, $fallback_gap_value = '0.5em' ) {
function gutenberg_get_layout_style( $selector, $layout, $padding, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false, $fallback_gap_value = '0.5em' ) {
$layout_type = isset( $layout['type'] ) ? $layout['type'] : 'default';

$style = '';
Expand All @@ -54,14 +55,29 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$wide_max_width_value = wp_strip_all_tags( explode( ';', $wide_max_width_value )[0] );

if ( $content_size || $wide_size ) {
$style = "$selector > :where(:not(.alignleft):not(.alignright)) {";
$style = "$selector {";
// Using important here to override the inline padding that could be potentially
// applied using the custom padding control before the layout inheritance is applied.
$style .= sprintf(
'padding: %s %s %s %s !important',
isset( $padding['top'] ) ? $padding['top'] : 0,
isset( $padding['right'] ) ? $padding['right'] : 0,
isset( $padding['bottom'] ) ? $padding['bottom'] : 0,
isset( $padding['left'] ) ? $padding['left'] : 0
Copy link
Member

@ramonjd ramonjd Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I will test this manually, but ran out of time today 😄

So this is a note for myself more than anything.

Let's say the result is padding: 10px 0 0 0;, where only padding-top: 10px was defined by the user in the editor.

Are there any problems with right, bottom and left being now 0?

What if a CSS style higher up says otherwise, and is consequently overridden?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point!

What if a CSS style higher up says otherwise, and is consequently overridden?

It sounds like that might be a similar challenge to what we've been discussing in the PR to add support for blockGap at the block level within global styles: #39870 (comment) — TL;DR: how do we reliably get the value that's cascaded down correctly at this point in the execution. E.g. do we need to look up global styles here as well, to get the right value?

Copy link
Contributor

@andrewserong andrewserong Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, wait, I think I might be getting confused here — because this is a different padding that comes from the layout object instead? No: it's coming from the style object... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. do we need to look up global styles here as well, to get the right value?

I think we might, yes. Though ideally, either the block attributes would know about global styles too, or that cascade would be resolved someplace else by the time we get to layout. Is that within the scope of the style engine work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that within the scope of the style engine work?

I'm not too sure, to be honest, it's at least beyond the initial work of refactoring the existing block supports, but I think we might naturally bump into it when we go to consolidate style generation for global styles / theme.json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rounding back to this: global/theme.json block styles are overwritten for the reasons @andrewserong mentions above

Screen Shot 2022-04-12 at 3 18 09 pm

Is that within the scope of the style engine work?

I see the intention is for !important to override inline styles. Works as intended, but the inline styles are still there.

Without promising anything 😄 I also wonder if the style engine will later dedupe this sort of stuff as well. Layout crosses over into spacing more and more it seems!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why we need to render the padding here (I mean in layout)? Can't we assume the padding block support will just output the same thing (inline or using a style tag) later?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's for the "inherited padding", maybe we can only output these styles if the layout is inherited to avoid duplication and avoid important?

);
$style .= '}';
$style .= "$selector > :where(:not(.alignleft):not(.alignright)) {";
$style .= 'max-width: ' . esc_html( $all_max_width_value ) . ';';
$style .= 'margin-left: auto !important;';
$style .= 'margin-right: auto !important;';
$style .= '}';

$style .= "$selector > .alignwide { max-width: " . esc_html( $wide_max_width_value ) . ';}';
$style .= "$selector .alignfull { max-width: none; }";
$style .= "$selector > .alignfull {";
$style .= 'max-width: none;';
$style .= isset( $padding['left'] ) ? sprintf( 'margin-left: calc( -1 * %s ) !important;', $padding['left'] ) : '';
$style .= isset( $padding['right'] ) ? sprintf( 'margin-right: calc( -1 * %s ) !important;', $padding['right'] ) : '';
$style .= '}';
}

$style .= "$selector > .alignleft { float: left; margin-inline-start: 0; margin-inline-end: 2em; }";
Expand Down Expand Up @@ -162,6 +178,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {

$block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) );
$default_layout = gutenberg_get_global_settings( array( 'layout' ) );
$padding = _wp_array_get( $block, array( 'attrs', 'style', 'spacing', 'padding' ), null );
$has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false;
$default_block_layout = _wp_array_get( $block_type->supports, array( '__experimentalLayout', 'default' ), array() );
$used_layout = isset( $block['attrs']['layout'] ) ? $block['attrs']['layout'] : $default_block_layout;
Expand All @@ -170,6 +187,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
return $block_content;
}
$used_layout = $default_layout;
$padding = isset( $default_layout['padding'] ) ? $default_layout['padding'] : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds padding to all default layout blocks, which is not the current behaviour in trunk, so it might break some stuff.

}

$class_names = array();
Expand Down Expand Up @@ -209,7 +227,7 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
$should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' );
$style = gutenberg_get_layout_style( ".$container_class", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
$style = gutenberg_get_layout_style( ".$container_class", $used_layout, $padding, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
// This assumes the hook only applies to blocks with a single wrapper.
// I think this is a reasonable limitation for that particular hook.
$content = preg_replace(
Expand Down
1 change: 1 addition & 0 deletions lib/compat/wordpress-6.0/class-wp-theme-json-6-0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ class WP_Theme_JSON_6_0 extends WP_Theme_JSON_5_9 {
'layout' => array(
'contentSize' => null,
'wideSize' => null,
'padding' => null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we're adding this when we can use the value at styles > spacing > padding.

),
'spacing' => array(
'blockGap' => null,
Expand Down
53 changes: 32 additions & 21 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { has, kebabCase } from 'lodash';
import { has, get, kebabCase } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -77,23 +77,14 @@ function getLayoutClasses( attributes ) {

function LayoutPanel( { setAttributes, attributes, name: blockName } ) {
const { layout } = attributes;
const defaultThemeLayout = useSetting( 'layout' );
const themeSupportsLayout = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings().supportsLayout;
}, [] );

const layoutBlockSupport = getBlockSupport(
blockName,
layoutBlockSupportKey,
{}
);
const { supportsFlowLayout, config } = useLayout( blockName );
const {
allowSwitching,
allowEditing = true,
allowInheriting = true,
default: defaultBlockLayout,
} = layoutBlockSupport;
} = config;

if ( ! allowEditing ) {
return null;
Expand All @@ -104,18 +95,14 @@ function LayoutPanel( { setAttributes, attributes, name: blockName } ) {
// and that the default / flow layout type is in use, as this is the only one that supports inheritance.
const showInheritToggle = !! (
allowInheriting &&
!! defaultThemeLayout &&
!! supportsFlowLayout &&
( ! layout?.type || layout?.type === 'default' || layout?.inherit )
);

const usedLayout = layout || defaultBlockLayout || {};
const { inherit = false, type = 'default' } = usedLayout;
/**
* `themeSupportsLayout` is only relevant to the `default/flow`
* layout and it should not be taken into account when other
* `layout` types are used.
*/
if ( type === 'default' && ! themeSupportsLayout ) {

if ( type === 'default' && ! supportsFlowLayout ) {
return null;
}
const layoutType = getLayoutType( type );
Expand Down Expand Up @@ -163,7 +150,7 @@ function LayoutPanel( { setAttributes, attributes, name: blockName } ) {
<layoutType.inspectorControls
layout={ usedLayout }
onChange={ onChangeLayout }
layoutBlockSupport={ layoutBlockSupport }
layoutBlockSupport={ config }
/>
) }
</PanelBody>
Expand All @@ -172,7 +159,7 @@ function LayoutPanel( { setAttributes, attributes, name: blockName } ) {
<layoutType.toolBarControls
layout={ usedLayout }
onChange={ onChangeLayout }
layoutBlockSupport={ layoutBlockSupport }
layoutBlockSupport={ config }
/>
) }
</>
Expand All @@ -197,6 +184,26 @@ function LayoutTypeSwitcher( { type, onChange } ) {
);
}

export function useLayout( blockName ) {
const defaultThemeLayout = useSetting( 'layout' );
const themeSupportsLayout = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings().supportsLayout;
}, [] );

const layoutBlockSupport = getBlockSupport(
blockName,
layoutBlockSupportKey,
{}
);

return {
supportsFlowLayout: themeSupportsLayout,
defaultLayout: defaultThemeLayout,
config: layoutBlockSupport,
};
}

/**
* Filters registered block settings, extending attributes to include `layout`.
*
Expand Down Expand Up @@ -276,6 +283,9 @@ export const withLayoutStyles = createHigherOrderComponent(
},
layoutClasses
);
const padding = layout?.inherit
? usedLayout?.padding
: get( attributes, [ 'style', 'spacing', 'padding' ] );

return (
<>
Expand All @@ -286,6 +296,7 @@ export const withLayoutStyles = createHigherOrderComponent(
blockName={ name }
selector={ `.wp-container-${ id }` }
layout={ usedLayout }
padding={ padding }
style={ attributes?.style }
/>,
element
Expand Down
13 changes: 11 additions & 2 deletions packages/block-editor/src/hooks/padding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
} from './dimensions';
import { cleanEmptyObject } from './utils';
import BlockPopover from '../components/block-popover';
import { useLayout } from './layout';

/**
* Determines if there is padding support.
Expand Down Expand Up @@ -80,11 +81,19 @@ export function resetPadding( { attributes = {}, setAttributes } ) {
*
* @return {boolean} Whether padding setting is disabled.
*/
export function useIsPaddingDisabled( { name: blockName } = {} ) {
export function useIsPaddingDisabled( { name: blockName, attributes } = {} ) {
const { supportsFlowLayout, config } = useLayout( blockName );
const hasInheritedLayout =
supportsFlowLayout && !! config && attributes?.layout?.inherit;
const isDisabled = ! useSetting( 'spacing.padding' );
const isInvalid = ! useIsDimensionsSupportValid( blockName, 'padding' );

return ! hasPaddingSupport( blockName ) || isDisabled || isInvalid;
return (
! hasPaddingSupport( blockName ) ||
hasInheritedLayout ||
isDisabled ||
isInvalid
);
}

/**
Expand Down
18 changes: 18 additions & 0 deletions packages/block-editor/src/layouts/flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export default {
layout = {},
style,
blockName,
padding,
} ) {
const { contentSize, wideSize } = layout;
const blockGapSupport = useSetting( 'spacing.blockGap' );
Expand All @@ -126,10 +127,17 @@ export default {
! shouldSkipSerialization( blockName, 'spacing', 'blockGap' )
? blockGapStyleValue?.top
: 'var( --wp--style--block-gap )';
// Using important here for the padding to override the inline padding that could be potentially
// applied using the custom padding control before the layout inheritance is applied.

let output =
!! contentSize || !! wideSize
? `
${ appendSelectors( selector ) } {
padding: ${ padding?.top || 0 } ${ padding?.right || 0 } ${
padding?.bottom || 0
} ${ padding?.left || 0 } !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the best way to go about it. If user sets a padding, we should respect that, and work with its value to add negative margins to full-width items. This is making it impossible to have a Group with both layout (needed for e.g. full width images) and padding, which is not ideal, especially if background color is set:

Screen Shot 2022-04-08 at 5 19 53 pm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making it impossible to have a Group with both layout (needed for e.g. full width images) and padding

Yes, I found that a bit challenging in testing, too, that toggling "Inherit default layout" hid the padding controls, which felt unexpected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the padding visible when we inherit a layout, we'd create another unexpected behavior though:

  • A user can set a padding and this one will be used
  • The user can remove the specific padding and the one from the inherited layout will take over (instead of no padding), we'd have to set 0 to reset the padding.

Not sure if that's confusing enough or if we can explain it to the user.

}
${ appendSelectors(
selector,
'> :where(:not(.alignleft):not(.alignright))'
Expand All @@ -143,6 +151,16 @@ export default {
}
${ appendSelectors( selector, '> .alignfull' ) } {
max-width: none;
${
padding?.left
? `margin-left: calc( -1 * ${ padding?.left } ) !important;`
: ''
}
${
padding?.right
? `margin-right: calc( -1 * ${ padding?.right } ) !important;`
: ''
}
}
`
: '';
Expand Down
9 changes: 9 additions & 0 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,14 @@ export default function VisualEditor( { styles } ) {
titleRef?.current?.focus();
}, [ isWelcomeGuideVisible, isCleanNewPost ] );

const padding = useMemo( () => {
if ( isTemplateMode || ! themeSupportsLayout ) {
return undefined;
}

return defaultLayout?.padding;
} );

return (
<BlockTools
__unstableContentRef={ ref }
Expand Down Expand Up @@ -245,6 +253,7 @@ export default function VisualEditor( { styles } ) {
<LayoutStyle
selector=".edit-post-visual-editor__post-title-wrapper, .block-editor-block-list__layout.is-root-container"
layout={ defaultLayout }
padding={ padding }
/>
) }
{ ! isTemplateMode && (
Expand Down