Disable submit buttons if required items are not selected

[Ryan J Ollos <ryan.j.ollos@…>]

A feature initially discussed in #10994 is to disable Remove selected items and Add attachment buttons if no items have been selected - table entries for the former and a file for the latter. The feature might be implemented for some additional buttons while working on this ticket. The initial patch can be seen in ticket:10994:t10994-r11508-1.patch​. I'm targeting this to 1.0.2 per comment:9:ticket:10994.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11682 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

t11056-r11682-1.patch​ applies behavior to the Add attachment (/attachment/*), Change tickets (/query), Install (/admin/general/plugin) and Remove selected items (/admin/ticket/*, /admin/versioncontrol/repository, /admin/general/perm) buttons.

One particular behavior worth pointing out is that by clicking on the labels for the Subject or Group all of the checkboxes in the row are toggled. This is a nice convenience, but unfortunately I can't see a way to make it more obvious to the user.

There are some other places that similar behavior could be implemented. I've opted not to include these in the patch because I couldn't convince myself that they would necessarily improve usability, but I'm hoping to hear additional opinions:

• /newticket: Disable Create ticket until at least one character has been entered in the Summary (or disable if summary is greater than max_description_size).
• /search: Disable Search until at least min_query_length characters have been entered.
• /milestone?action=new: Disable Add milestone until at least one character has been entered for the Name of the milestone field.

The general criteria I took when deciding where to implement the button disabling is to disable a button if the user could be redirected to an error page by not selecting an item before submitting.

[Remy Blank]

Looks nice. I'll review and apply the current patch.

[Christian Boos]

I'm a bit concerned about the increasing complexity of the Javascript, which changes like this.

For example in this patch, there are two very specific selectors for each of the invocation of which.disableIfNoneChecked(what). Not only one has to find the proper selector expressions, but they have to be maintained whenever the structure changes (either which or what).

So I wonder if we couldn't find a better way to encapsulate this, kind of standard "behaviors" for those forms (e.g. a trac-choose class on the which side, a trac-choose-option class for the input from which one must be chosen (or a "negative" one), and have the disableIfNoneChecked() behavior applied to all of these, without having to care for how the individual case is placed or named within each page.

In short, I think we should find a way to improve encapsulation, pack together the structure and its behavior.

[Ryan J Ollos <ryan.j.ollos@…>]

That sounds interesting, but I'm probably not grasping the plan yet. It sounds like it might be a way to satisfy my aim for some lightweight generic code in comment:3:ticket:11027 though.

Would it look something like this?:

1. Add trac-disable class to the Remove selected items button.
2. Add trac-disable-unchecked class to the checkboxes.
3. Add the following trac.js:

   $("input:submit.trac-disable").disableIfNoneChecked(
     "input:checkbox.trac-disable-unchecked");
   

4. Modify $.fn.disableIfNoneChecked to take a selector string as input rather than a wrapped set.

[Christian Boos]

Well, I don't have a plan yet, just trying to shape things as we go… Something good will hopefully emerge ;-)

And what you proposed is already much better I think.

[Ryan J Ollos <ryan.j.ollos@…>]

I'll work up a new patch to serve as a point of discussion. It will probably be a few days before I can spend the required time on it though.

#11027 may be another good use-case to help with shaping things since the patch is fairly simple.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

One particular behavior worth pointing out is that by clicking on the labels for the Subject or Group all of the checkboxes in the row are toggled. This is a nice convenience, but unfortunately I can't see a way to make it more obvious to the user.

I probably got the idea for this from ​th:QueryUiAssistPlugin, and I think it would be worthwhile to implement the feature more widely in Trac such as on the custom query filter labels.

[Ryan J Ollos]

I've been working on simplifying the selectors, with the idea that the jQuery code would go in layout.html. A similar, and much simpler scenario, would involve moving the datepicker and datetimepicker calls to layout.html, so I made an attempt at that change first: log:rjollos.git:t11056. How does that look?

I was a bit surprised by the need to test the length of the jQuery object before calling datepicker and datetimepicker. I haven't looked at the source, but I wonder why the behavior is different than other jQuery functions such as focus?:

> $().focus()
[]
> $().datetime()
TypeError: Object [object Object] has no method 'datetime'

[Jun Omae]

I've been working on simplifying the selectors, with the idea that the jQuery code would go in layout.html. A much simpler scenario would be involve moving the datepicker and datetimepicker calls to layout.html, so I made an attempt at that change first: log:rjollos.git:t11056. How does that look?

Looks good. However, I like .trac-datepicker and .trac-datetimepicker rather than .trac-date and .trac-datetime. We might use the classes like this in the future.

   <span class="trac-datetime" data-trac-datetime="2013-12-21T15:22:42Z">42 minutes ago</span>

I was a bit surprised by the need to test the length of the jQuery object before calling datepicker and datetimepicker. I haven't looked at the source, but I wonder why the behavior is different than other jQuery functions such as focus?:

$.datepicker and $.datetimepicker functions require jquery-ui.js and jquery-ui-addons.js. If Chrome.add_jquery_ui is not called, these functions are not available.

trac/templates/layout.html

@@ -47 +47 @@
         </py:if>
         $(".trac-autofocus").focus();
         $(".trac-target-new").attr("target", "_blank");
-        if ($(".trac-date").length) $(".trac-date").datepicker();
-        if ($(".trac-datetime").length) $(".trac-datetime").datetimepicker();
+        if ($.ui) { /* is jquery-ui added? */
+          $(".trac-date").datepicker();
+          $(".trac-datetime").datetimepicker();
+        }
         setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);
       });
     </script>

