Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38672 closed defect (bug) (fixed)

Custom CSS should work with existing Jetpack custom CSS

Reported by: helen's profile helen Owned by: georgestephanis's profile georgestephanis
Milestone: 4.7 Priority: highest omg bbq
Severity: blocker Version:
Component: Customize Keywords: has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

As somebody who's used Jetpack's custom CSS extensively in the past, it was surprising to me on my existing site that the new panel in the "Additional CSS" customizer didn't show me what I currently have in the screen that's called "Edit CSS". Whether people like it or not, Jetpack is a hugely popular plugin and core should be aiming to make good user experiences - this gap is a bad one.

It may be that Jetpack needs to do some amount of shuffling as well, which is fine. This just overall needs looking into, and if there's anything else out there that has similarly huge adoption, we should make sure that that has also deliberately been considered.

Main ticket: #35395

Change History (66)

#1 follow-up: @beaulebens
7 years ago

On the Jetpack side, we're more than happy to work out a plan for making things compatible, and honestly are a bit surprised that this feature happened completely independently, with no input or interaction. Jetpack has had Custom CSS since 2012.

If this is shipping in core, then we'd prefer to deprecate our version entirely, we'd just want to make sure that things are compatible, and sensible for everyone who has used the Jetpack version previously.

#2 follow-up: @lukecavanagh
7 years ago

@beaulebens

But the Jetpack Custom CSS will stay active, if a user change themes. So it will still be of use if a user has CSS for plugin changes. Whilst the custom css in core is not currently global and will only be active per theme.

#3 in reply to: ↑ 2 @beaulebens
7 years ago

Replying to lukecavanagh:

I'm not really sure what you're saying. Are you saying we should just leave things as they are, and ship both? That seems really confusing, not to mention redundant for users.

We can definitely look at making changes to how the Jetpack one works, if it makes sense to bring it into line with something core is shipping, or even look at just porting differences over to what core ships, and deprecating ours entirely.

At the moment, it looks like the proposed core one only exists in the Customizer, and is theme-specific. It's a basic textarea only.

Jetpack's, on the other hand, is _not_ in the Customizer (appears as Appearance > Edit CSS), and is "global" (not theme-specific). It is a syntax-highlighted textarea, saves revisions, and also allows for pre-processing if the user desires.

We could look at merging this functionality, or we (Jetpack) can perhaps just make it so that our editor comes in as an upgrade to what core provides.

At the very least, it seems like it would make sense to make the way things are stored be compatible?

Can you (or someone) point me to any decision process around why the core editor is theme-specific vs global perhaps? That seems like one of the most fundamental actual differences right now.

#4 @lukecavanagh
7 years ago

@beaulebens

I still think users need a global option, so plugin specific CSS can carry over. Maybe if there was a revision of the CSS which showed then the previous CSS from that post revision could be restored when a user changed the active theme.

#5 @helen
7 years ago

Jetpack's implementation does clear between theme changes, so while it appears to only use one parent post object and indicate in each revision for that post object what the theme active at the time was, as far as what faces users it's theme-specific and not global. This means I can browse revisions to copy back what I had before, but it's not really all that intuitive or usable because of the way revisions are displayed (can't make a sane selection to copy-paste).

Because what's in core is a basic textarea in the customizer only, it seems to me that from a UI/UX front Jetpack could "enhance" things, such as the IDE-esque and preprocessor features. We should then work together to bring those enhancements into core as they make sense, and perhaps even work within Jetpack to experiment with other enhancements (seeing revisions in the customizer interface, for instance). I also don't mind there not being an admin interface for the core one straight off the bat or even really ever, as having a separate preview is infuriating. Editing in a narrow sidebar is kind of annoying, but at least it's not infuriating. Jetpack can keep on with adding its existing admin screen, as its users are probably quite used to it, and maybe let users know they can now edit that CSS with awesome live previews in the customizer.

Since we don't really have interface collisions here, I think the technical difference may really boil down to the name of the CPT and whether you start a new post for each theme with its own revisions (as core is doing) or you use one post with revisions forever with the theme info stored for each revision (as Jetpack appears to be doing).

Core can change what it's doing fairly easily because we're in beta, but I want to make sure we are working with the Jetpack team just as we should work with anybody behind a plugin with major adoption whose features match something that is coming into core. I really hope this was an honest oversight and not a case of not-invented-here, "ugh Automattic" syndrome, because this is an incredibly annoying thing for myself and many others coming awfully late in the development cycle. There is a serious risk of dropping this feature from 4.7 because of this.

