vanishing label text from manage fields page

Definitely a usability annoyance.

I found the cause and have a remedy...

    $(this).bind('change keyup', function (e){//, updateText) {
      //var updateText = (typeof updateText == 'undefined' ? true : updateText);
      var selectedField = this.options[this.selectedIndex].value;
      var selectedFieldType = (selectedField in fields ? fields[selectedField].type : null);
      var selectedFieldWidget = (selectedField in fields ? fields[selectedField].widget : null);
      var options = (selectedFieldType && (selectedFieldType in widgetTypes) ? widgetTypes[selectedFieldType] : []);
      this.targetSelect.fieldPopulateOptions(options, selectedFieldWidget);

      //if (updateText) {
      //  $(this.targetTextfield).attr('value', (selectedField in fields ? fields[selectedField].label : ''));
      //}
    });

This is the adpated code to make it work.
What went wrong:


$(this.targetTextfield).attr('value', (selectedField in fields ? fields[selectedField].label : ''));


Sets the value of the text field to the label of fields[selectedField]. The label is always empty. I don't know why... I also don't know why we want to update the textfield in the first place.
I put this to need review for the code snippet I posted in here. I'll make a patch if we don't need to update the text.

Great aspilicious! What file is this in? I'm having a little trouble following what you changed-- your fix is to not update the textfield ever? (That seems fine to me; at any rate better than what we are doing now.)

That means the entire block of JavaScript there can be dropped?

Thanks for looking into this.

this code is in modules/field_ui/field_ui.js or something like that...

I'm not sure if I can delete all of it cause it also updates the widget list wich we need...

It is actually template_preprocess_field_ui_field_overview_form() trying to fetch the label (and widget) from an instance that doesn't exist.

This is a review of aspilicious' patch in #5, and it works.

That's this file:
http://drupal.org/files/issues/field_ui_js.patch

I don't follow what #7 tried to do. But i have tried and tested #5 and it works beautifully.

#5 is just wrong. No time to fix patch of #7 now though. Maybe in the weekend.

Casey's patch fixes a definite bug, and i'd be willing to RTBC. I think we can fix the usability problem initially raised here at the same time, though.

Before casey's patch in #11, type in anything for a label for an existing field, and it would be blanked out when selecting the field. Now, whether we've typed anything for the existing field label or not, a label for the existing field is fetched and put in the text box for the label. This is great if we haven't typed anything yet.

However, if we have typed a label for an existing field we're about to add, say "Example label", and then we select "Text: field_existingtext (Existing text)", the "Example label" we just typed is wiped out by "Existing text". This leaves the user to wonder why the box was available at all.

Two solutions:
1. If there is any typing in the label textfield, do not overwrite it (my preference).
2. Do not allow typing in the label textfield until after a Field to share is selected.

If anyone thinks the bug that the patch fixes should be committed first, and then this usability issue dealt with, please go ahead and mark RTBC because the patch is tested and approved to fix the never-fetching-label bug.

One question on the code– is there any reason not to only use the code from the else statement? It seems to cover all cases (lightly tested) and looks to be as fast:

    $field = field_info_field($field_name);
    $instance = field_info_instance($form['#entity_type'], $field_name, $form['#bundle']);
    $existing_field_entity_types = array_keys($field['bundles']);
    $existing_field_entity_type = array_pop($existing_field_entity_types);
    // Get the last existing instance of the last available entity type.
    $instance = field_info_instance($existing_field_entity_type, $field_name, array_pop($field['bundles'][$existing_field_entity_type]));
    $js_fields[$field_name] = array('label' => $instance['label'], 'type' => $field['type'], 'widget' => $instance['widget']['type']);

In any case, great work on figuring out that the first block of code would not cover cases of different entity types!

Thanks.

@Benjamin Melançon, you are right about the else statement.

Also included your proposed solution 1.

Wonderful! Well, i for one am very happy. For real, casey, i think you just saved about a hundred bug reports when D7 is released. Tested and (once cache is cleared, heh), much approved.

Note: Firebug feels the need to repeatedly note "The 'charCode' property of a keyup event should not be used. The value is meaningless." when Field to share is tabbed to from either direction, but that is not caused by this patch and appears to be a harmless, if stupid, Firefox quirk.

#13: fields_ui-existinglabel.patch queued for re-testing.

There are two things here :

- an actual bug : the list of labels for the JS autopopulate is built populated correctly
That is fixed by #684310: When adding existing field "Label" is cleared when exiting field, which is pending reviews, and which is the direction I'd rather go with (because I (re)wrote the patch)

- a UX improvement : if the user started entering a label, it gets overwritten by the label of the existing field (that is, if the code was fixed with #684310: When adding existing field "Label" is cleared when exiting field...)
I'm fine with a solution to prevent this and only populate 'Label' if it has no user input.
Note that filling the existing label is the behavior in CCK D6, and don't think I remember anyone actually complaining (not to say that it shouldn't be enhanced, of course)

Could we get #684310: When adding existing field "Label" is cleared when exiting field in and only keep the hasManualExistingFieldLabel part here ?

As of D7 beta 2, the problem of the user-typed label being cleared occurs in two instances:

1. When tabbing from Label to the Field to share when the - Select an existing field - is first-- that is, the default case (using the mouse to select something else in the drop-down does not trigger this unwanted clearing).
2. When, for whatever reason, returning to the -Select an existing field - option with the mouse.

#13: fields_ui-existinglabel.patch queued for re-testing.

Don't seem to have this bug with the latest 8.x and 7.x