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_

Files: 
CommentFileSizeAuthor
#14 states_for_type_item_broken-1589176-14.patch1.01 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,774 pass(es).
[ View ]
#7 core-js-data-states-1589176-7.patch2.63 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 58,930 pass(es).
[ View ]
core-js-data-states.patch2.37 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 36,601 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+JavaScript

forgot tagging

Title:Use data-* attribute to store #states informationsUse data-* to store #states api informations

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

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

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 ;)

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

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).

Priority:Normal» Major
Status:Active» Needs review
StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,930 pass(es).
[ View ]

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

Category:task» bug
Priority:Major» Normal

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

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.

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.

Status:Needs review» Reviewed & tested by the community

Okay fine, if this is a standard.

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.

Status:Closed (fixed)» Needs review
Issue tags:+quickfix, +sprint, +Spark
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 59,774 pass(es).
[ View ]

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.

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).

Title:Use data-* to store #states api informationsFollow-up: Use data-* to store #states api informations

Issue summary:View changes

blub

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

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Status:Fixed» Closed (fixed)

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