Problem/Motivation

The "check all" checkbox in table headers currently checks all checkboxes in the table column, regardless of whether the checkbox is disabled or not. This may confuse users, as the disabled checkboxes are not actually submitted.

Steps to reproduce

  1. Theme a form to have several checkboxes in a table, with at least one of them being #disabled = TRUE
  2. Add the select-all checkbox to the table via theme_table_select_header_cell();
  3. Add the select-all checkbox to the table via theme_table_select_header_cell();
  4. Visit the form and click the select-all checkbox

Expected result

The select-all checkbox should select only enabled checkboxes (those with #disabled unset or FALSE).

Actual result

All of the checkboxes are checked, despite that the disabled ones could not be selected by the user (via mouse nor keyboard) directly.

Proposed resolution

Modify the javascript callback to check only non-disabled elements. Final patch implementing this change is in #363354-26: Tableselect should only select enabled checkboxes.

Remaining tasks

Determine whether to backport. The fix is small and backwards-compatible.

User interface changes

Select all checkbox no longer selects disabled elements.

API changes

None.

Original report by @Boobaa

Tableselect's select-all checkbox (the one in the table header) should select only enabled checkboxes.

Wrong behaviour: at the time of writing even drupal-7.x-dev has a select-all checkbox which select all the corresponding checkboxes, regardless of their enabled/disabled status.

Steps to reproduce: theme a form to have several checkboxes in a table, with at least one of them being #disabled = TRUE; add the select-all checkbox to the table via theme_table_select_header_cell(); and click this select-all checkbox - voilá, all of the checkboxes are selected, while the disabled ones could not be selected by the user (via mouse nor keyboard) by themselves.

Expected behaviour: the select-all checkbox should select only enabled checkboxes (IOW the ones with #disabled unset or FALSE).

Please find the attached patch which implements this - I have tested it with drupal-6.9 as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apaderno’s picture

Title: Tableselect only enabled checkboxes » Tableselect should only select enabled checkboxes

It is just a visual problem, as the form submit code will not get the values associated with the disabled checkboxes.

cburschka’s picture

Pardon me if I'm wrong, but I don't see support for disabled checkboxes at all in the tableselect form element. Should the Javascript have to anticipate a use case that bypasses the Form API?

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Disabled checkboxes and radios are supported in a tableselect now.

aspilicious’s picture

Should I reroll that patch?

Dave Reid’s picture

Yes please, that would be great! :)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
780 bytes

Here it is

Status: Needs review » Needs work

The last submitted patch, tableSelect.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#7: tableSelect.patch queued for re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
realityloop’s picture

#7: tableSelect.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, tableSelect.patch, failed testing.

casey’s picture

Status: Needs work » Needs review
FileSize
780 bytes
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

This is rtbc.

aspilicious’s picture

#13: tableselect-enabledonly.patch queued for re-testing.

aspilicious’s picture

*Retesting old patches*

sun’s picture

#13: tableselect-enabledonly.patch queued for re-testing.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport, not sure if this one actually should be or not but looks like a straight bug fix.

webchick’s picture

Issue tags: -Needs backport to D7

#13: tableselect-enabledonly.patch queued for re-testing.

xjm’s picture

#13: tableselect-enabledonly.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, tableselect-enabledonly.patch, failed testing.

xjm’s picture

Attached simply rerolls #13 in correct format and for current codebase.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

She's still green if the patch tests properly.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/misc/tableselect.js
@@ -36,8 +36,8 @@ Drupal.tableSelect = function () {
-  checkboxes = $('td input:checkbox', table).click(function (e) {
...
+  checkboxes = $('td input:checkbox:enabled', table).click(function(e) {

Bogus change: There should be a space after "function" for anonymous functions, see JS coding standards.

+++ b/misc/tableselect.js
@@ -36,8 +36,8 @@ Drupal.tableSelect = function () {
+  // For each of the enabled checkboxes within the table.

The "enabled" in this comment is a bit too ambiguous... it confused me big time, had to look up what :enabled is doing and whether it is correct. (it is)

The :enabled pseudo-selector is the negation of :disabled, whereas :disabled maps to the "disabled" HTML attribute, which actually exists.

Can we change the comment to state "...the checkboxes within the table that are not disabled." ?

1 days to next Drupal core point release.

xjm’s picture

Status: Needs work » Needs review
FileSize
659 bytes

Fixed for #25.

xjm’s picture

Issue summary: View changes

Revamped issue summary.

xjm’s picture

Added issue summary.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

sun’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Note from catch re: backport and more accurate UI info (technically, there will be a change in user-facing behavior, even if it's a bugfix).

catch’s picture

catch’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x. Looks good for backport so moving back for webchick to consider.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense to me as well.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

It's a bugfix, not a feature.