Drupal.settings.ajaxPageState.css gets overwritten

Possibly related:

#914792: Custom element properties entirely override default element info properties
#791860: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead.
#208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice

subscribe

Wow, @TwoD, thanks for the detailed report *and* solution!

subscribe

Excellent report and the patch looks sound. Let's get a couple manual testers confirming the patch resolves the issue and does not have unintended side effects.

Attached is a reroll for core/.

I wonder whether we can assert this in a unit test, too? Doesn't have to be a full AJAX interaction, testing the internal functions without any page loads should be fine.

I have performed steps 1-7 as described in the issue
After performing step 4:
:
In original state (patch not applied)

After pressing 'Save' or 'Cancel' value of Drupal.settings.ajaxPageState.css is cleared
Patch fixes the issue

Just one tiny remark on steps to reproduce on D8:
1. Open 'display fields' screen directly, not in overlay (URL should be '/admin/structure/types/manage/article/display',
not '/#overlay=admin/structure/types/manage/article/display'. In the latter case Drupal.settings.ajaxPageState.css contains correct values both with and without patch.

my deep respect to the patch author for such a thorough description!

Patch in #7 works nicely with #356480-68: Lazy-load editors to fix CKeditor in panels in my Drupal 7.10 site.

Same here #7 patch works.
core 7.9
We ran into the same issue with forms in a modal window via a ctools content plugin.
The wysiwyg issue was fixed by http://drupal.org/node/356480
The issue, that attached CSS files were not loaded is fixed with the above mentioned patch.

Nice work, thanks!

I have modified ajax.test (AJAXFormValuesTestCase class) to include simple checks either AJAX submit returns CSS/JS in case it was not changed by callback (empty CSS array is the bug's reason)

+++ b/core/includes/common.inc
--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc

Your local git checkout is outdated and you diffed against origin/8.x. Either diff against 8.x, or make sure your local checkout is current.

+++ b/core/modules/simpletest/tests/ajax.test
@@ -376,6 +376,10 @@ class AJAXFormValuesTestCase extends AJAXTestCase {
+      // Since callback does not add CSS and JS, ajaxPageState must not contain CSS and JS arrays

Exceeds 80 chars. The comment could also be more clear about what exactly is being verified here.

+++ b/core/modules/simpletest/tests/ajax.test
@@ -376,6 +376,10 @@ class AJAXFormValuesTestCase extends AJAXTestCase {
+      // @see http://drupal.org/node/1287368

Can be removed.

+++ b/core/modules/simpletest/tests/ajax.test
@@ -376,6 +376,10 @@ class AJAXFormValuesTestCase extends AJAXTestCase {
+      $this->assertFalse(key_exists('css', $commands[0]['settings']['ajaxPageState']), 'AJAX form state must not contain CSS if it was not changed by callback');

There is no function key_exists(); there is array_key_exists() only.

We should also test the second expectation that these are objects and not arrays somewhere.

14 days to next Drupal core point release.

Rerolled patch against latest 8.x.
I can't find better description of the added checks, any ideas?

By the way, I have a feeling that there is a bug in ajax.test:
several AJAX tests assume that consequent calls to drupalGetAjax()/drupalPostAjax() change return value of drupalGetSettings(), while what happens is that drupalGetSettings() remembers result returned by the first request, and does not update it during subsequent calls.
Have spent some time to find that.
Or am I missing something?

Um, isn't #7 already contained in the patch in #18? :)

#18: test of CS and/or JS added by AJAX callback is already done by AJAXFrameworkTestCase::testLazyLoad() (ajax.test).
The missing case is when AJAX callback does not add anything.

May I mark this RTBC?

Thanks! This looks good to me as well.

Mainly nitpicks:

+  // Cast the Array to an Object

Arrays and objects aren't proper nouns, should be lower cased.

+    // Verify a settings command does not add css

However CSS should be all upper-cased because it's an acronym.

Quick re-roll?

+++ b/core/modules/simpletest/tests/ajax.test
@@ -135,13 +147,34 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
     // Verify that the base page doesn't have the settings and files that are to
-    // be lazy loaded as part of the next request.
+    // be lazy loaded as part of the last request.

The change to "last" is bogus here; the files are still expected to be lazy-loaded as part of the next requests.

Additionally fixed that, back to RTBC.

Thanks, great detailed report to.

Last thing, is there an issue open for this already?

+    // @todo D8: Add a drupal_css_defaults() helper function.

If not, can we open one?

Looks much better otherwise, so will commit once that's linked from this issue.

There we go: #1392874: Add a drupal_css_defaults() helper function like drupal_js_defaults()

We can probably take the @todo out then, right?

Nope, that's fine, as long as it's valid.

Thanks! Committed/pushed to 8.x, moving to 7.x for backport.

git cherry-pick 8cc5ede
git format-patch -1

I came here from this wysiwyg issue - lazy load editors. I applied the patch (#40), cleared cache, and it works!

Am I correct that #40 has *not* been included in D7.12?
http://drupalcode.org/project/drupal.git/blob/7bb06635ee0fbb36d5e4b24368...

yeah, unfortunately not.

Anything we can do to improve the odds that it gets into the next release?

@pdcarto -- The patch has already been committed to Drupal 8, the Drupal 7 backport is marked RTBC, and the next release of Drupal 7 won't be for another month. Therefore, in all likelihood, this patch will be included in the next release, unless something else comes up. (7.12 already included a lot of changes, which is why this and a handful of other issues didn't make it in time.)

A way to help might be to add issue summaries to this and other RTBC issues, which can help committers review the patches more easily so that they have time for more issues. :)

If you'd like to see an issue fixed, please always make sure to help the maintainers and state:

1. whether you have applied the patch
2. which patch from which issue #comment you've applied
3. whether the patch applied cleanly
4. whether the patch resolved the issue
5. whether the patch resulted in any odd behavior that wasn't there before
6. and if you can, whether you have reviewed the patch and whether the changes look clean and solid to you, and check whether someone unfamiliar with this issue will be able to understand from the code and comments why the new/patched code does what it does.

Cool, thanks very much for the thorough bug report and test in this issue. I'm really sorry it didn't make it into 7.12, but hopefully #1423530: Help project maintainers manage RTBC queue on a first-in, first-out basis gets done; that'd certainly help with this kind of thing in the future.

Committed and pushed to 7.x. Thanks!

Just ran into this issue as well.
I'm glad to see it reported, fixed and committed instead of having to write a report myself :)

Nice job TwoD!

i would like to mention that after this update on drupal 7.14 having installed hierarchical select. After i make the selection of my hierarchical drop down, when an image is uploaded the style of the image field disappers. (see attached file). Wheni replace the code with the one on drupal 7.12 everything works fine!

i restored only the following code and everything worked fine again

diff --git a/includes/common.inc b/includes/common.inc
index 268e36b..37c3295 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -2955,8 +2955,13 @@ function drupal_get_css($css = NULL, $skip_alter = FALSE) {

// Provide the page with information about the individual CSS files used,
// information not otherwise available when CSS aggregation is enabled.
- $setting['ajaxPageState']['css'] = array_fill_keys(array_keys($css), 1);
- $styles['#attached']['js'][] = array('type' => 'setting', 'data' => $setting);
+ // Skip if no files were added to the page or jQuery.extend() will overwrite
+ // the Drupal.settings.ajaxPageState.css Object with an empty Array.
+ // Cast the Array to an Object to be on the safe side even if not empty.
+ if (!empty($css)) {
+ $setting['ajaxPageState']['css'] = (object) array_fill_keys(array_keys($css), 1);
+ $styles['#attached']['js'][] = array('type' => 'setting', 'data' => $setting);
+ }

return drupal_render($styles);
}

@pmos That code does nothing new except check if the $css variable is empty and, if not, casting the created array to an object to prevent the issues summarized in the original post.

I suggest you open a new ticket in the Hierarchical Select queue and refer to this one in it.

Updated to reflect last patch.