Bug 463598: Improve the performance of the JavaScript that adjusts field values based on the value of another field

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=wicked, a=mkanat

7 files changed, 116 insertions, 73 deletions

diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index fe5c7bdcb..f23b0c46d 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -199,7 +199,7 @@ sub _check_if_controller {
     my $self = shift;
     my $vis_fields = $self->controls_visibility_of_fields;
     my $values     = $self->controlled_values;
-    if (@$vis_fields || @$values) {
+    if (@$vis_fields || scalar(keys %$values)) {
         ThrowUserError('fieldvalue_is_controller',
             { value => $self, fields => [map($_->name, @$vis_fields)],
               vals => $values });
@@ -287,13 +287,13 @@ sub controlled_values {
     my $self = shift;
     return $self->{controlled_values} if defined $self->{controlled_values};
     my $fields = $self->field->controls_values_of;
-    my @controlled_values;
+    my %controlled_values;
     foreach my $field (@$fields) {
-        my $controlled = Bugzilla::Field::Choice->type($field)
-                         ->match({ visibility_value_id => $self->id });
-        push(@controlled_values, @$controlled);
+        $controlled_values{$field->name} = 
+            Bugzilla::Field::Choice->type($field)
+            ->match({ visibility_value_id => $self->id });
     }
-    $self->{controlled_values} = \@controlled_values;
+    $self->{controlled_values} = \%controlled_values;
     return $self->{controlled_values};
 }
 
@@ -431,4 +431,14 @@ The key that determines the sort order of this item.
 
 The L<Bugzilla::Field> object that this field value belongs to.
 
+=item C<controlled_values>
+
+Tells you which values in B<other> fields appear (become visible) when this
+value is set in its field.
+
+Returns a hashref of arrayrefs. The hash keys are the names of fields,
+and the values are arrays of C<Bugzilla::Field::Choice> objects,
+representing values that this value controls the visibility of, for
+that field.
+
 =back
diff --git a/js/field.js b/js/field.js
index 629fb8a23..700c1de8d 100644
--- a/js/field.js
+++ b/js/field.js
@@ -389,40 +389,52 @@ function handleVisControllerValueChange(e, args) {
     }
 }
 
-function showValueWhen(controlled_field_id, controlled_value, 
-                       controller_field_id, controller_value)
+function showValueWhen(controlled_field_id, controlled_value_ids, 
+                       controller_field_id, controller_value_id)
 {
     var controller_field = document.getElementById(controller_field_id);
     // Note that we don't get an object for the controlled field here, 
     // because it might not yet exist in the DOM. We just pass along its id.
     YAHOO.util.Event.addListener(controller_field, 'change',
-        handleValControllerChange, [controlled_field_id, controlled_value,
-                                    controller_field, controller_value]);
+        handleValControllerChange, [controlled_field_id, controlled_value_ids,
+                                    controller_field, controller_value_id]);
 }
 
 function handleValControllerChange(e, args) {
     var controlled_field = document.getElementById(args[0]);
-    var controlled_value = args[1];
+    var controlled_value_ids = args[1];
     var controller_field = args[2];
-    var controller_value = args[3];
-
-    var item = getPossiblyHiddenOption(controlled_field, controlled_value);
-    if (bz_valueSelected(controller_field, controller_value)) {
-        showOptionInIE(item, controlled_field);
-        YAHOO.util.Dom.removeClass(item, 'bz_hidden_option');
-        item.disabled = false;
-    }
-    else if (!item.disabled) {
-        YAHOO.util.Dom.addClass(item, 'bz_hidden_option');
-        if (item.selected) {
-            item.selected = false;
-            bz_fireEvent(controlled_field, 'change');
+    var controller_value_id = args[3];
+
+    var controller_item = document.getElementById(
+        _value_id(controller_field.id, controller_value_id));
+
+    for (var i = 0; i < controlled_value_ids.length; i++) {
+        var item = getPossiblyHiddenOption(controlled_field,
+                                           controlled_value_ids[i]);
+        if (item.disabled && controller_item && controller_item.selected) {
+            item = showOptionInIE(item, controlled_field);
+            YAHOO.util.Dom.removeClass(item, 'bz_hidden_option');
+            item.disabled = false;
+        }
+        else if (!item.disabled) {
+            YAHOO.util.Dom.addClass(item, 'bz_hidden_option');
+            if (item.selected) {
+                item.selected = false;
+                bz_fireEvent(controlled_field, 'change');
+            }
+            item.disabled = true;
+            hideOptionInIE(item, controlled_field);
         }
-        item.disabled = true;
-        hideOptionInIE(item, controlled_field);
     }
 }
 
+// A convenience function to generate the "id" tag of an <option>
+// based on the numeric id that Bugzilla uses for that value.
+function _value_id(field_name, id) {
+    return 'v' + id + '_' + field_name;
+}
+
 /*********************************/
 /* Code for Hiding Options in IE */
 /*********************************/
@@ -431,24 +443,50 @@ function handleValControllerChange(e, args) {
  * on <option> tags. However, you *can* insert a Comment Node as a
  * child of a <select> tag. So we just insert a Comment where the <option>
  * used to be. */
+var ie_hidden_options = new Array();
 function hideOptionInIE(anOption, aSelect) {
     if (browserCanHideOptions(aSelect)) return;
 
     var commentNode = document.createComment(anOption.value);
-    aSelect.replaceChild(commentNode, anOption);
+    commentNode.id = anOption.id;
+    // This keeps the interface of Comments and Options the same for
+    // our other functions.
+    commentNode.disabled = true;
+    // replaceChild is very slow on IE in a <select> that has a lot of
+    // options, so we use replaceNode when we can.
+    if (anOption.replaceNode) {
+        anOption.replaceNode(commentNode);
+    }
+    else {
+        aSelect.replaceChild(commentNode, anOption);
+    }
+
+    // Store the comment node for quick access for getPossiblyHiddenOption
+    if (!ie_hidden_options[aSelect.id]) {
+        ie_hidden_options[aSelect.id] = new Array();
+    }
+    ie_hidden_options[aSelect.id][anOption.id] = commentNode;
 }
 
 function showOptionInIE(aNode, aSelect) {
-    if (browserCanHideOptions(aSelect)) return;
-    // If aNode is an Option
-    if (typeof(aNode.value) != 'undefined') return;
+    if (browserCanHideOptions(aSelect)) return aNode;
 
     // We do this crazy thing with innerHTML and createElement because
     // this is the ONLY WAY that this works properly in IE.
     var optionNode = document.createElement('option');
     optionNode.innerHTML = aNode.data;
     optionNode.value = aNode.data;
-    var old_node = aSelect.replaceChild(optionNode, aNode);
+    optionNode.id = aNode.id;
+    // replaceChild is very slow on IE in a <select> that has a lot of
+    // options, so we use replaceNode when we can.
+    if (aNode.replaceNode) {
+        aNode.replaceNode(optionNode);
+    }
+    else {
+        aSelect.replaceChild(optionNode, aNode);
+    }
+    delete ie_hidden_options[aSelect.id][optionNode.id];
+    return optionNode;
 }
 
 function initHidingOptionsForIE(select_name) {
@@ -465,26 +503,19 @@ function initHidingOptionsForIE(select_name) {
     }
 }
 
-function getPossiblyHiddenOption(aSelect, aValue) {
-    var val_index = bz_optionIndex(aSelect, aValue);
-
-    /* We have to go fishing for one of our comment nodes if we
-     * don't find the <option>. */
-    if (val_index < 0 && !browserCanHideOptions(aSelect)) {
-        var children = aSelect.childNodes;
-        for (var i = 0; i < children.length; i++) {
-            var item = children[i];
-            if (item.data == aValue) {
-                // Set this for handleValControllerChange, so that both options
-                // and commentNodes have this.
-                children[i].disabled = true;
-                return children[i];
-            }
-        }
+function getPossiblyHiddenOption(aSelect, optionId) {
+    // Works always for <option> tags, and works for commentNodes
+    // in IE (but not in Webkit).
+    var id = _value_id(aSelect.id, optionId);
+    var val = document.getElementById(id);
+
+    // This is for WebKit and other browsers that can't "display: none"
+    // an <option> and also can't getElementById for a commentNode.
+    if (!val && ie_hidden_options[aSelect.id]) {
+        val = ie_hidden_options[aSelect.id][id];
     }
 
-    /* Otherwise we just return the Option we found. */
-    return aSelect.options[val_index];
+    return val;
 }
 
 var browser_can_hide_options;
diff --git a/skins/standard/global.css b/skins/standard/global.css
index d36f88f4a..3e5765434 100644
--- a/skins/standard/global.css
+++ b/skins/standard/global.css
@@ -319,7 +319,7 @@ div#docslinks {
 
 /** End Comments **/
 
-.bz_default_hidden, .bz_tui_hidden {
+.bz_default_hidden, .bz_tui_hidden, .bz_hidden_field, .bz_hidden_option {
     /* We have !important because we want elements with these classes to always
      * be hidden, even if there is some CSS that overrides it (we use these
      * classes inside JavaScript to hide things). */
@@ -456,13 +456,6 @@ div.user_match {
     vertical-align: top;
 }
 
-.bz_hidden_field, .bz_hidden_option {
-    display: none;
-}
-.bz_hidden_option {
-    visibility: hidden;
-}
-
 .calendar_button {
     background: transparent url("global/calendar.png") no-repeat;
     width: 20px;
diff --git a/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl b/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl
index b215edf04..64c24357d 100644
--- a/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl
+++ b/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl
@@ -126,13 +126,15 @@
     [% IF value.controlled_values.size %]
       <li>This value controls the visibility of the following values in
         other fields:<br>
-       [% FOREACH controlled = value.controlled_values %]
-         <a href="editvalues.cgi?action=edit&field=
-                  [%- controlled.field.name FILTER url_quote %]&value=
-                  [%- controlled.name FILTER url_quote %]">
-           [% controlled.field.description FILTER html %]
-           ([% controlled.field.name FILTER html %]):
-           [%+ controlled.name FILTER html %]</a><br>
+       [% FOREACH field_name = value.controlled_values.keys %]
+         [% FOREACH controlled = value.controlled_values.${field_name} %]
+           <a href="editvalues.cgi?action=edit&field=
+                    [%- controlled.field.name FILTER url_quote %]&value=
+                    [%- controlled.name FILTER url_quote %]">
+             [% controlled.field.description FILTER html %]
+             ([% controlled.field.name FILTER html %]):
+             [%+ controlled.name FILTER html %]</a><br>
+         [% END %]
        [% END %]
       </li>
     [% END %]
diff --git a/template/en/default/bug/field-events.js.tmpl b/template/en/default/bug/field-events.js.tmpl
index 7cdf64687..06fba1245 100644
--- a/template/en/default/bug/field-events.js.tmpl
+++ b/template/en/default/bug/field-events.js.tmpl
@@ -27,10 +27,14 @@
                 '[% controlled_field.visibility_value.name FILTER js %]');
 [% END %]
 [% FOREACH legal_value = field.legal_values %]
-  [% FOREACH controlled_value = legal_value.controlled_values %]
-    showValueWhen('[% controlled_value.field.name FILTER js %]',
-                  '[% controlled_value.name FILTER js %]',
+  [% FOREACH controlled_field = legal_value.controlled_values.keys %]
+    [% SET cont_ids = [] %]
+    [% FOREACH val = legal_value.controlled_values.$controlled_field %]
+      [% cont_ids.push(val.id) %]
+    [% END %]
+    showValueWhen('[% controlled_field FILTER js %]',
+                  [[% cont_ids.join(',') FILTER js %]],
                   '[% field.name FILTER js %]',
-                  '[% legal_value.name FILTER js %]');
+                  [% legal_value.id FILTER js %]);
   [% END %]
 [% END %]
diff --git a/template/en/default/bug/field.html.tmpl b/template/en/default/bug/field.html.tmpl
index 1d5a194e2..21a73a805 100644
--- a/template/en/default/bug/field.html.tmpl
+++ b/template/en/default/bug/field.html.tmpl
@@ -134,6 +134,8 @@
             [% SET control_value = legal_value.visibility_value %]
             [% SET control_field = field.value_field %]
             <option value="[% legal_value.name FILTER html %]"
+                    id="v[% legal_value.id FILTER html %]_
+                        [%- field.name FILTER html %]"
               [%# We always show selected values, even if they should be
                 # hidden %]
               [% IF value.contains(legal_value.name).size %]
@@ -144,8 +146,7 @@
               %]
                 class="bz_hidden_option" disabled="disabled"
               [% END %]>
-              [%- legal_value.name FILTER html -%]
-            </option>
+              [%- legal_value.name FILTER html %]</option>
           [% END %]
         </select>
         [%# When you pass an empty multi-select in the web interface,
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 7994b187c..69811f210 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -490,9 +490,11 @@
     [% IF vals.size %]
       it controls the visibility of the following field values:
       <ul>
-        [% FOREACH val = vals %]
-          <li>[% val.field.name FILTER html %]:
-            '[% val.name FILTER html %]'</li>
+        [% FOREACH field_name = vals.keys %]
+          [% FOREACH val = vals.${field_name} %]
+            <li>[% val.field.name FILTER html %]:
+              '[% val.name FILTER html %]'</li>
+          [% END %]
         [% END %]
       </ul>
     [% END %]