Updated: Comment 0

Problem/Motivation

drupalSettings.states never clears, it's very noticeable on the views UI where #states is used all over the place. It gets slower and slower to open modals. Because all the rules just adds up in drupalSettings.states (It's easy to arrive to 120 rules after opening a few modals). and they all get processed every time even if the elements don't exist on the page.

Also, very slow. Event delegation on the views UI modal reduce the time it takes to open a modal by 1 second (!) on desktop (from 3.8s to 2.9s).

Proposed resolution

This patch move the #states configuration away from Drupal.settings and into a data-drupal-states attribute on the relevant form element (it's data-drupal-states because data-states sounds like it'd be an easy naming confilct).

I had to move the drupal_process_states() call before drupal_render to add the attributes from this function.

The JS change is really straightforward, instead of getting the values from Drupal.settings it takes it from all the elements having a data-drupal-states attribute. The #states configuration is serialized in json inside the attribute. It's legit from a HTML standpoint, anything goes inside data-* attributes.

Remaining tasks

User interface changes

API changes

Original report by @nod_

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Status: Active » Needs review
Issue tags: +JavaScript

forgot tagging

nod_’s picture

Title: Use data-* attribute to store #states informations » Use data-* to store #states api informations
nod_’s picture

Status: Needs review » Closed (won't fix)

1 year and no feedback, probably not such a good idea :p

klonos’s picture

Version: 8.x-dev » 9.x-dev
Status: Closed (won't fix) » Active

...or perhaps not enough time. Still a good idea though (since I see no negative comments stating the opposite). Let's see if we can do it in D9 then ;)

nod_’s picture

It's easy enough to put in 8.x it's just that I see no justification for it really.

nod_’s picture

Version: 9.x-dev » 8.x-dev
Category: feature » task

Now I see it.

drupalSettings.states never clears, it's very noticeable on the views UI where #states is used all over the place. It gets slower and slower to open modals. Because all the rules just adds up in drupalSettings.states (It's easy to arrive to 120 rules after opening a few modals). and they all get processed every time even if the elements don't exist on the page.

Also, very slow. Event delegation on the views UI modal reduce the time it takes to open a modal by 1 second (!) on desktop (from 3.8s to 2.9s).

nod_’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
2.63 KB

There is also the problem that the script works kind of backwards, but we can't fix everything at once.

The following patches cut down 800ms from init of Views UI modals #1851414: Convert Views to use the abstracted dialog modal

nod_’s picture

Category: task » bug
Priority: Major » Normal

actually having leftover settings from elements that have been removed from the page in drupalSettings is a bug.

dawehner’s picture

Issue tags: +VDC

Manual testing of some bits of the views UI still works and I have to admin that it feels way smoother, a little bit more like D7 to be honest.

+++ b/core/misc/states.js
@@ -18,17 +18,17 @@ var states = Drupal.states = {
+    for (var i = 0, il = $states.length; i < il; i += 1) {

This variable name really seems odd. What about using states_length instead?

Updated the issue summary.

nod_’s picture

the variable name is used all over the place in the rest of the scripts so I don't have anything against changing it but it'll be the odd one out.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay fine, if this is a standard.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Wim Leers’s picture

Status: Closed (fixed) » Needs review
Issue tags: +quickfix, +sprint, +Spark
FileSize
1.01 KB

This broke #states for #type => item!

Before this commit, unchecking admin/config/content/formats/manage/basic_html's "Enable image uploads" checkbox correctly hid the "Maximum dimensions" (which is #type => item) form item, but since this commit, that's broken.

The reason is that for #type => item elements, #attributes aren't used (because there's only a wrapper and not an actual HTML form input element), which is precisely what's necessary since this patch for #states to work.

However, #wrapper_attributes does always get set, and in this case that's in fact more appropriate (i.e. only a wrapper, no actual HTML form input element on which #attributes could get set). Patch attached.

Ideally, we'd have test coverage that would've prevented this regression, but that's something sun should've done years ago, when this exact same bug was first encountered at #783438: #states doesn't work for #type item. In fact, AFAICT we have zero test coverage for #states non-JS side of things, as demonstrated by the committed patch. So there are no tests I can extend.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Tested and it works. Not pretty but that's just how it is.

this makes the code for #2033959: Improve design of language detection and selection settings page a bit cleaner (which is why i know it works).

nod_’s picture

Title: Use data-* to store #states api informations » Follow-up: Use data-* to store #states api informations
nod_’s picture

Issue summary: View changes

blub

webchick’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.