Last edited 7 years ago by helen (previous) (diff)

#6 @helen
7 years ago

  • Priority changed from high to highest omg bbq
  • Severity changed from normal to blocker

I consider this a blocker for the next beta.

#7 in reply to: ↑ 1 ; follow-up: @westonruter
7 years ago

Replying to beaulebens:

On the Jetpack side, we're more than happy to work out a plan for making things compatible, and honestly are a bit surprised that this feature happened completely independently, with no input or interaction. Jetpack has had Custom CSS since 2012.

For what it's worth, I did ping the Jetpack team about the developments of the Additional CSS feature being worked on in core: https://github.com/Automattic/jetpack/issues/1621#issuecomment-247155624

Also, the Custom CSS feature in Jetpack is not the only plugin that provides the feature, so I don't think that it's necessarily something that needed to be dependent on collaboration with specifically Jetpack, though earlier collaboration would certainly have been ideal. Naturally the two should not be incompatible as the feature in Jetpack may have the most marketshare.

Can you (or someone) point me to any decision process around why the core editor is theme-specific vs global perhaps? That seems like one of the most fundamental actual differences right now.

Yes. The decision there was centered around the fact that themes normally have different styles and so the styles that are added to customize the appearance of one theme may not apply correctly when applying to another theme. It's the same thought process behind the decision to make Custom Logo a theme mod and not an option. Consider a theme having a different color scheme or even elements which have different class names in the markup. In #38549 I've suggested that a user be able to import CSS from another theme via a dropdown to opt-in instead of having it copy by default.

As you noted, the custom CSS feature currently merged in core is pretty stripped-down in terms of its UI and functionality. If we can make sure the underlying infrastructure is aligned, I love your idea to let Jetpack then extend what is in core and provide a large user base to test the improvements which could then be merged into core in future releases.

#8 in reply to: ↑ 7 ; follow-up: @georgestephanis
7 years ago

Replying to westonruter:

For what it's worth, I did ping the Jetpack team about the developments of the Additional CSS feature being worked on in core: https://github.com/Automattic/jetpack/issues/1621#issuecomment-247155624

Ack! Posting a comment on a year-and-a-half old issue isn't a great way to get eyes on it, unfortunately. :( That was somewhat compounded by the fact that it was right at the start of our Grand Meetup, so kinda doubtful anyone caught it.

I think someone had DM'd me on Slack at it some point in the past, but I had no idea it was even being considered for merging into 4.7 until I saw some chatter about it this morning.

For anyone else coming in via this ticket, the Custom CSS ticket was #35395 and the big commit was [38829]

