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.

Files: 
CommentFileSizeAuthor
#26 tableselect-enabledonly-363354-26.patch659 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,047 pass(es).
[ View ]
#23 tableselect-enabledonly-363354-23.patch644 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,598 pass(es).
[ View ]
#13 tableselect-enabledonly.patch780 bytescasey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tableselect-enabledonly.patch.
[ View ]
#7 tableSelect.patch780 bytesaspilicious
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
tableselect-only-enabled.patch630 bytesBoobaa
Failed: Failed to apply patch.
[ View ]

Comments

Title:Tableselect only enabled checkboxesTableselect 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.

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.

Disabled checkboxes and radios are supported in a tableselect now.

Should I reroll that patch?

Yes please, that would be great! :)

Status:Needs work» Needs review
StatusFileSize
new780 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Here it is

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new780 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tableselect-enabledonly.patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

This is rtbc.

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

*Retesting old patches*

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

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).

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.

Issue tags:-needs backport to D7

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

#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.

StatusFileSize
new644 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,598 pass(es).
[ View ]

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

Status:Needs work» Reviewed & tested by the community

She's still green if the patch tests properly.

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.

Status:Needs work» Needs review
StatusFileSize
new659 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,047 pass(es).
[ View ]

Fixed for #25.

Issue summary:View changes

Revamped issue summary.

Added issue summary.

Status:Needs review» Reviewed & tested by the community

Thanks!

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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).

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

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

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.

Issue summary:View changes

It's a bugfix, not a feature.