Adding new CSS assets to the page by appending a rendered HTML snippet conflicts with a Content-Security-Policy that does not include style-src 'unsafe-inline' #3100151: Adding JS/CSS assets in AJAX responses requires 'unsafe-inline' Content Security Policy
Drupal 9.5 / 10.0 added a new AddJsCommand for JavaScript assets, in order to resolve issues with out-of-order execution of new JS files, but the loadjs library also adds assets to the page in a way that does not conflict with CSP inline code restrictions and can be used for CSS files as well.
Drupal\Core\Ajax\AddCssCommand was introduced for IE <= 9, because they didn't support @import statements if they were in a PrependCommand.
Drupal 8 no longer uses @import statements (#2897408: Remove IE9 support from CssCollectionRenderer and provide it in contrib instead), and has now also removed the IE9 support code from AddCssCommand (#3100147: Remove @import parsing from Drupal.AjaxCommands.prototype.add_css), so AddCssCommand now effectively operates the same as PrependCommand.
Alter \Drupal\Core\Ajax\AddCssCommand to accept an array of CSS assets in addition to a rendered HTML string (retained for backwards compatibility until Drupal 11.0.0), adding the assets to the page with the loadjs library.
None
Passing a rendered HTML string to Drupal\Core\Ajax\AddCssCommand is deprecated.
None
Passing a string to AddCssCommand is now deprecated, instead an array of attributes is expected like for AddJsCommand. CSS files added with Ajax commands are now loaded with loadjs and Ajax commands wait for all CSS files to load before executing the next commands.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !1420
Comments
Comment #2
gappleAlso worth noting that prepending / appending HTML that includes script or style elements isn't compatible with Content Security Policy.
A CSP-compatible alternative for adding assets in an AJAX response is in this issue: #3100151: Adding JS/CSS assets in AJAX responses requires 'unsafe-inline' Content Security Policy
Comment #4
gappleHere's a patch that adds deprecation notices, and replaces the one use of
AddCssCommand. Corresponding tests will need to be updated.Comment #5
gappleI think this should address the test changes needed
Comment #6
gappleComment #9
gappleMove functional test of
add_cssto a separate, legacy, test methodComment #11
gappleTest fix
Comment #12
gappleAdded
@expectedDeprecationannotations to testsComment #14
gappleComment #15
gapple@expectedDeprecationannotation is deprecated,ExpectDeprecationTrait::expectDeprecation()method should now be used insteadhttps://www.drupal.org/node/3176667
Comment #18
gappleAn alternative to complete deprecation could be to deprecate passing a rendered HTML string, but allow passing an array of URLs similar to #3228352: Add an add_js ajax command that uses promises. The loadjs library might not be necessary, but would allow returning a promise so that actions could be deferred until all CSS is loaded.
Comment #19
wim leers#3228352: Add an add_js ajax command that uses promises is adding the JS sibling command.
IMHO it's actually a conceptually different action, and the fact that it's abstracted into a specific command keeps the door option for interesting capabilities in the future.
So IMHO it's fine as-is.
Assigning to @nod_ for feedback.
Comment #20
gappleMy approach to #3100151: Adding JS/CSS assets in AJAX responses requires 'unsafe-inline' Content Security Policy (and currently implemented in csp_extras module) was to introduce a new
AddAssetsCommandthat handled both CSS and JS, but I see the benefit of dedicated commands for each asset type.The current implementation of
AddCssCommandprepending a rendered HTML string still has the issue of requiring a page's content security policy includestyle-src 'unsafe-inline', reducing the protection that csp can offer to any page using the drupal.ajax library. Updating it to accept an array of assets in the same manner as the proposedadd_jscommand would allow it to add the assets to the page in a CSP-compatible manner.Comment #22
gappleMerge Request retains support for passing a rendered HTML string which is appended to the document head, but will trigger a deprecation warning.
In order to retain support for IE Conditional Comments, which won't be removed until Drupal 10.0.0, if any assets have a
#browsersattribute specified the whole array of assets is rendered to HTML - selectively rendering only those assets and passing the rest to AddCssCommand as an array could introduce issues with asset reordering.Comment #23
gappleoops, fixing title.
Comment #24
wim leers#20: TIL! Yes, improving
AddCssCommandmakes a ton of sense!Posted an initial review :)
Comment #25
gappleI've rebased to 9.4, and updated the JS function to use the loadjs library, based on the changes for add_js, though the deferred may not be necessary for CSS.
Comment #26
gappleComment #28
nod_left a couple of minor comments, This is a good idea +1 to it.
Comment #30
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #31
gappleDid a rebase & squash, since merge conflicts were pretty significant, and addressed remaining comments.
I changed the jQuery Deferred to a native Promise, matching
add_js. I'm still not sure that it is actually necessary - there isn't a concern about out-of-order execution like with new JS files that was the reason for introducing theadd_jscommand.Comment #32
gappleTests are now passing, and I've updated the draft Change Record
Comment #33
andypostIt looks nice and ready except legacy test for JS deprecation message
Comment #34
gappleAdded a test for a the JavaScript deprecation message
Comment #35
andypostThanks 👍 last mossing brick is follow-up to remove deprecation in 11.x - todo needs link to issue
Comment #36
andypostFiled follow-up #3339374: [11.x] Require array argument for AddCssCommand fixed TODOs https://git.drupalcode.org/project/drupal/-/merge_requests/1420/diffs?co...
Comment #37
andypostComment #38
smustgrave commentedChange record makes sense (to me)
Test coverage of deprecation is there
New tests for new way to do the call is there
No remaining tasks
All remaining threads closed
This looks good to me.
Comment #39
nod_updated release notes.
I was about to commit this but have a question, with this change it's not possible to add inline CSS with the addCss command. It's easy enough to add a new append/prepend command with the styles to do the same but just wondering if we're all good with that.
Comment #41
nod_There is an easy workaround, let's move forward and fix if/when necessary
Committed 8496c2c and pushed to 10.1.x. Thanks!
Comment #43
wim leersVery nice!
Comment #45
acbramley commentedThis introduced a fairly convoluted regression #3378341: claro.jquery.ui css assets may be added the page multiple times
Comment #46
dieterholvoet commentedThis broke the Critical CSS module: #3389128: Appending inline critical CSS is broken in AJAX requests since Drupal 10.1.0
Comment #47
jonmcl commentedPrevious behavior was to prepend the CSS:
$('head').prepend(response.data);I think the new code is appending to
<head>which can cause order of precedence issues. A module might have CSS component assets (weight of 0) which are added into<head>after our theme's CSS_THEME assets (weight of 200).This might not be an issue for most situation, but I just noticed it with some quick D10 testing in advance of an upgrade. Maybe it was just a coincidence that we were able to have our theme CSS override the module's component CSS?
Any known workarounds for this (potential) issue?
Comment #48
yonailo commentedWe are experiencing an issue with z-index in the media library widget related to this CSS sorting change.
It seems that https://www.drupal.org/project/drupal/issues/3396505 fixes our issue.
Hope this helps.
Comment #49
yonailo commentedComment #50
vincent.hoehnWe also have a problem with the z-index and the media-library-widget. It occurred after the update from 10.1.7 to 10.2.0.