-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
There was a problem hiding this 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; |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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' } ) ] ) |
There was a problem hiding this comment.
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. 🤷♂️
There was a problem hiding this comment.
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.
Can confirm the heading is red: 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
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 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 |
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
I believe both of these issues can be fixed using: 1- More smart injecting mechanism. If we detect that the selector Alternatively, we can build a dedicated "gutenberg" API and avoid supporting the old editor styles for now. |
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. |
I updated the PR to:
I'd appreciate some eyes on this PR, to ensure
Also, I'd appreciate thoughts on the approach. |
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. |
039b0ab
to
da90000
Compare
Noting that bundling postCSS in the browser does add some Kb (280). Do you think it's worth it? how can we improve it? |
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). |
There was a problem hiding this 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?
packages/postcss-url/src/index.js
Outdated
* | ||
* @return {Promise} - the Promise | ||
*/ | ||
function processDecl( result, decl, opts ) { |
There was a problem hiding this comment.
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?
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. |
I'm not against the added weight if it solves some very important issues in an elegant way. |
I've been working on a similar issue in Automattic/wp-calypso#26683 Instead of Some of the shortcomings I learned were that these prefixing libraries don't handle well:
|
The reason I chose And the bundle size increase is not because of 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. |
Sure, I was thinking that ditching |
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. |
I updated the PR to use the CSS AST approach instead of PostCSS. We gain something like It uses |
@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 In these editor styles, you can also target blocks directly without any wrapper |
This is not perfect yet. But let's get it in and iterate. Thanks all |
Thanks Riad, this is amazing. Should we update these sections: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#basic-colors |
@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. |
@youknowriad Thanks for explaining how this works, I'll give it another try soon :) |
@youknowriad Update – tried this out and it's working for me, thanks! This should open up lots more possibilities for us :) |
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. |
@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. |
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. |
@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. |
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. |
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...
This is already possible, you just enqueue your CSS script like you'd do prior to this PR. The transformations should be rewritten using
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) |
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:
The injected CSS would be
To test it, notice that the editor styles do work for the Twenty Seventeen theme.