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

Block editor: add a way to extend style dependency handles for editor content #37466

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions lib/experimental/content-styles.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

/**
* Resolves WP Dependency handles to HTML.
*
* @param array $instance WP Dependency instance.
* @param array|null $handles Handles to resolve.
*
* @return string HTML.
*/
function gutenberg_resolve_dependencies( $instance, $handles ) {
if ( ! $handles || count( $handles ) === 0 ) {
return '';
}

ob_start();

$done = $instance->done;
$instance->done = array();
$instance->do_items( array_unique( $handles ) );
$instance->done = $done;

return ob_get_clean();
}

add_filter(
'block_editor_settings_all',
function( $settings ) {
// In the future the above assets should be passed through here as well,
// but some stylesheets cannot be prefixed without specificity issues,
// so we need to make an exception.
$settings['__unstableResolvedContentStyles'] = gutenberg_resolve_dependencies(
wp_styles(),
isset( $settings['__experimentalContentStyles'] ) ? $settings['__experimentalContentStyles'] : array()
);
return $settings;
},
100
);
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ function gutenberg_is_experiment_enabled( $name ) {
}

require __DIR__ . '/experimental/editor-settings.php';
require __DIR__ . '/experimental/content-styles.php';

// Gutenberg plugin compat.
require __DIR__ . '/compat/plugin/edit-site-routes-backwards-compat.php';
Expand Down
28 changes: 19 additions & 9 deletions packages/block-editor/src/components/block-preview/auto.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@ function AutoBlockPreview( {
useResizeObserver();
const [ contentResizeListener, { height: contentHeight } ] =
useResizeObserver();
const { styles, assets, duotone } = useSelect( ( select ) => {
const settings = select( store ).getSettings();
return {
styles: settings.styles,
assets: settings.__unstableResolvedAssets,
duotone: settings.__experimentalFeatures?.color?.duotone,
};
}, [] );
const { styles, assets, __unstableResolvedContentStyles, duotone } =
useSelect( ( select ) => {
const settings = select( store ).getSettings();
return {
styles: settings.styles,
assets: settings.__unstableResolvedAssets,
duotone: settings.__experimentalFeatures?.color?.duotone,
__unstableResolvedContentStyles:
settings.__unstableResolvedContentStyles,
};
}, [] );

// Avoid scrollbars for pattern previews.
const editorStyles = useMemo( () => {
Expand Down Expand Up @@ -77,7 +80,14 @@ function AutoBlockPreview( {
} }
>
<Iframe
head={ <EditorStyles styles={ editorStyles } /> }
head={
<EditorStyles
styles={ editorStyles }
__unstableResolvedContentStyles={
__unstableResolvedContentStyles
}
/>
}
assets={ assets }
contentRef={ useRefEffect( ( bodyElement ) => {
const {
Expand Down
82 changes: 81 additions & 1 deletion packages/block-editor/src/components/editor-styles/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,71 @@ function useDarkThemeBodyClassName( styles ) {
);
}

export default function EditorStyles( { styles } ) {
function useParsedAssets( html ) {
return useMemo( () => {
const doc = document.implementation.createHTMLDocument( '' );
doc.body.innerHTML = html;
return Array.from( doc.body.children );
}, [ html ] );
}

function PrefixedStyle( {
tagName,
__unstablePrefix,
href,
id,
rel,
media,
textContent,
} ) {
const TagName = tagName.toLowerCase();

function onLoad( event ) {
Copy link
Member

@oandregal oandregal Dec 23, 2021

Choose a reason for hiding this comment

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

This technique is cool, though there's a noticeable delay in the styles being applied. Presumably, because we add the stylesheet in the client after all things have been already loaded and the onLoad event happens even after.

Thinking about how to improve this:

  • What if we convert all styles (both link and style) to embedded styles (style) and prefixed them just here, so we don't wait to onLoad to do it?
  • Alternatively, would it make sense to add the stylesheets in the server (no passing them through __unstableResolvedContentStyles) and attach to its onLoad event a public function that takes care of prefixing them? While we still rely on the onLoad event here it'll happen earlier because we're not adding it in the client.

Copy link
Member Author

@ellatrix ellatrix Dec 23, 2021

Choose a reason for hiding this comment

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

The good thing here is that we don't have to do any CSS parsing. I talked with @mcsf about the delay. I wonder if we could block the original CSS from applying, maybe by emptying and later refilling it. 🤔
It would be good if we don't have to do any parsing. I was hoping there would be some API to scope link and style tags to a given selector, but that doesn't exist apparently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@oandregal Could you remind me where you see the delay?

I now removed the prefixing from the iframes since it's not needed there. Only the "old" non iframe editors will need prefixing.

Copy link
Member

Choose a reason for hiding this comment

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

I was able to repro with a big post (Gutenberg demo). It's more noticeable the 1st time the post is loaded and if you disable the cache. Also by throtling the network and/or the CPU it becomes more visible.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is more visible? What do you see? Could you record it?

const { sheet, ownerDocument } = event.target;
const { defaultView } = ownerDocument;
const { CSSStyleRule } = defaultView;
const { cssRules } = sheet;

let index = 0;

for ( const rule of cssRules ) {
const { selectorText, cssText } = rule;

if ( ! ( rule instanceof CSSStyleRule ) ) {
continue;
}

if ( selectorText.includes( EDITOR_STYLES_SELECTOR ) ) {
continue;
}

sheet.deleteRule( index );
sheet.insertRule(
'html :where(.editor-styles-wrapper) ' + cssText,
Copy link
Member

Choose a reason for hiding this comment

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

Hey, I don't understand why we're prefixing the styles with html :where(.editor-styles-wrapper) instead of .editor-styles-wapper.

Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken I think it's just to lower the specificity, but I'm also curious to know if that is indeed the case or not 🤔

Copy link
Member

@oandregal oandregal Dec 23, 2021

Choose a reason for hiding this comment

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

Yeah, I understand it lowers the specificity but I don't know why. I guess my question is: why the styles loaded through a wp dependency should be any different than the styles loaded via add_editor_styles?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just used the same thing as #32659. But I guess if we consistently increase the specificity of all styles, this is not necessary. Cc @jasmussen.

Copy link
Member

Choose a reason for hiding this comment

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

Where did we land in this conversation? I'd think we want to use the same we use for styles passed via add_editor_style?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the :where will ensure that the styles that are prefixed with .editor-styles-wrapper have the same specificity in the editor as they have in the frontend.

index
);

index++;
}
}

if ( ! __unstablePrefix ) {
onLoad = null;
}

if ( TagName === 'style' ) {
return <TagName { ...{ id, onLoad } }>{ textContent }</TagName>;
}

return <TagName { ...{ href, id, rel, media, onLoad } } />;
}

export default function EditorStyles( {
styles,
__unstableResolvedContentStyles,
__unstablePrefix,
} ) {
const _styles = useParsedAssets( __unstableResolvedContentStyles );
const transformedStyles = useMemo(
() => transformStyles( styles, EDITOR_STYLES_SELECTOR ),
[ styles ]
Expand All @@ -77,6 +141,22 @@ export default function EditorStyles( { styles } ) {
{ /* Use an empty style element to have a document reference,
but this could be any element. */ }
<style ref={ useDarkThemeBodyClassName( styles ) } />
{ _styles.map(
Copy link
Member

Choose a reason for hiding this comment

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

Where do you think these styles should load? Before or after the other inline styles? I'd think they should load after.

I've also found that styles are now inconsistent, they load in different order depending on the context.

In the front, post editor, and template editor we do:

  1. inline global styles
  2. theme styles (the ones loaded via add_editor_style() in the editors)

In the site editor we do:

  1. theme styles
  2. global styles

cc @scruffian @andrewserong I thought we had them all loading in the same order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can load them after.

( { tagName, href, id, rel, media, textContent } ) => (
<PrefixedStyle
key={ id }
__unstablePrefix={ __unstablePrefix }
{ ...{
tagName,
href,
id,
rel,
media,
textContent,
} }
/>
)
) }
{ transformedStyles.map( ( css, index ) => (
<style key={ index }>{ css }</style>
) ) }
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/store/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,5 +240,5 @@ export const SETTINGS_DEFAULTS = {
},
],

__unstableResolvedAssets: { styles: [], scripts: [] },
__unstableResolvedAssets: { styles: '', scripts: '' },
};
32 changes: 18 additions & 14 deletions packages/block-editor/src/utils/transform-styles/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ import traverse from './traverse';
import urlRewrite from './transforms/url-rewrite';
import wrap from './transforms/wrap';

export function transformStyle( { css, baseURL }, wrapperClassName ) {
const transforms = [];
if ( wrapperClassName ) {
transforms.push( wrap( wrapperClassName ) );
}
if ( baseURL ) {
transforms.push( urlRewrite( baseURL ) );
}
if ( transforms.length ) {
return traverse( css, compose( transforms ) );
}

return css;
}

/**
* Applies a series of CSS rule transforms to wrap selectors inside a given class and/or rewrite URLs depending on the parameters passed.
*
Expand All @@ -23,20 +38,9 @@ import wrap from './transforms/wrap';
* @return {Array} converted rules.
*/
const transformStyles = ( styles, wrapperClassName = '' ) => {
return map( styles, ( { css, baseURL } ) => {
const transforms = [];
if ( wrapperClassName ) {
transforms.push( wrap( wrapperClassName ) );
}
if ( baseURL ) {
transforms.push( urlRewrite( baseURL ) );
}
if ( transforms.length ) {
return traverse( css, compose( transforms ) );
}

return css;
} );
return map( styles, ( style ) =>
transformStyle( style, wrapperClassName )
);
};

export default transformStyles;
30 changes: 30 additions & 0 deletions packages/e2e-tests/plugins/iframed-editor-style.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php
/**
* Plugin Name: Gutenberg Test Iframed Editor Style
* Plugin URI: https://github.com/WordPress/gutenberg
* Author: Gutenberg Team
*
* @package gutenberg-test-iframed-add-editor-style
*/

// Enable the template editor to test the iframed content.
add_action(
'setup_theme',
function() {
add_theme_support( 'block-templates' );
}
);

add_filter(
'block_editor_settings_all',
function( $settings ) {
wp_register_style(
'my-theme-stylesheet',
plugin_dir_url( __FILE__ ) . 'iframed-editor-style/style.css',
array(),
filemtime( plugin_dir_path( __FILE__ ) . 'iframed-editor-style/style.css' )
);
$settings['__experimentalContentStyles'][] = 'my-theme-stylesheet';
return $settings;
}
);
3 changes: 3 additions & 0 deletions packages/e2e-tests/plugins/iframed-editor-style/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
p {
border: 1px solid red
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* WordPress dependencies
*/
import {
activatePlugin,
createNewPost,
deactivatePlugin,
insertBlock,
canvas,
createNewTemplate,
} from '@wordpress/e2e-test-utils';

async function getComputedStyle( context ) {
return await context.evaluate( () => {
const container = document.querySelector( '.wp-block-paragraph' );
return window.getComputedStyle( container )[ 'border-width' ];
} );
}

describe( 'iframed editor styles', () => {
beforeEach( async () => {
await activatePlugin( 'gutenberg-test-iframed-editor-style' );
await createNewPost( { postType: 'page' } );
} );

afterEach( async () => {
await deactivatePlugin( 'gutenberg-test-iframed-editor-style' );
} );

it( 'should load editor styles through the block editor settings', async () => {
await insertBlock( 'Paragraph' );

expect( await getComputedStyle( page ) ).toBe( '1px' );

await createNewTemplate( 'Iframed Test' );
await canvas().waitForSelector( '.wp-block-paragraph' );

expect( await getComputedStyle( canvas() ) ).toBe( '1px' );
} );
} );
24 changes: 22 additions & 2 deletions packages/edit-post/src/components/visual-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,20 @@ function MaybeIframe( {
styles,
assets,
style,
__unstableResolvedContentStyles,
} ) {
const ref = useMouseMoveTypingReset();

if ( ! shouldIframe ) {
return (
<>
<EditorStyles styles={ styles } />
<EditorStyles
styles={ styles }
__unstableResolvedContentStyles={
__unstableResolvedContentStyles
}
__unstablePrefix
/>
<WritingFlow
ref={ contentRef }
className="editor-styles-wrapper"
Expand All @@ -70,7 +77,14 @@ function MaybeIframe( {

return (
<Iframe
head={ <EditorStyles styles={ styles } /> }
head={
<EditorStyles
styles={ styles }
__unstableResolvedContentStyles={
__unstableResolvedContentStyles
}
/>
}
assets={ assets }
ref={ ref }
contentRef={ contentRef }
Expand Down Expand Up @@ -123,6 +137,7 @@ export default function VisualEditor( { styles } ) {
themeSupportsLayout,
assets,
useRootPaddingAwareAlignments,
__unstableResolvedContentStyles,
} = useSelect( ( select ) => {
const _settings = select( blockEditorStore ).getSettings();
return {
Expand All @@ -131,6 +146,8 @@ export default function VisualEditor( { styles } ) {
assets: _settings.__unstableResolvedAssets,
useRootPaddingAwareAlignments:
_settings.__experimentalFeatures?.useRootPaddingAwareAlignments,
__unstableResolvedContentStyles:
_settings.__unstableResolvedContentStyles,
};
}, [] );
const { clearSelectedBlock } = useDispatch( blockEditorStore );
Expand Down Expand Up @@ -251,6 +268,9 @@ export default function VisualEditor( { styles } ) {
styles={ styles }
assets={ assets }
style={ { paddingBottom } }
__unstableResolvedContentStyles={
__unstableResolvedContentStyles
}
>
{ themeSupportsLayout &&
! themeHasDisabledLayoutStyles &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,12 @@ function ResizableEditor( { enableResizing, settings, children, ...props } ) {
style={ enableResizing ? { height } : deviceStyles }
head={
<>
<EditorStyles styles={ settings.styles } />
<EditorStyles
styles={ settings.styles }
__unstableResolvedContentStyles={
settings.__unstableResolvedContentStyles
}
/>
<style>{
// Forming a "block formatting context" to prevent margin collapsing.
// @see https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Block_formatting_context
Expand Down
Loading