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

Styles: add Editor Styles support #9008

Merged
merged 17 commits into from
Sep 5, 2018
Merged

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Aug 15, 2018

Extending the editor styles is not an easy task for theme developers. They have to take care of all the specifity that Gutenberg styles use. And it's prone to errors because Gutenberg can change the wrappers at any time.

In this PR I'm exploring auto-injecting namespaces (specifity) for theme CSS to avoid leaving that to the theme developpers.

A theme author would write:

h2 { color: red; }

The injected CSS would be

.editor-block-list__block h2 { color: red; }

To test it, notice that the editor styles do work for the Twenty Seventeen theme.

@youknowriad youknowriad added the [Feature] Extensibility The ability to extend blocks or the editing experience label Aug 15, 2018
@youknowriad youknowriad self-assigned this Aug 15, 2018
@youknowriad youknowriad requested review from mtias, jasmussen and a team August 15, 2018 08:57
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I think this is pretty cool as a POC! I haven't tried to make a theme so I'm not sure what edge cases this would affect, but the approach makes sense to me. 👍

.process( this.props.settings.styles )
.then( ( css ) => {
const node = document.createElement( 'style' );
node.innerHTML = css;
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a POC but I assume we'd need to sanitise this (I'm not sure what kind of sanitisation, if any, postcss does). Just stating it so it's known 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, If a user installs a theme or a plugin injecting anything here, it's a cautious decision :). Plugin and themes can already do worse than injecting malicious CSS.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see!

Is there any reason not to escape it though? They shouldn't be putting anything here that isn't CSS… I guess I just always default to tinfoil-hat mode 😄

if ( ! this.props.settings.styles ) {
return;
}
postcss( [ wrap( { selector: '.editor-block-list__block' } ) ] )
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for us to have a new class/set of classes we use for this purpose? It might make it easy to disable theme styles or pinpoint what is styling the editor.

I don't know if that'll ruin our specificity; I figured if the class was on the same div as .editor-block-list__block it'd be fine, but I forget if that's the case. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, maybe a new class makes sense. Something clear like .editor-styles-wrapper or something like that.

@tofumatt tofumatt requested a review from a team August 15, 2018 09:11
@jasmussen
Copy link
Contributor

Can confirm the heading is red:

screen shot 2018-08-15 at 11 35 54

Nice work, very interesting. Given that the editing canvas is neither inside its own iframe, nor utilizing Shadow DOM in any way, this seems like a promising alternative way to simplify the instructions here: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

However this would require a build process for themers, no? Is there a way we can do this on the fly in Gutenberg whenever a theme declares it has an editor style for Gutenberg?

Overall I think this could be a positive step in the right direction. But as we simplify these aspects, other challenges present themselves. Notably the fact that the editor markup is wildly different from the front-end markup. This is also discussed here: #8350 (comment), where I use the quote as an example.

Where it gets tricky is when nested blocks are introduced. Take the table, for example. This is basic table markup:

<table>
	<tr>
		<td></td>
	</tr>
	<tr>
		<td></td>
	</tr>
</table>

If we add nesting support to this block, then the table itself will become one block, the table row another, and the table cell a third. There might still be tr's and td's buried in there, but in the editing canvas every one of those elements is now wrapped in a bunch of extra elements necessary for the nesting to function.

We've been able to work around this so far, but at some point we'll likely have to tackle this. Would Shadow DOM help here? Can we use display: contents; to simulate passthrough here?

@youknowriad
Copy link
Contributor Author

So this doesn't require any build step, it works on the fly. Locally I have a commit enabling this for the old "editor styles".

This surfaces some issues

  • sometimes these theme styles do things like body { rule } or body selector { rule }. Since I’m just adding a namespace, these styles won’t be applied.
  • the stylesheets are converted on the fly, which mean the relative links (images,...) in the styles won’t work at the moment.

I believe both of these issues can be fixed using:

1- More smart injecting mechanism. If we detect that the selector body in the selector, we inject the namespace after that
2- Since the CSS should be read and injected, we need to rewrite the relative urls on the fly to inject the right prefixes on the fly.

Alternatively, we can build a dedicated "gutenberg" API and avoid supporting the old editor styles for now.

@youknowriad
Copy link
Contributor Author

So this surfaced something else:

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

For example, in gutenberg we do this to customize the editor's font family

.edit-post-visual-editor, .edit-post-visual-editor p {
  font-family: "Noto Serif", serif;
}

We should update this to support themes extending the default font.

