Disable widget select box if there's only one selection

All that needs to be done here is something like this (pseudo-code):

if ($('option:visible', element).size() == 1) {
  $(element).hide()
}

The solution should probably be JavaScript-only.

Left a little bit of debug code in and now it works for existing fields as well

IMHO, the testbot is incorrectly failing this patch.
The file is JS. It shouldn't be tested for PHP syntax.
See this issue: http://drupal.org/node/608044

Here's how I tested this patch:
- Applied patch - success
- Code review of patch - looking good. Could use a comment.
- Manual testing:
. create new content type "test"
. added boolean field (shows dropdown) - success
. added image field (doesn't show) - success
. created new content of type "test" and uploaded image - success

Looks good. Ship it!

I'm not (yet) convinced that should be worked around using Javascript. It doesn't optimize the behavior of people who don't have Javascript.

re Dries #7:
People without JS are out of this anyway, because they will always get a 'Select widget' dropbox filled with *all* available widgets for all field types (selecting an invalid 'field type / widget' pair then comes back with a form error). Restricting the list of widgets to the ones available for the selected field type is already done through JS (obviously).

Regarding patch in #4, I don't think I see why the two .end() calls are needed, but I might be missing it.
Also, is *hiding* the select really what we want ? Simply disabling it might be less confusing.

Also : It would be best to commit #578544: field creation widgets in different columns from existing fields first. This fixes a trivial WTF on colspans which should make this patch clearer (there's actually no need to mess with the 'operations' column here).

Thus, back to CNW

#578544: field creation widgets in different columns from existing fields has been committed.

Reworked patch from #4. Please review.
Changes:
- Got rid of code that messes with the operations column as suggested in #10.
- Instead of simply hiding select lists that hold only one option, it shows their only option as text (see attached jpg). Note: I tried to simply use the disabled attribute, but it shows no visual difference in my (mac) browsers (FF 3.5, Safari 4).
- Added some comments.

I'm a bit concerned though about the copy/paste nature of this code. Is it acceptable here? I would gladly introduce a new function to avoid copy/paste programming.

"Text only" doesn't look too great, it appears misaligned, and I'm not sure we can find a set of CSS margins that can make it look aligned in all browsers.
It's a shame OSX doesn't make any visual distinction between enabled and disabled selects - win does ;-)

Dunno, maybe a disabled textfield, then ? Might require some work in field.js - so maybe this should be validated first with a hacked sreenshot by modifying the HTML in firebug...

Also, any solution we come up with, we'll need to make sure the form submit callback handles the case where it receives no value for the widget.

Ok, found what causes this weird behavior: if you change style of a select list then OS X browsers will use this new style even for disabled elements. So simply changing color to gray helps a lot. See:

• Safari before: http://skitch.com/zserno/ne3a3/34ewrwef-localhost-1
• Safari after: http://skitch.com/zserno/ne32g/34ewrwef-localhost
• FF before: http://skitch.com/zserno/ne3a4/34ewrwef-localhost
• FF after: http://skitch.com/zserno/ne32u/34ewrwef-localhost

I re-rolled the patch to use the disabled attribute on widget select boxes that hold only one option. Please try. Although I'm not sure if this problem appears only on this form thus changes made to field_ui.css might need to be moved to a higher level.

Cool ! Approach looks fine, now code review:

+++ modules/field_ui/field_ui.css	24 Nov 2009 15:01:45 -0000
@@ -16,3 +16,7 @@
+  color: GrayText;

Hmm, I think we use real #rgb in core.

