Posted by Jacine on November 9, 2010 at 10:05pm
24 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
Problem
- Stylesheets that have been removed by the theme (via .info) load with AJAX content.
Goal
- Prevent stylesheets that were removed from coming back under all circumstances.
Steps to reproduce
- Remove system.menus.css via a theme's .info file, e.g. add
stylesheets[all][] = system.menus.cssand don't include the file in your theme. - Visit
admin/structure/types/manage/article/display. - Click the image configuration options.
- Watch system.menu.css come back (and wreck your site design if you have different tab styles).
Note
- The bug does not occur with stylesheets that have been removed via
hook_css_alter().
Watch the screencast for a demonstration of the bug in progress:
http://vimeo.com/16672744
Comments
#1
Awesome issue summary, thanks! :)
The proper solution would be to re-open #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files against D7 and introduce the originally proposed stylesheets-remove[] .info file property.
#2
Still no word on that, and I don't think it's going to happen.
Unfortunately this has serious potential to break sites in a bad way.
#3
Here's a fix, but a test for it remains challenging until I (or someone else) makes good on the promise in #561858-201: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework.
#4
@effulgentsia Thank you!
The patch solves the problem, the change appears simple enough and the docs are good! Yay :D
I'd mark it RTBC, but I'm not really clear on what needs to happen with regard to the other issue. It seems like a test for this is not even possible and that issue is closed.
#5
#3: 967166-ajax-css-3.patch queued for re-testing.
#6
Is there any fix to prevent this from happening in Drupal 6?
I'm using TAO which strips out core's css using the .info file. When used with Skinr every time I click on Edit Skin it loads an ajax modal popup. When the modal is displayed it reinjects drupal's css causing my theme to break.
Thanks for any help you can provide. I also tried the stylestripper module, but that didn't work.
#7
Patch works great. Anything else I can help with to get this into a release version?
#8
Looks like a straight up bug fix to me. Any test for this would depend on the other issue. Seems like we should fix the bug now.
#9
D8 first. Let's get these fixed on D8 and into D7.
#10
By the way, #561858-211: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework contains the tests to finish off that issue. Once that's in, a test can be added to this one.
#11
.
#12
#3: 967166-ajax-css-3.patch queued for re-testing.
#13
The last submitted patch, 967166-ajax-css-3.patch, failed testing.
#14
I was just bit by this. The patch in #3 still applies to D7 w/ an offset.
#15
A simple reroll would help move things forward here.
#16
#967166-3: Content rendered via AJAX does not respect stylesheets removed in .info files 967166-ajax-css-3.patch retest.
#17
#3: 967166-ajax-css-3.patch queued for re-testing.
#18
The last submitted patch, 967166-ajax-css-3.patch, failed testing.
#20
Patch attached which applies cleanly to D8 HEAD, and applies to D7 HEAD with an offset.
#21
#22
I applied the patch in #20 and the stylesheets no longer come back.
#23
The patch looks good to me, and manual testing in #22 confirms that it resolves the issue. Thanks for the reroll and the manual testing!
The next thing we will need is an automated test for the issue. #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework has been committed to 8.x, so the comments above seem to indicate it should be possible to write this test now. The test should fail when applied by itself, and pass when applied together with #20.
#24
Oops, marking NW for addition of tests.
#25
#20: 967166-ajax-css-19.patch queued for re-testing.
#26
The last submitted patch, 967166-ajax-css-19.patch, failed testing.
#27
The patch will need to be rerolled against the latested 8.x and for the D8 core/ directory change.
#28
Some changes over in #1287368: Drupal.settings.ajaxPageState.css gets overwritten affected the patch. I've done a re-roll for D8 and D7 and tried not to step on any toes, but please let me know if I missed anything.
#29
Thanks! I'll work on adding tests.
#30
Sending to the bot.
#32
Here's the test only. It should fail. Note that most of the -/+ lines are just a change to indentation.
#33
Here's #32 plus the fix in #28, but with a slight reworking of the if statement and code comments. This should pass.
#34
Tested original problem on the 8.x branch of git and was able to reproduce it following Jacine's steps (tested with system.base.css on bartik for a more drastic effect)
ran the patch
$ git apply -v 967166-ajax-css-32.patch
Followed same reproduction steps and can confirm the css file was not loading.
The faster this gets committed to 8, the sooner it'll get backported and we'll be able to remove the unintuitive hook_css_alter() from our themes in 7 :)
RTBC
#35
#36
The above fixes two small things:
Leaving at RTBC because these are trivial changes. Thanks!
#37
This looks fine, most of the change is indentation and test additions, committed/pushed to 8.x.
#38
d7 reroll
#39
#40
#38 looks good. It's a straight reroll. This patch only fixes this:
+++ b/modules/simpletest/tests/ajax_test.module@@ -32,6 +32,14 @@ function ajax_test_menu() {
/**
+* Implements hook_system_theme_info().
+*/
These two lines need a leading space, which they already have in what was committed to D8.
Trivial change, so RTBC.
#41
Hooray!!! Thanks all :D
#42
Committed and pushed to 7.x. Thanks!
#43
Automatically closed -- issue fixed for 2 weeks with no activity.