.editor-block-list__block {
  font-family: "Noto Serif", serif;
}

After this change, a theme would be able to provide

body {
  font-family: "my custom font" 
}

and it will work.

@youknowriad
Copy link
Contributor Author

I updated the PR to:

  • Fix the body selectors issues
  • Fix the relative URLs issue (using a custom post CSS plugin)
  • Support the current theme's editor styles.

I'd appreciate some eyes on this PR, to ensure

  • Current themes with editor styles don't break Gutenberg's styling :)
  • Some of these editor styles are applied properly (we don't want to support everything, but basic support should work)

Also, I'd appreciate thoughts on the approach.

@youknowriad
Copy link
Contributor Author

I refactored the Gutenberg default styles to make this #9008 (comment) possible

You can see that if you use The twenty* themes for instance, there's basic support for editor styles.

@youknowriad youknowriad force-pushed the try/auto-wrap-theme-css branch from 039b0ab to da90000 Compare August 20, 2018 11:06
@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 20, 2018

Noting that bundling postCSS in the browser does add some Kb (280). Do you think it's worth it? how can we improve it?

@mtias
Copy link
Member

mtias commented Aug 20, 2018

To make some of the Gutenberg default styles overridable, we should use the exact same specificity, this transformation uses.

Yes, agreed. Might even consider doing these on the fly as well for our editor styles (basically assume we have a shadow dom when we write our CSS).

@mtias mtias added Customization Issues related to Phase 2: Customization efforts [Type] Task Issues or PRs that have been broken down into an individual action to take labels Aug 20, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Noting that bundling postCSS in the browser does add some Kb (300). Do you think it's worth it? how can we improve it?

That's a lot. At the moment we are at 1.2-1.3 MB minified and gzipped JavaScript code served to the users. Adding another 300 kB doesn't seem like a good idea. Should we look into some CSS in JS alternatives?

*
* @return {Promise} - the Promise
*/
function processDecl( result, decl, opts ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we unwrap decl and opts names by providing full names?

@youknowriad
Copy link
Contributor Author

Should we look into some CSS in JS alternatives?

What alternatives do you think of? Ideally, this could be achieved in PHP but I feel rewriting something like PostCSS in PHP (5.2, a rewrite in recent versions exist) seems overkill? We could do a regexp based parser but doesn't seem like a solid solution.

If we were to stick with JavaScript, we can try to build a lighter version of this by looping through the styles with regex or something else, but it doesn't feel solid either or would take a lot of time to make it work properly in all cases.

I'm open to alternatives, but it does feel like a lot of work to achieve the same thing.

@mtias
Copy link
Member

mtias commented Aug 21, 2018

I'm not against the added weight if it solves some very important issues in an elegant way.

@simison
Copy link
Member

simison commented Aug 22, 2018

We could do a regexp based parser but doesn't seem like a solid solution.

postscss-prefixwrap really is just a bunch of regexp so might be there's a way to make this more lightweight?

I've been working on a similar issue in Automattic/wp-calypso#26683 Instead of postcss-prefixwrap, I've been looking into namespace-css. It uses CSS AST to modify CSS, instead of regexp which might be expensive for big stylesheets and can be error-prone.

Some of the shortcomings I learned were that these prefixing libraries don't handle well:

  • body and html (it just prefixes them to .prefix body {} instead of renaming them to .prefix {})
  • @keyframes stay as is thus making name collisions possible
  • Font imports
  • ::selection
  • @page

@youknowriad
Copy link
Contributor Author

The reason I chose postscss-prefixwrap is because it handles body properly. It does exactly that #9008 (comment)

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs. The other drawbacks of namespacing are acceptable IMO.

Also, namespacing is not the only thing we need here, we also need to rewrite the URLs because we're not including the stylesheet by URL, we're reading it and including it in any random webpage, so we need to use absolute URLs.

@simison
Copy link
Member

simison commented Aug 22, 2018

And the bundle size increase is not because of postscss-prefixwrap but it's because of postcss which itself also works on ASTs.

Sure, I was thinking that ditching postscss requirement would bring the size down but if there are other use cases for it than prefixing, it indeed does make sense to keep it. 👍

@youknowriad
Copy link
Contributor Author

I wonder if using just CSS AST and looping through the rules would work, we'd need to recreate CSS namespacing and URL replacement our selves though. Worth a try.

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 23, 2018

I updated the PR to use the CSS AST approach instead of PostCSS. We gain something like 200Kb (Thanks @simison for the hint). That said there are some issues with the css dependency.

It uses fs which makes it node dependent. The thing is, we don't need fs because it's only used in source maps generation which we don't use at all. So I just added fs to our webpack config as external. We also have some warnings when we run the build (due to the sourcemaps too), So I was wondering how to proceed. I'm leaning towards forking the css dependency inside Gutenberg and just remove the sourcemap handling entirely. Thoughts (@mtias @gziolo @aduth @simison )

@youknowriad
Copy link
Contributor Author

@haszari See the changes to the page you link to in this PR

In summary. you can define a stylesheet like this https://github.com/WordPress/gutenberg/blob/c58f0eb98bd9ffab76607f1bf3cef5f76d5dc635/packages/editor/src/editor-styles.scss (like the old editor styles file)

and do add_theme_support( 'editor-styles' ); in your theme to load the editor styles in Gutenberg as well.

In these editor styles, you can also target blocks directly without any wrapper .wp-block-quote { } ...

@youknowriad youknowriad merged commit 4fec0d2 into master Sep 5, 2018
@youknowriad youknowriad deleted the try/auto-wrap-theme-css branch September 5, 2018 08:30
@youknowriad
Copy link
Contributor Author

This is not perfect yet. But let's get it in and iterate. Thanks all

@jasmussen
Copy link
Contributor

@youknowriad
Copy link
Contributor Author

@jasmussen There is a mechanism to set the width in an easier way but not yet the wide and full widths. I don't know yet about the colors.

So my thinking is not yet, we should iterate, test this in some themes and we'll update once we have the final picture.

@haszari
Copy link
Contributor

haszari commented Sep 5, 2018

@youknowriad Thanks for explaining how this works, I'll give it another try soon :)

@haszari
Copy link
Contributor

haszari commented Sep 6, 2018

@youknowriad Update – tried this out and it's working for me, thanks! This should open up lots more possibilities for us :)

@kadamwhite
Copy link
Contributor

First off, this is awesome work! I love the idea.

However, because of the mechanism by which styles are detected and processed, it seems like this completely rules out using a webpack dev server or hot-reloading when developing our styles, as there's no CSS file enqueued for Gutenberg to detect and parse and because new CSS updates may be getting injected periodically. Has anybody else encountered this, and is there any thought about providing a mechanism to permit usage of modern dev server tooling when developing Gutenberg editor styles? Forgive me if I've overlooked something.

@youknowriad
Copy link
Contributor Author

@kadamwhite I understand the pain here but unfortunately, I can't see how we can make it work with Hot reloading. I'd suggest just reloading the page instead of "hot reloading" when building editor styles.

@fklein-lu
Copy link
Contributor

While I really like this feature, I'm aware that it is not a long term viable solution. Over the medium term, theme developers will just use the build pipeline to generate the editor styles.

To me editor styles are similar to RTL styles. The easiest way to do this is to take the stylesheet, flip it with CSSJanus, enqueue that, and then enqueue a second stylesheet that overrides the rules of the automatically generated stylesheet that are not completely valid.

So once we have a build pipeline plugin for GB editor styles, it can be integrated into the normal development workflow, and you'll then have hot reloading again.

@youknowriad
Copy link
Contributor Author

@fklein-lu This definitely can be achieved at build time but while some developpers have a build step, others don't and don't want to learn how to do so especially theme developpers. We can't force developers to use build tools. That's why we opted for a runtime solution.

@kadamwhite
Copy link
Contributor

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

@youknowriad
Copy link
Contributor Author

youknowriad commented Sep 14, 2018

One important advantage of making it runtime compared to a build tool is that we're not attached to a specific DOM structure, the transformations can be updated over time to match Gutenberg classes etc...

On the go, so apologies if I’ve missed this while skimming the code: but what about making it filterable, so a dev who wanted a build step could turn this off on a specific big project?

This is already possible, you just enqueue your CSS script like you'd do prior to this PR. The transformations should be rewritten using post-css in your build scripts.

We could then release a canonical package that includes a postcss transform or some such that could be used to decorate a CSS file with the same wrapper classes, providing the same basic benefit for the smaller subset of developers who want a build system.

The PostCSS transforms (not all of them) are available in the first commits, I think it's a great idea to showcase those but not certain if it should be "community" territory or "Core" especially because of the point about the backwards compatibility (DOM structure changes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts [Feature] Custom Editor Styles Functionality for adding custom editor styles [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.