Add a scalable weight select element

This is the same issue as #1293930: Weight select elements on taxonomy term pages for large vocabularies kill sites.. However, since we have a patch here, I'll close that one as a duplicate.

Also most of #589440: Reordering fails with more than 31 book pages in a book ended up being about this. Uploading the latest patch from there which I've just rolled back in 8.x so we can do this in one place.

Per webchick's comments in there, broadening scope a bit.

This is a general fields issue then, right?

Edit: or FAPI, really.

So an alternate implementation of this could be something like:

- Pick a number at which select boxes become unwieldy. Let's say it's 100 (not married to this number).
- Add a preprocess (?) function to the weight element in system_elements to dynamically swap the type to textfield, validator to integer if that number of options is exceeded.

Because:

a) I think select boxes are easier to manage from a usability perspective. I can just hit the up/down arrows to quickly get to the number above or behind the one I'm at, without having to type out "5 8".
b) If you grep for '#type' => 'weight', there are many more instances of weight fields in core, and theoretically all of them could have this problem.

Cross-post.

I am not really interested working on this, the weight option doesn't really work above a small number of items anyway. We can definitely swap it out as webchick suggests, I dont think its any more or less usable then - perhaps, more as in not crashing :').

Yeah I don't think this is about usability, it's about not crashing. However whatever we do should be consistent across drag and drop forms rather than one by one.

I'll look into this. Sounds fun!

Attached automatically switches from a select box to a text field when the weight widget is over a certain maximum.

With three terms

With 1000 terms

Sorry, about usability I meant it in the more common case. MOST of the time you do not have 3,000 items. MOST of the time you have like 20. And it's a pain to have to tab to a field, hit backspace twice over 1 9, and then type in 2 0 (5 keystrokes, unless you make typos often like me in which case it could be twice that :)), when I could just tab to the field and press the up button (2 keystrokes).

So I was saying, let's not unconditionally swap the field widget for a text field. Only in the "extreme" case.

Oh, and this is important because this field doesn't only show up in the drag and drop screen, it also shows up on edit pages. And it's perfectly reasonable to want to set your weights as you're initially creating menu items in your node, for example.

Re #14: That's pretty nifty. However, since this a bugfix that will need to be backported, maybe a separate issue to consider the HTML5 element? Edit: Although it looks like browser support for it is still a bit iffy.

Oh, and re: #12: The patch only swaps to a text field after the constant DRUPAL_WEIGHT_SELECT_MAX (I'm open to a better name for it...)

I set the constant to 100 in the patch, which means the largest weight select will range from -100 to 100.

Marked #1354528: Huge memory consumption on term overview page with lots of terms because of weight-dropdown as duplicate of this issue.

This looks good overall, however:

+    // Count the number of digits in the number of options.
+    $digits = floor(log10(abs($element['#delta']) + 1)) + 1;
+    // Add space for one additional digit.
+    $element['#size'] = $digits + 1;

This seems a bit conscientious to me, can't just pick a big number (like the most likely limit in the db?).

Attached just uses a fixed width of 10 chars (max number of digits in a MySQL int).

With 10 terms

With over 100 terms

+++ b/core/modules/system/system.moduleundefined
@@ -46,6 +46,13 @@ define('DRUPAL_OPTIONAL', 1);
+define('DRUPAL_WEIGHT_SELECT_MAX', 100);

Should be const now.

1 days to next Drupal core point release.

Otherwise, looking good!

Rerolled to use const now that that patch is in (thanks @tim.plunkett for pointing this out).

I guess we could add a config variable and use the constant as the default for it, if that's desired. It wouldn't add much overhead, though I'm not sure how much demand there would be for that.

If it is made configurable, please don't provide a UI for it. But I personally think that's overkill. Something either is or is not going to cause performance problems. It's not something that adjusts on a per-site basis (more like on a per-browser/per-machine basis).

Yeah, certainly no UI. I'm also tentatively -1 on the variable in general, but I guess it doesn't really hurt. Maybe someone with 101 dalmatians book pages might care.

