#38672 closed defect (bug) (fixed)
Custom CSS should work with existing Jetpack custom CSS
Reported by: | helen | Owned by: | 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
Attachments (11)
Change History (66)
#2
follow-up:
↓ 3
@
8 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
@
8 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
@
8 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
@
8 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.
#6
@
8 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:
↓ 8
@
8 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:
↓ 10
@
8 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.
#9
@
8 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) :(
#10
in reply to:
↑ 8
;
follow-up:
↓ 11
@
8 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 usesafecss
.
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 namedunfiltered_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 tweakedwp_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:
↓ 12
@
8 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 thatcustom_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
@
8 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 astr_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
@
8 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:
↓ 15
@
8 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
@
8 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
toedit_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 theunfiltered_css
being what's tested on saving. If that makes sense?
Strongly agree here
#16
follow-up:
↓ 18
@
8 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 ) { 320 320 else 321 321 $caps[] = 'do_not_allow'; 322 322 break; 323 case 'unfiltered_css' : 323 case 'edit_css' : 324 $caps[] = 'unfiltered_html'; 325 break; 324 326 case 'unfiltered_html' : 325 327 // Disallow unfiltered_html for all users, even admins and super admins. 326 328 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
@
8 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
@
8 years ago
Replying to westonruter:
Re: 38672.diff should
edit_css
map tounfiltered_html
according to the same rules as howunfiltered_html
maps? In other words, shouldDISALLOW_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 theunfiltered_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:
↓ 20
@
8 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
@
8 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 thecapability
when theWP_Customize_Custom_CSS_Setting
is registered. With vanilla core this would map tounfiltered_html
(orunfiltered_css
?) and would thus be restricted to admins/superadmins. A plugin (read: Jetpack) which adds a sanitization library could then re-mapedit_css
to something more permissive likeedit_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
@
8 years ago
@westonruter
So the default is unfiltered_css
is mapped to unfiltered_html
.
https://github.com/WordPress/WordPress/commit/1ccd9e7a6c7bc4a0f9ffae670c526f9420a3edeb
#22
@
8 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
@
8 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:
↓ 26
@
8 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
@
8 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:
- Jetpack detects Core's setup
- Jetpack migrates its CSS to Core's storage,
- Jetpack disables the wp-admin UI
- Jetpack enabled its enhancement to Core
Wouldn't that work?
What are the downsides of it?
Would it need any change in Core?
#26
in reply to:
↑ 24
;
follow-up:
↓ 30
@
8 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 lackunfiltered_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.
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#30
in reply to:
↑ 26
@
8 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
@
8 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.
#34
@
8 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.
8 years ago
#36
@
8 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 inWP_Customize_Custom_CSS_Setting::update()
and inwp_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
@
8 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.
8 years ago
#40
@
8 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 fromWP_Customize_Custom_CSS_Setting
class to allow extensibility. - Eliminate use of
wp_get_custom_css()
when getting the setting value. Use the underlyingpost_value
directly whenis_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
, andbackground
into named functions on thewp.customize.settingPreviewHandlers
to allow plugins to override/extend. - Update
_custom_background_cb
to always print astyle
tag in the customizer preview. - Update
background
preview logic to replace existingstyle
element instead of appending a newstyle
to thehead
, and thus unexpectedly overriding any Custom CSS in the preview.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
8 years ago
#42
@
8 years ago
In 38672.extensibility.2.diff:
- Introduce
customize_value_custom_css
filter inWP_Customize_Custom_CSS::value()
method. - Introduce
customize_update_custom_css_post_content_args
filter inWP_Customize_Custom_CSS::update()
method. - Restore
final
keyword onWP_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.
8 years ago
This ticket was mentioned in Slack in #core-customize by colorful-tones. View the logs.
8 years ago
#48
follow-up:
↓ 49
@
8 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
@
8 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:
#50
@
8 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
@
8 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.
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.