[Ryan J Ollos]

Latest changes can be found in log:rjollos.git:t11056.

• Included Jun's changes from comment:13. Thanks!
• Group toggling on permissions page:
  • I could imagine some refinement to this behavior, but it's not clear to me yet.
  • The code to disable selection of text is from ​SO:2700029/121694 and a function could be added to trac.js.
• I'm not sure that I like the naming of the trac-disable / trac-disable-determinant classes, so I will continue to give that some thought.

Tested with Chrome 31 / Firefox / Opera 12.16 on Ubuntu 13.04.

[Ryan J Ollos]

Fixed an issue and cleaned up commit history in log:rjollos.git:t11056.2.

[Ryan J Ollos]

Replying to rjollos:

• The code to disable selection of text is from ​SO:2700029/121694 and a function could be added to trac.js.

If that isn't done though, the multiple calls to css and attr could be combined.

[Ryan J Ollos]

Committed to trunk in [12373:12374]. Minor change committed to 1.0-stable in [12375] and merged to trunk in [12376].

[Ryan J Ollos]

Replying to rjollos:

• Group toggling on permissions page:
  • I could imagine some refinement to this behavior, but it's not clear to me yet.
  • The code to disable selection of text is from ​SO:2700029/121694 and a function could be added to trac.js.

These changes have been moved to #11417.

[Ryan J Ollos]

I'm thinking that we should add a tooltip: You must select at least one item in the table. Previously the batch modification form would display next to the Change tickets button: You must select at least one ticket.

[Ryan J Ollos]

Also, it seems like we could just remove the disableSubmit call from query.js and add the trac-disable, trac-disable-determinant classes where necessary.

[Ryan J Ollos]

Proposed changes can be found in log:rjollos.git:t11056.3. Moving the validation out of submit in the batch modification form will make it easier to implement #10138 by avoiding multiple submit handlers (comment:21:ticket:10138).

[Ryan J Ollos]

The string in [22aa2281/rjollos.git] needs to be wrapped for translation. I'll fix that before committing the changes.

[Ryan J Ollos]

Changes from comment:21 committed in [12450:12451].

[Ryan J Ollos]

In [12373#file1] the trac-datetimepicker class is only added if readonly is False. It looks like it would be better to modify the selectors so that the datepicker and datetimepicker aren't added if the readonly attribute is present:

trac/templates/layout.html

@@ -60 +60 @@
         $(".trac-autofocus").focus();
         $(".trac-target-new").attr("target", "_blank");
         if ($.ui) { /* is jquery-ui added? */
-          $(".trac-datepicker").datepicker();
-          $(".trac-datetimepicker").datetimepicker();
+          $(".trac-datepicker:not([readonly])").datepicker();
+          $(".trac-datetimepicker:not([readonly])").datetimepicker();
         }
         $(".trac-disable").disableSubmit(".trac-disable-determinant");
         setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);

trac/ticket/templates/milestone_edit_form.html

@@ -62 +62 @@
           </label>
           <label>
             <input type="text" id="duedate" name="duedate"
-                   class="${'trac-datetimepicker' if not readonly else None}"
-                   size="${len(datetime_hint)}"
+                   class="trac-datetimepicker" size="${len(datetime_hint)}"
                    value="${format_datetime(milestone.due or default_due)}"
                    readonly="$readonly"
                    title="${_('Format: %(datehint)s', datehint=datetime_hint)}"
@@ -78 +77 @@
           </label>
           <label>
             <input type="text" id="completeddate" name="completeddate"
-                   class="${'trac-datetimepicker' if not readonly else None}"
-                   size="${len(datetime_hint)}"
+                   class="trac-datetimepicker" size="${len(datetime_hint)}"
                    value="${format_datetime(milestone.completed)}"
                    readonly="$readonly"
                    title="${_('Format: %(datehint)s', datehint=datetime_hint)}"

[Ryan J Ollos]

We should maybe capture disabled elements as well: .trac-datepicker:not([readonly]):not([disabled]) (untested). does not seem to be needed.

[Ryan J Ollos]

Change from comment:24 committed in [12867].