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.
Comment | File | Size | Author |
---|---|---|---|
#26 | tableselect-enabledonly-363354-26.patch | 659 bytes | xjm |
#23 | tableselect-enabledonly-363354-23.patch | 644 bytes | xjm |
#13 | tableselect-enabledonly.patch | 780 bytes | casey |
#7 | tableSelect.patch | 780 bytes | aspilicious |
tableselect-only-enabled.patch | 630 bytes | Boobaa | |
Comments
Comment #1
apadernoIt is just a visual problem, as the form submit code will not get the values associated with the disabled checkboxes.
Comment #2
cburschkaPardon 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?
Comment #4
Dave ReidDisabled checkboxes and radios are supported in a tableselect now.
Comment #5
aspilicious CreditAttribution: aspilicious commentedShould I reroll that patch?
Comment #6
Dave ReidYes please, that would be great! :)
Comment #7
aspilicious CreditAttribution: aspilicious commentedHere it is
Comment #9
aspilicious CreditAttribution: aspilicious commented#7: tableSelect.patch queued for re-testing.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedComment #11
realityloop#7: tableSelect.patch queued for re-testing.
Comment #13
casey CreditAttribution: casey commentedComment #14
aspilicious CreditAttribution: aspilicious commentedThis is rtbc.
Comment #15
aspilicious CreditAttribution: aspilicious commented#13: tableselect-enabledonly.patch queued for re-testing.
Comment #16
aspilicious CreditAttribution: aspilicious commented*Retesting old patches*
Comment #17
sun#13: tableselect-enabledonly.patch queued for re-testing.
Comment #18
sunAlthough 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).
Comment #19
catchTagging for backport, not sure if this one actually should be or not but looks like a straight bug fix.
Comment #20
webchick#13: tableselect-enabledonly.patch queued for re-testing.
Comment #21
xjm#13: tableselect-enabledonly.patch queued for re-testing.
Comment #23
xjmAttached simply rerolls #13 in correct format and for current codebase.
Comment #24
xjmShe's still green if the patch tests properly.
Comment #25
sunBogus change: There should be a space after "function" for anonymous functions, see JS coding standards.
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.
Comment #26
xjmFixed for #25.
Comment #26.0
xjmRevamped issue summary.
Comment #27
xjmAdded issue summary.
Comment #28
sunThanks!
Comment #28.0
sunUpdated issue summary.
Comment #28.1
xjmUpdated issue summary.
Comment #28.2
xjmNote 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).
Comment #29
catch#26: tableselect-enabledonly-363354-26.patch queued for re-testing.
Comment #30
catchCommitted to 8.x. Looks good for backport so moving back for webchick to consider.
Comment #31
webchickMakes sense to me as well.
Committed and pushed to 7.x. Thanks!
Comment #32.0
(not verified) CreditAttribution: commentedIt's a bugfix, not a feature.