| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
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
- 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(); - Add the select-all checkbox to the table via
theme_table_select_header_cell(); - 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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| tableselect-only-enabled.patch | 630 bytes | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
It is just a visual problem, as the form submit code will not get the values associated with the disabled checkboxes.
#2
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?
#3
The last submitted patch failed testing.
#4
Disabled checkboxes and radios are supported in a tableselect now.
#5
Should I reroll that patch?
#6
Yes please, that would be great! :)
#7
Here it is
#8
The last submitted patch, tableSelect.patch, failed testing.
#9
#7: tableSelect.patch queued for re-testing.
#10
#11
#7: tableSelect.patch queued for re-testing.
#12
The last submitted patch, tableSelect.patch, failed testing.
#13
#14
This is rtbc.
#15
#13: tableselect-enabledonly.patch queued for re-testing.
#16
*Retesting old patches*
#17
#13: tableselect-enabledonly.patch queued for re-testing.
#18
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).
#19
Tagging for backport, not sure if this one actually should be or not but looks like a straight bug fix.
#20
#13: tableselect-enabledonly.patch queued for re-testing.
#21
#13: tableselect-enabledonly.patch queued for re-testing.
#22
The last submitted patch, tableselect-enabledonly.patch, failed testing.
#23
Attached simply rerolls #13 in correct format and for current codebase.
#24
She's still green if the patch tests properly.
#25
+++ 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.
#26
Fixed for #25.
#27
Added issue summary.
#28
Thanks!
#29
#26: tableselect-enabledonly-363354-26.patch queued for re-testing.
#30
Committed to 8.x. Looks good for backport so moving back for webchick to consider.
#31
Makes sense to me as well.
Committed and pushed to 7.x. Thanks!
#32
Automatically closed -- issue fixed for 2 weeks with no activity.