For a brief technical summary of how the core implementation operates (because I can't seem to find one anywhere else) -- it works similar to the Jetpack/WPCOM one, storing it in a Custom Post Type. Core uses custom_css whereas Jetpack and WPCOM use safecss. The Core implementation doesn't attempt at any sort of sanitization or normalization of the CSS ( :\ ), or any syntax highlighting (I don't blame you, the JS library we use adds a lot of weight to Jetpack). The Core Post Type doesn't support revisions either (which I feel is probably a mistake).

The Core implementation also just echoes the generated CSS on wp_head -- personally I'd prefer it if core tweaked wp_add_inline_style to allow adding inline styles without an external stylesheet to attach it to, so it could be dequeued or the like as needed, just as any other style is treated. (Or, depending on the size of the styles, perhaps enqueueing a link to ... a REST API endpoint that echoes the CSS so it can be cached? Or at least strip out newlines and tabs when printing it to the page?)

@beaulebens -- I worry that if we drop Jetpack's Custom CSS entirely in favor of just prettying up the Core implementation, a lot of users are going to get left out in the cold. Permissions for each are radically different. For example, you must be a superadmin on multisite to use the core implementation, because they don't even make an attempt at sanitizing it -- treating it akin to the unfiltered_html cap (it's actually mapped to that and named unfiltered_css).

I have serious misgivings about how the Core implementation will work for users in multisite, it doesn't feel very well thought out, merely defaulting to just taking it away.

I don't know what the ideal solution is /yet/, for how we play nicely with the Core implementation -- it may be to add Sass/LESS preprocessors, Syntax highlighting, Revisions, and Sanitization as an enhancement to the Core implementation -- with some sort of "nag" link at the top asking the user if they'd like to copy their existing Jetpack CSS into the new modal to work on further? Whatever it is, we need to do it in a way that either when activating Jetpack's Custom CSS or deactivating Jetpack's Custom CSS, no data is lost. Which could be particularly tricky.

I'm not thrilled with how quickly this feature was merged into core after first being actively discussed, it gives very little time for plugin authors to react and work to sort out interop between various data structures.

Last edited 7 years ago by georgestephanis (previous) (diff)

#9 @georgestephanis
7 years ago

Honestly, if the Core implementation isn't using revisions, I'm not sure why Core isn't storing the CSS in a theme mod? Revisions really are the number one advantage to using a post type (imo) :(

Last edited 7 years ago by georgestephanis (previous) (diff)

#10 in reply to: ↑ 8 ; follow-up: @westonruter
7 years ago

Replying to georgestephanis:

Replying to westonruter:

For what it's worth, I did ping the Jetpack team about the developments of the Additional CSS feature being worked on in core: https://github.com/Automattic/jetpack/issues/1621#issuecomment-247155624

Ack! Posting a comment on a year-and-a-half old issue isn't a great way to get eyes on it, unfortunately. :( That was somewhat compounded by the fact that it was right at the start of our Grand Meetup, so kinda doubtful anyone caught it. ¶ I think someone had DM'd me on Slack at it some point in the past, but I had no idea it was even being considered for merging into 4.7 until I saw some chatter about it this morning. […] I'm not thrilled with how quickly this feature was merged into core after first being actively discussed, it gives very little time for plugin authors to react and work to sort out interop between various data structures.

I guess I assumed the Jetpack team would be following the conversations on Slack and the posts on Make/Core and would join the discussions. You can see from the Slack entries on #35395 and from the Trac comments on that ticket that the feature has been discussed extensively from the start of the 4.7 dev cycle in August. I was also surprised by the lack of input from the Jetpack team. Anyway, that's in the past.

For anyone else coming in via this ticket, the Custom CSS ticket was #35395 and the big commit was [38829]

For a brief technical summary of how the core implementation operates (because I can't seem to find one anywhere else) -- it works similar to the Jetpack/WPCOM one, storing it in a Custom Post Type. Core uses custom_css whereas Jetpack and WPCOM use safecss.

There are some implementation details in the feature proposal post: https://make.wordpress.org/core/2016/10/11/feature-proposal-better-theme-customizations-via-custom-css-with-live-previews/

The Core implementation doesn't attempt at any sort of sanitization or normalization of the CSS ( :\ ), or any syntax highlighting (I don't blame you, the JS library we use adds a lot of weight to Jetpack). For example, you must be a superadmin on multisite to use the core implementation, because they don't even make an attempt at sanitizing it -- treating it akin to the unfiltered_html cap (it's actually mapped to that and named unfiltered_css).

There has been extensive discussion about this. We deliberated a lot about whether we should try to incorporate CSSTidy. You can read about that on #35395 and by searching Slack for CSSTidy, @azaozz argued against in comment:36:

“In addition there is no good way to make the user entered CSS "secure". CSSTidy and similar tools can check/fix the syntax but cannot sanitize the CSS for security purposes. I'm not sure such tools exist. New versions of the browsers introduce support for new CSS features pretty much every month. Don't think it is feasible even trying to sanitize all of them. The only way would be to severely limit what is supported then parse the CSS and remove everything else.”

The size of the CSSTidy codebase, the lack of lack of an official maintainer, the lack of unit tests, and the discussion about how it could not 100% guarantee safe CSS—all these contributed to why it wasn't included in the core merge. That being said, a plugin like Jetpack could continue to bundle it and then grant unfiltered_css to everyone, while then invoking CSSTidy when sanitizing CSS to allow it to be available to everyone.

I have serious misgivings about how the Core implementation will work for users in multisite, it doesn't feel very well thought out, merely defaulting to just taking it away.

So yeah, the reason why the unfiltered_css capability maps to unfiltered_html is because it was deemed that CSS could not be made safe for arbitrary users to input. A plugin can enable unfiltered_css for users on multisite just as can be done for granting unfiltered_html.

The Core Post Type doesn't support revisions either (which I feel is probably a mistake). […] Honestly, if the Core implementation isn't using revisions, I'm not sure why Core isn't storing the CSS in a theme mod? Revisions really are the number one advantage to using a post type (imo) :(

The reason for not adding revisions support in core is because there is no UI, yet. Plugins, such as Jetpack, can add revisions post type support. The reasons for the custom post type without revisions is in #35395 comments 19 and 74 among others. A major consideration in using a custom post type was scalability and actually specifically having in mind the WordPress.com environment where there is a 1MB limit on all options combined due to Memcached. But a custom post type was also chosen to support revisions in the future and also so that custom_css posts can be imported and exported via WXR, eventually.

The Core implementation also just echoes the generated CSS on wp_head -- personally I'd prefer it if core tweaked wp_add_inline_style to allow adding inline styles without an external stylesheet to attach it to, so it could be dequeued or the like as needed, just as any other style is treated. (Or, depending on the size of the styles, perhaps enqueueing a link to ... a REST API endpoint that echoes the CSS so it can be cached? Or at least strip out newlines and tabs when printing it to the page?)

I agree with you that wp_add_inline_style() should allow style insertion without being attached to another registered CSS stylesheet.

The reason for having the styles inline is so that they can be dynamically overridden with JavaScript in the customizer preview. We did discuss being able to link to an external resource, especially when in a non-preview context, but for the initial scope it seemed like that could be deferred to a future enhancement.

#11 in reply to: ↑ 10 ; follow-up: @georgestephanis
7 years ago

Replying to westonruter:

That being said, a plugin like Jetpack could continue to bundle it and then grant unfiltered_css to everyone, while then invoking CSSTidy when sanitizing CSS to allow it to be available to everyone.

I'd sooner leave unfiltered_css for just that -- unfiltered css -- and then for non-superadmins that have access to custom css, run it against csstidy to strip out unexpected syntax.

The reason for not adding revisions support in core is because there is no UI, yet. Plugins, such as Jetpack, can add revisions post type support. The reasons for the custom post type without revisions is in #35395 comments 19 and 74 among others. A major consideration in using a custom post type was scalability and actually specifically having in mind the WordPress.com environment where there is a 1MB limit on all options combined due to Memcached. But a custom post type was also chosen to support revisions in the future and also so that custom_css posts can be imported and exported via WXR, eventually.

It'd be much nicer when revisions support is added to core, or a plugin adds it, if the history has already been saved -- as opposed to just throwing out the data and it then being unrecoverable.

Also, WXR doesn't feel right for this -- as the WordPress WXR importer isn't really meant for theme-specific stuff as Custom CSS is? More a gut feeling than a firm opinion, fwiw.

The reason for having the styles inline is so that they can be dynamically overridden with JavaScript in the customizer preview. We did discuss being able to link to an external resource, especially when in a non-preview context, but for the initial scope it seemed like that could be deferred to a future enhancement.

Any thoughts on adding some base level minification when SCRIPT_DEBUG isn't set to true -- even if it's just a str_replace( "\r\n\t", '', $css ); ?

#12 in reply to: ↑ 11 @westonruter
7 years ago

Replying to georgestephanis:

I'd sooner leave unfiltered_css for just that -- unfiltered css -- and then for non-superadmins that have access to custom css, run it against csstidy to strip out unexpected syntax.

That's a good idea. So when CSSTidy is availably, as with Jetpack, the capability restricting with the custom_css setting could be opened up, but then if the user does not have unfiltered_css then Jetpack then the sanitization filters would apply on the setting value. In this case, it would also be beneficial to add selective refresh for the style element so that the actual sanitized CSS would be previewed rather than the pre-sanitized value coming straight from CSS.

It'd be much nicer when revisions support is added to core, or a plugin adds it, if the history has already been saved -- as opposed to just throwing out the data and it then being unrecoverable.

Yeah, I suppose so. It just seems that having revisions in the DB without them being accessible in any way seems like it would be a first time in core where permanent data is kept but it is not accessible?

Also, WXR doesn't feel right for this -- as the WordPress WXR importer isn't really meant for theme-specific stuff as Custom CSS is? More a gut feeling than a firm opinion, fwiw.

If I were trying to export data from one site to import into another, as a user I sure would like it if my custom CSS were included in the export. But for that matter, I'd also want all of my other site options to be included as well, but as you know presently there isn't a core way to export site settings without a plugin. Better something than nothing.

Any thoughts on adding some base level minification when SCRIPT_DEBUG isn't set to true -- even if it's just a str_replace( "\r\n\t", '', $css ); ?

Yeah, some whitespace normalization could be good. The amount of byte savings would border on being trivial though? I also wonder if there could be a problem with whitespace normalization stripping corrupting CSS in any way. I'd feel safer if a CSS tokenizer were used to identify the actual syntactic whitespace, in the same way as we need a tokenizer to properly identify unbalanced parens, braces, and brackets.

#13 @westonruter
7 years ago

@georgestephanis so do we have a path forward to harmonize Jetpack and Core for 4.7? Who would we be collaborating with? With the architectural considerations share in comments above, are there still specific architectural changes that you see as still needing to be done in core? If Jetpack switches to using a separate post specific to the current theme as opposed to using the same post for all themes, would that mean implementing #38549 in Jetpack so that CSS from one theme could easily be copied into the currently-activated theme?

#14 follow-up: @georgestephanis
7 years ago

@westonruter I'm polishing some other stuff off and waiting to hear back internally as to our ideal path forward. I'm going to be largely deferring to @samhotchkiss and @beaulebens on direction, but I'm happy to do what I can to get implementation dealt with once the direction is decided.

One change I would very much like to see in core though is the capability being changed from unfiltered_css to edit_css or the like -- because if the cap is going to be remapped, then the name is really bad. The name should be what the capability allows, with the unfiltered_css being what's tested on saving. If that makes sense?

#15 in reply to: ↑ 14 @samhotchkiss
7 years ago

Replying to georgestephanis:

One change I would very much like to see in core though is the capability being changed from unfiltered_css to edit_css or the like -- because if the cap is going to be remapped, then the name is really bad. The name should be what the capability allows, with the unfiltered_css being what's tested on saving. If that makes sense?

Strongly agree here

@lukecavanagh
7 years ago

Basic patch for edit_css

#16 follow-up: @westonruter
7 years ago

Re: 38672.diff should edit_css map to unfiltered_html according to the same rules as how unfiltered_html maps? In other words, should DISALLOW_UNFILTERED_HTML still be considered or whether it is multisite and the user is a superadmin? It seems the mapping could also be done as:

  • src/wp-includes/capabilities.php

    function map_meta_cap( $cap, $user_id ) { 
    320320                else
    321321                        $caps[] = 'do_not_allow';
    322322                break;
    323         case 'unfiltered_css' :
     323        case 'edit_css' :
     324                $caps[] = 'unfiltered_html';
     325                break;
    324326        case 'unfiltered_html' :
    325327                // Disallow unfiltered_html for all users, even admins and super admins.
    326328                if ( defined( 'DISALLOW_UNFILTERED_HTML' ) && DISALLOW_UNFILTERED_HTML )

Or should it map to unfiltered_css, a brand new primitive capability? Or should we re-use the unfiltered_html primitive?

#17 @ocean90
7 years ago

Worth to mention that there are some other plugins like CSS (probably the first one for the customizer?) or Simple Custom CSS. Both are using an option for storing the CSS though. Shouldn't they also play well with the new setting in the customizer?

@johnregan3 was quite active in the original ticket. Maybe he can tell us about the plans for Simple Custom CSS?

#18 in reply to: ↑ 16 @jorbin
7 years ago

Replying to westonruter:

Re: 38672.diff should edit_css map to unfiltered_html according to the same rules as how unfiltered_html maps? In other words, should DISALLOW_UNFILTERED_HTML still be considered or whether it is multisite and the user is a superadmin? It seems the mapping could also be done as:

Or should it map to unfiltered_css, a brand new primitive capability? Or should we re-use the unfiltered_html primitive?

What about having two capabilities: unfiltered_css and edit_css. right now, both would map to unfiltered_html. Plugins could remap them as they see fit (such as if they implement a sanitizer, mapping edit_css to customize ). In the future, core could decide that edit_css should be mapped to something else.

#19 follow-up: @westonruter
7 years ago

@jorbin I was chatting with @georgestephanis and I think his intention is that a single edit_css meta capability could be passed as the capability when the WP_Customize_Custom_CSS_Setting is registered. With vanilla core this would map to unfiltered_html (or unfiltered_css?) and would thus be restricted to admins/superadmins. A plugin (read: Jetpack) which adds a sanitization library could then re-map edit_css to something more permissive like edit_theme_options so that users who formerly were unable to edit the CSS could then do so. I don't know if it makes sense to have two separate CSS-related meta capabilities.

#20 in reply to: ↑ 19 @jorbin
7 years ago

Replying to westonruter:

@jorbin I was chatting with @georgestephanis and I think his intention is that a single edit_css meta capability could be passed as the capability when the WP_Customize_Custom_CSS_Setting is registered. With vanilla core this would map to unfiltered_html (or unfiltered_css?) and would thus be restricted to admins/superadmins. A plugin (read: Jetpack) which adds a sanitization library could then re-map edit_css to something more permissive like edit_theme_options so that users who formerly were unable to edit the CSS could then do so. I don't know if it makes sense to have two separate CSS-related meta capabilities.

I'm cool with that. My main goal is to ensure 1) vanilla core restricts access to the equivalent of unfiltered_html ( either in and of itself or as unfiltered_css ) and 2) At some point in the future, the feature doesn't need to be restricted as there is well-tested sanitization available in core.

#21 @lukecavanagh
7 years ago

@westonruter

So the default is unfiltered_css is mapped to unfiltered_html.

https://github.com/WordPress/WordPress/commit/1ccd9e7a6c7bc4a0f9ffae670c526f9420a3edeb

Last edited 7 years ago by lukecavanagh (previous) (diff)

#22 @westonruter
7 years ago

@lukecavanagh I know. But if we replace unfiltered_css with edit_css as the meta capability, should we introduce a new unfiltered_css primitive capability to map it to by default, or should we map it to the existing unfiltered_html? And if so, should we follow the same conditionals for honoring DISALLOW_UNFILTERED_HTML and multisite?

#23 @westonruter
7 years ago

Just opened #38705 which will ensure that the user who makes a change to a custom_css setting will have the appropriate sanitization filters applied at the time of publishing (such as scheduled changesets published via WP Cron).

#24 follow-up: @westonruter
7 years ago

@georgestephanis here's what I have in mind for adding restrictive filtering for custom_css settings for underprivileged users (who lack unfiltered_html). The following gives the essential logic for what I think Jetpack could do:

<?php
add_action( 'customize_register', function( WP_Customize_Manager $wp_customize ) {
        $sanitize_css = function( $css ) {
                if ( ! current_user_can( 'unfiltered_html' ) ) {
                        $css = jetpack_sanitize_custom_css( $css );
                }
                return $css;
        };

        foreach ( $wp_customize->settings() as $setting ) {
                if ( $setting instanceof WP_Customize_Custom_CSS_Setting ) {
                        add_filter( "customize_sanitize_{$setting->id}", $sanitize_css, 10, 2 );
                }
        }
}, 20 );

#25 @folletto
7 years ago

make it so that our editor comes in as an upgrade to what core provides.

This seems the ideal approach to me.

... It is a syntax-highlighted textarea, saves revisions, and also allows for pre-processing if the user desires ...
... from a UI/UX front Jetpack could "enhance" things, such as the IDE-esque and preprocessor features ...
... seeing revisions in the customizer interface ...
... Editing in a narrow sidebar is kind of annoying, but at least it's not infuriating ...
... it may be to add Sass/LESS preprocessors, Syntax highlighting, Revisions, and Sanitization as an enhancement ...

Note that there's a design for this already see #38707 too.
Also #31089 that is specifically about designing and building a general approach to revisions in Customizer, which I think would be better than revisions for just CSS, but we can get there incrementally if we prefer building the CSS one first, and then removing it and moving to the global one.

I really hope this was an honest oversight and not a case of not-invented-here, "ugh Automattic" syndrome

Given I was the designer involved (as well as in .com's CSS side) I'd say that's more a technical oversight than a design one. Jetpack's design is old and required a refresh, and both in .com and in the Core discussion in #35395 we were progressing with better alternatives.

Just a note on preprocessors: I'd avoid them, or as mentioned in the other ticket, I'd simply include SCSS, as the syntax is identical (so a user that just knows CSS, simply writes CSS) but at the same time with zero UI needed a user writing SCSS will benefit from it too. No need of dropdowns or configuration flags.

with some sort of "nag" link at the top asking the user if they'd like to copy their existing Jetpack CSS into the new modal to work on further?

I agree on the general approach of moving Jetpack's configuretion to Core's implementation, data wise.

May I ask why it couldn't just be a straight migration like:

  1. Jetpack detects Core's setup
  2. Jetpack migrates its CSS to Core's storage,
  3. Jetpack disables the wp-admin UI
  4. Jetpack enabled its enhancement to Core

Wouldn't that work?
What are the downsides of it?
Would it need any change in Core?

Last edited 7 years ago by folletto (previous) (diff)

#26 in reply to: ↑ 24 ; follow-up: @georgestephanis
7 years ago

Replying to westonruter:

@georgestephanis here's what I have in mind for adding restrictive filtering for custom_css settings for underprivileged users (who lack unfiltered_html). The following gives the essential logic for what I think Jetpack could do:

Honestly, the other option is we could just sanitize everything from everybody. Which wouldn't be a horrible idea, it's what we do already.

#27 @lukecavanagh
7 years ago

@westonruter

It makes sense to map by default edit_css to unfiltered_html.

This ticket was mentioned in Slack in #core by helen. View the logs.


7 years ago

#29 @westonruter
7 years ago

  • Keywords has-patch added

To include in patch: Enable revisions post type support.

#30 in reply to: ↑ 26 @jnylen0
7 years ago

Replying to georgestephanis:

Honestly, the other option is we could just sanitize everything from everybody. Which wouldn't be a horrible idea, it's what we do already.

I don't think we should do this. The sanitization is kind of annoying, it pretty severely restricts what you can do in terms of unrecognized CSS rules (vendor-specific or newer properties/values) and also other things like whitespace and comments.

#31 @westonruter
7 years ago

@jnylen0 I agree. If I have unfiltered_html then I should expect I should be able to enter arbitrary CSS without any sanitization/mutation in the same way that if I can enter arbitrary HTML and not have it changed when saving.

#32 @westonruter
7 years ago

In 39175:

Customize: Rename unfiltered_css meta capability to edit_css; add revisions support to custom_css post type.

Props lukecavanagh, georgestephanis, westonruter.
See #38672, #35395.

#33 @westonruter
7 years ago

In 39178:

Customize: Rename remaining instances unfiltered_css meta capability to edit_css in unit tests.

Amends [39175].
See #38672, #35395.

#34 @westonruter
7 years ago

  • Owner set to georgestephanis
  • Status changed from new to reviewing

@georgestephanis please share if there's anything else we can do on the core side to facilitate a migration/integration path with Jetpack.

This ticket was mentioned in Slack in #core-customize by georgestephanis. View the logs.


7 years ago

#36 @georgestephanis
7 years ago

Tentative needs (discussing in #core-customize)

  • Abstract out wp_get_custom_css_post -- fetching the CPT Post and returning it from existing usages in WP_Customize_Custom_CSS_Setting::update() and in wp_get_custom_css()
  • Filter in WP_Customize_Custom_CSS_Setting::update() to allow modification of cpt before it's saved in the db.

This would be very useful to allow saving of preprocessor stuff -- less, scss -- in post_content_formatted, and the post-processed stuff in post_content of the core CPT -- which should ensure that if they turn off Jetpack, they can still edit the generated css.

If final was removed, I could child class WP_Customize_Custom_CSS_Setting -- but then it's basically treating it as a pluggable function, and multiple plugins could fight over which child class 'wins' -- and then everyone loses. A filter is more stable for extensibility imo

#37 @georgestephanis
7 years ago

Also, if it's possible to put a filter on the return value from WP_Customize_Custom_CSS_Setting::value so we can populate it with the SCSS/LESS instead of the processed CSS.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#39 @westonruter
7 years ago

In 39185:

Customize: Split out custom_css query logic from wp_get_custom_css() into a re-usable wp_get_custom_css_post() function to also be used when updating.

Props georgestephanis, westonruter.
See #38672, #35395.

#40 @westonruter
7 years ago

@georgestephanis ok, I've taken a stab at putting together a standalone SCSS extension to Custom CSS, working out the shortcomings in core that need to be patched (see 38672.extensibility.diff). With that patch applied, take a look at this Custom SCSS Demo plugin and the approach it takes to extend.

Two key things are adding a subclass of WP_Customize_Custom_CSS_Setting to handle the special logic for fetching the SCSS value and updating it into the post_content_filtered. And also the logic for doing selective refresh when SCSS is selected as the preprocessor.

Changes included in 38672.extensibility.diff:

  • Eliminate final-ity from WP_Customize_Custom_CSS_Setting class to allow extensibility.
  • Eliminate use of wp_get_custom_css() when getting the setting value. Use the underlying post_value directly when is_previewed.
  • Make clear that wp_get_custom_css() is specifically for obtaining the value to render/display.
  • Move anonymous functions handing JS previewing for custom_logo, custom_css, and background into named functions on the wp.customize.settingPreviewHandlers to allow plugins to override/extend.
  • Update _custom_background_cb to always print a style tag in the customizer preview.
  • Update background preview logic to replace existing style element instead of appending a new style to the head, and thus unexpectedly overriding any Custom CSS in the preview.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#42 @westonruter
7 years ago

In 38672.extensibility.2.diff:

  • Introduce customize_value_custom_css filter in WP_Customize_Custom_CSS::value() method.
  • Introduce customize_update_custom_css_post_content_args filter in WP_Customize_Custom_CSS::update() method.
  • Restore final keyword on WP_Customize_Custom_CSS.

Also updated the Custom SCSS Demo plugin to make use of the new filters.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#44 @westonruter
7 years ago

  • Keywords has-unit-tests added

#45 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 39209:

Customize: Improve extensibility of Custom CSS.

  • Add customize_value_custom_css filter to WP_Customize_Custom_CSS::value() method.
  • Introduce customize_update_custom_css_post_content_args filter in WP_Customize_Custom_CSS::update() method.
  • Make clear that wp_get_custom_css() and wp_get_custom_css filter are specifically for obtaining the value to render/display. Eliminate use of wp_get_custom_css() when getting the setting value. Use the underlying post_value directly when is_previewed.
  • Move anonymous functions handing JS previewing for custom_logo, custom_css, and background into named functions on the wp.customize.settingPreviewHandlers to allow plugins to override/extend preview logic.
  • Update _custom_background_cb to always print a style tag wen in the customizer preview, and update background preview logic to replace existing style element instead of appending a new style to the head so that background changes don't unexpectedly override any Custom CSS in the preview's stylesheet cascade.

Props westonruter, georgestephanis.
See #22058.
Fixes #38672.

#46 @westonruter
7 years ago

In 39270:

Customize: Prevent edit shortcut buttons from being inserted into container elements in the head or into elements which should not get interactive children.

See #27403, #38672.
Fixes #38830.

This ticket was mentioned in Slack in #core-customize by colorful-tones. View the logs.


7 years ago

#48 follow-up: @johnregan3
7 years ago

@ocean90 Apologies for the late response.
For Simple Custom CSS, I plan to create a migrator tool, and eventually phase out the plugin. I still need to architect a good solution for this, but this feature will render the plugin obsolete.

#49 in reply to: ↑ 48 @georgestephanis
7 years ago

Replying to johnregan3:

For Simple Custom CSS, I plan to create a migrator tool, and eventually phase out the plugin. I still need to architect a good solution for this, but this feature will render the plugin obsolete.

Here's what I've got for the Jetpack migration, so far:

https://github.com/Automattic/jetpack/blob/core/custom-css-4.7/modules/custom-css/migrate-to-core.php

#50 @westonruter
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

A function is needed for a plugin to facilitate writing to custom_css posts without having to bootstrap the customizer. This facilitates plugins that migrate CSS into the custom_css post type, and it will facilitate updating custom_css posts via the REST API and WP-CLI. See https://wordpress.slack.com/archives/core-customize/p1479829461000955

#51 @westonruter
7 years ago

  • Keywords needs-testing added

In 38672.wp_update_custom_css_post.3.diff (see changes) I've renamed customize_update_custom_css_post_content_args filter to just update_custom_css_data and moved it from \WP_Customize_Custom_CSS_Setting::update() to wp_update_custom_css_post(). The filter's arguments are changed to align with the arguments for the new wp_update_custom_css_post() function. This all seems better but it will require some changes to the WIP Jetpack PR and other projects that are in the midst of refactor. But better now change than forever be something we're not happy with architecturally.

#52 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39350:

Customize: Refactor logic for updating custom_css posts by introducing wp_update_custom_css_post() function and renaming update filter.

  • Moves logic from WP_Customize_Custom_CSS_Setting::update() into a re-usable wp_update_custom_css_post() function, useful for future REST API endpoint, WP-CLI command, or plugin migrations.
  • Renames customize_update_custom_css_post_content_args filter to update_custom_css_data and improves the naming of the parameters. Instead of passing post_content and post_content_filtered the filtered array now contains css and preprocessed respectively.
  • The second context param for the update_custom_css_data filter is now an array of the original args passed to wp_update_custom_css_post() and there is now no more $setting arg since it isn't necessarily being called in the customizer context.

Props westonruter, georgestephanis.
See #35395.
Fixes #38672.

#53 @westonruter
7 years ago

In 39477:

Customize: Ensure a custom_css post insertion gets an initial post revision.

Props georgestephanis, westonruter.
See #30854, #38672, #35395.
Fixes #39032.

#54 @westonruter
7 years ago

In 39616:

Customize: Bump wp_custom_css_cb from running at wp_head priority 11 to 101 to ensure Custom CSS overrides other CSS.

Aligns wp_head action priority with Jetpack's Custom CSS.

Amends [38829].
See #35395, #38672.
Fixes #39270.

#55 @dd32
7 years ago

In 39651:

Customize: Bump wp_custom_css_cb from running at wp_head priority 11 to 101 to ensure Custom CSS overrides other CSS.

Aligns wp_head action priority with Jetpack's Custom CSS.

Amends [38829].
See #35395, #38672.
Merges [39616] to the 4.7 branch.
Fixes #39270.

Note: See TracTickets for help on using tickets.