+++ modules/field_ui/field_ui.js	24 Nov 2009 15:01:46 -0000
@@ -24,7 +24,14 @@ function attachUpdateSelects(context) {
+      // Don't show widget select box if there's only one selection.
+      if (this.targetSelect.fieldPopulateOptions(options).find('option').size() > 1) {
+        this.targetSelect.removeAttr('disabled');
+      }
+      else {
+        this.targetSelect.attr('disabled', 'disabled');
+      }

2 occurrences - I think this could be moved inside fieldPopulateOptions().
+ comment is not to date with the current approach (disable, not "don't show")

+++ modules/field_ui/field_ui.js	24 Nov 2009 15:01:46 -0000
@@ -48,12 +55,26 @@ function attachUpdateSelects(context) {
+  // Make sure that disabled select box values get submitted too.
+  $('#field-ui-field-overview-form', context).submit(function () {
+    $('#field-overview select[disabled]').removeAttr('disabled');
+    return true;
+  });

Sounds like a hack.
+ Won't this cause a visual blip between the moment you hit Submit and the moment the new page loads ?

I'd rather have the case where there's no 'widget type' in the incoming form values dealt with in field_ui's form handlers.

Something like :

if (!isset($form_state['values']['_add_new_field']['widget_type']) {
  $widget_type = the only valid widget type;
  form_set_value($form['_add_new_field']['widget_type'], $widget_type);
}

in _field_ui_field_overview_form_validate_add_new() - and similar in _field_ui_field_overview_form_validate_add_existing()

I'm on crack. Are you, too?

@yched: Thank you for your thorough review, I'll re-roll the patch according to your remarks.

Rerolled the patch according to yched's remarks in #16.
Please review and test it.

+++ modules/field_ui/field_ui.admin.inc	28 Nov 2009 10:33:02 -0000
@@ -402,13 +402,20 @@ function _field_ui_field_overview_form_v
+      // If there's only one possible widget type, use that.

Please make it clear that it relates to the JS feature of disabling the select if there is only one valid widget type.

+++ modules/field_ui/field_ui.js	28 Nov 2009 10:33:02 -0000
@@ -59,9 +59,18 @@ function attachUpdateSelects(context) {
+    var counter=0;
+
+    for (i in options) {
+      counter++;
+    }

Hm - can't we do better than this ? Why doesn't options.length work ?
+ the code comment "Disable widget select box if selection is obvious" doesn't really apply to the (already in HEAD) case where there is 0 option to select because no field type has been selected yet.

Other than that, code looks good to me. We'd need a corresponding test in Field UI, though.

This review is powered by Dreditor.

Rerolled patch from #18 according to @yched's review.

Hm - can't we do better than this ? Why doesn't options.length work ?

Unfortunately .length property is 'undefined', so we can't use that. I spent quite some time on researching this question and this was the best solution I found. Now I'm really curious if there's any better way to do this. :)

About the corresponding test: could you please give me some hints about what this test should check?

Fixed diff.

zserno, I got an error message when I applied your patch.

asrob@alpaca [~/public_html/dev]# patch -p0 < 601952_dont_show_widget_select_box_22.patch 
patching file modules/field_ui/field_ui.admin.inc
Hunk #1 succeeded at 411 (offset 9 lines).
patching file modules/field_ui/field_ui.css
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file modules/field_ui/field_ui.css.rej
patching file modules/field_ui/field_ui.js

Thanks asrob!
Attached is an updated patch, please test it.

I reproduced again and it works. Thanks zserno.

Sorry, it works but needs more tests.

Hi, I have this feature request on Drupal6/Ubercart2 with selectbox of an attribute with only one option.
Will this method also work for that?

Greetings,
Martijn

Hi, I'm pretty new to this, but is it good if I try to write a test so this patch can get applied? I'm not sure if that's what asrob meant.

@Summit I doubt it. That patch will not work with D6 for sure. However you can adjust it following the ideas behind it.

@FreekyMage That would be fantastic. Here is yched's comment about the test from #19:

We'd need a corresponding test in Field UI, though.

I tested again but it seems to me that does not work.

patching file modules/field_ui/field_ui.admin.inc
Hunk #1 FAILED at 539.
Hunk #2 FAILED at 584.
2 out of 2 hunks FAILED -- saving rejects to file modules/field_ui/field_ui.admin.inc.rej
patching file modules/field_ui/field_ui.css
Hunk #1 FAILED at 15.
1 out of 1 hunk FAILED -- saving rejects to file modules/field_ui/field_ui.css.rej
patching file modules/field_ui/field_ui.js
Hunk #1 succeeded at 69 with fuzz 1 (offset 10 lines).

Patch in #25 no longer applies. Partial re-roll needs testing because I'm not sure if it's all there anymore.

#34: 601952_dont_show_widget_select_box_34.patch queued for re-testing.

Shouldn't this be moved to Drupal 8?

Re-rolled, with trailing whitespace correction.

- Fixed flawed logic in _field_ui_field_overview_form_validate_add_new() (the $widget_types variable was tested in an "if" condiftion but defined only within the conditional branch)
- Added similar logic in the validate function for the "add existing field" row, which needed the same treatment
- Reworded comments a bit.

I'm not sure about the field_ui-specific CSS rule to explicitly grey out disabled select boxes. If some browsers need it, then this sounds like something that belongs to drupal's generic CSS, not just field_ui.

Adjusting title for the current approach

issue summary

Created issue summary

original report by webchick

Where is the image?

Rerolled after the /core move.

+ @Bojhan : screenshot attached. The select in the 'widget' column is disabled if there is only one choice for the field type.

I am not really convinced this is a good improvement, although it sucks to have a one option dropdown. It sucks even more if people are confused whether something is wrong or not, because it looks disabled..

#45 feels like a won't fix.

inlined the image