Here's what that would look like. I don't set this config variable ever to avoid adding a query that 0.00001% of sites might ever want, but if desired a particular site or contrib module could set a config variable that would overrride the constant.

I'm very -1 on adding args. Is a config variable an okay compromise? One could add code to make the config var -1 for mobile, or something.

In the interest of not ruining someone's day when some client asks for something crazy, I like the idea of a non-exposed variable.

+
+
+  // If the number of options is small enough, use a select field.

One too many line-breaks but I fixed this locally prior to commit.

Having configuration for this seems OK, although currently variable_get() for a variable that realistically never gets set is zero overhead, that might not be the case once CMI hits but we'll have to see.

hook_element_info_alter() ought to allow people to completely swap this out (unless I'm missing something), so I don't think we need anything beyond that.

Committed/pushed to 8.x. Needs the define() change and ideally that whitespace fix as part of the re-roll.

With define(), and with fewer hyperactive linebreaks.

Awesome. Looks good. It's a UI change, but I think the performance trade-off is worth it.

Committed and pushed to 7.x. Thanks!

I looked around in D8, but it seems like the patch in #3 with the book-specific logic got reverted at some point or another. So I think this is now fixed.

Marked #941310: FormAPI select/weight fields do not scale as a duplicate.

Correcting the title since this fixes more than drag-and-drop.

I post it here by error. Please erase it. thnx.

I haven't verified, but it seems this may have broken Context module?

#1439134: Error when saving context - unable to define weight

A change only affecting the taxonomy admin UI could have been backported. This patch, however, changed the generic form_process_weight() function that applies to all weight selectors throughout Drupal.

This change should not have been backported. I'd recommend to roll it back for 7.13.

Also, this issue probably duplicates 4 or 5 others in the core queue that are open since years.

@sun -- Several issues have already been marked duplicate of this one.

Also, this has been in the wild for months now. I think it's a bit late to be rolling it back...

IMHO, the fix was also wrong. Should have changed the tableDrag implementation instead, to not output select widgets in the first place, but output text widgets instead.

Now we have a totally weird element #type, because #type weight == weight selector is no longer true. Whether you get one depends on an arbitrary magic number that's defined in a constant somewhere, and whether your number of items happens to exceed that or not...

It's still a performance issue even if you're not using tabledrag when your select box has 100,000 options in it.... The arbitrary magic number is indeed arbitrary, but I did ask for feedback on that point, and it can be overridden.

No, I think what sun's saying is that instead of making this count($options) > 100 : 'textfield' : 'select' decision in the weight widget itself, to make it in the tabledrag.js when it renders things out.

I'd agree with that. Having 100 selects with 100 options in each is not fun to load in any browser.

Why do we keep the select around anyway? it doesn't reorder when you change the weight with select.

In any case making drupal output textfield would be best, the rest can be dealt with in JS.

@webchick: Sorry, my two sentences in #42 were not related; they should have been separate paragraphs.

As webchick pointed out in one of the many duplicates of this issue, a select box is nicer UX when you are manually selecting from a small number of values. That's why we added a constant where it "switches." It's a magic number, yes, but in a defined constant with a variable override, so I don't think it's a great sin. Certainly not worth an open major bug sitting around.

I don't actually see what the issue has to do with tabledrag.js at all. Some of these weight selects are on tabledrag forms (like the taxonomy admin form) and others are not (like the book fieldset on the node form). The base problem is the same regardless.

The only reason I think there is to keep this at needs work is if there is a functional bug with the current behavior. Otherwise, let's open a followup and we can bikeshed magic numbers until the cows come home. ;)

I think sun's point is that while the drop-down at http://drupal.org/node/add/project-issue is to some extent a performance issue, it's not remotely one in relation to combining 50 of them on one page at the same time, as in a tabledrag situation. And one-off selects like the Monster Project Selector™ don't actually hit performance issues until you're in the 10s of 1000s of elements. But in a tabledrag (or book admin screen) situation, 100 or so is probably an appropriate limit.

So a solution that targets the source of the problem here—lots of big select fields rendered at once, not just big select fields as a general concept—seems like a good thing to at least explore. Whether here or in another issue, I'm relatively ambivalent.

Actually performance is hit much faster than that. One of the slowest page (it made it into the top ten) in Drupal core is admin/config/regional/language/add, I was surprised until I realized that there is a select with 185 options on this page.

the 10s of 1000 is the "I will make your browser burn and crash" threshold, less than that is the "I will make you page twice as slow to load" threshold.

For D8 the problem could be solved by #1174640: Add new HTML5 FAPI element: number, it's this kind of thing the input type is made for.

So maybe we need an additional API addition rather than a config variable, something like a #max_select_items property for the element? (That would make the config variable redundant, although I don't think it makes sense to back it out at this point.) Tabledrag could turn it off entirely so a textfield is always used, and we can leave 100 as a sensible default, and whoever wants can override it on a per-field basis if so inclined.

Then in D8 we can use the number element.

So can we close this again and open a normal task as a followup? Unless someone can confirm a functional bug. I don't think we're going to be rolling this back now; it has been out long enough that any impacted contrib is already adjusting to it.

That sounds like a good approach to me!

Opened #1518182: Improve #weight select element behavior based on the number of items.

Any chance that this can get accepted for D6?

I kind of backported it to an existing site. The things I had to change w.r.t. the D7 patch:
- element_validate_integer does not exist. However, an _element_validate_integer exists in CCK and its code can be used by copying it to form.inc and renaming it to element_validate_integer.
- element_info is named _element_info in D6.

A quick test seems to show that it works.

I also noticed a decrease in memory_peak_usage from 95.5M to 42.8M on admin/build/block with some 450 blocks! Thus this is not only about browser performance or network delays.

I will post a patch for review if there's a chance it may be accepted.

Good point.

I copied element_validate_integer from D7 and put it near the end of form.inc but still within the "defgroup form_api", exactly as in D7. Though it does introduce a new UI string, chances are that it already has been translated as this string is translated as part of CCK.

Oh, and my editor did remove some white space in form.inc. you can ignore this when reviewing.

update img

Moving this back to 7.x as fixed.

Hi!

Hope core developers will notice this.
Thou this one is closed - I have a bit of additional info, maybe it will be useful.

My current problem raised in D6 under PHP between 5.4 and 5.6 (about PHP a bit later, real problem is in it).
I have a node with node-reference field, with almost 600 referenced nodes, with obviously weight "control".
And node edit page gives "memory exhausted" while 256 Mb of RAM is allowed on server.
So I have made some profiling and found that...

On generation of every option of weight select box check_plain() is run 2 times.

Here it is in includes/form.inc:1516

function form_select_options($element, $choices = NULL) { 
...
  foreach ($choices as $key => $choice) {
...
      $options .= '<option value="'. check_plain($key) .'"'. $selected .'>'. check_plain($choice) .'</option>';
...
  }
...
}

And check_plain() have htmlspecialchars() inside, which ... !!! ... does not free memory until page load ends.
I have looked into sources of PHP 5.6 to findout this.

With a bit of calculation we will have:

600 items in node_reference
*
doubling run of check_plain() on each item
*
1200 weight options in each weight select box
(to explain this in small numbers: 10 field references will produce 10 weight options for negative weights and 10 for positive)
-------------------------
= 1440000 runs of htmlspecialchars() without cleaning memory after each run...

Which in my case for real was a bit more than 1300000, because I was having a bit less than 600 references, but anyway...

So for quick solution I`m making dirty patch specially for my project:

      if ($element['#array_parents'][0] == 'field_sub_tools' && $element['#array_parents'][2] == '_weight') {
        $options .= '<option value="'. $key .'"'. $selected .'>'. $choice .'</option>';
      }
      else {
        $options .= '<option value="'. check_plain($key) .'"'. $selected .'>'. check_plain($choice) .'</option>';
      }  

But I suppose somthing global must be mage about this problem.

Before this patch only htmlspecialchars() was taking about 60Mb, after this patch whole form_select_options() is taking about 20Mb while loading that node edit page.

I understand that form_select_options() is used for every select box in drupal and check_plain() is ok, but do we really need check_plain() to generate weight select boxes?