- Issue created by @Anybody
- First commit to issue fork.
- Merge request !8678Issue #3459246: Add a '#selectall' option for checkboxes element â (Open) created by MaxPah
- Status changed to Needs work
2 months ago 4:08pm 5 July 2024 - đ«đ·France MaxPah
Hello,
Forced to rename '#checkall' to '#check_all' due to spell-checking test.
For now code, is only a proof of concept. It miss some documentation, if the implementation is what you think of, i'll doing it. - đłđżNew Zealand quietone New Zealand
Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. Also, 10.2 is in security mode now.
- đ©đȘGermany Anybody Porta Westfalica
@MaxPah thank you very much! Naming this check_all is totally fine!
What's needed now:
- Write tests to ensure it works as expected
To be discussed:
- Should we hide the button until JS is ready, so it's not shown without JS? Is there a core policy for that? Or a helper class maybe?
- Should we already copy and align the module functionality for the Drupal Field API UI? Or leave this in contrib for now?
- đ©đȘGermany Anybody Porta Westfalica
PS: @MaxPah would you mind implementing the functionality and test also here in contrib:
âš Extend core's Checkboxes Element ActiveIt will need to be changed a bit, but I think it would be helpful to already have this in contrib, until it reaches core?
- First commit to issue fork.
- Merge request !8985Issue #3459246: Add a '#selectall' option for checkboxes element â (Open) created by LRWebks
- đ©đȘGermany LRWebks Porta Westfalica
LRWebks â changed the visibility of the branch 11.x to hidden.
- đ©đȘGermany LRWebks Porta Westfalica
LRWebks â changed the visibility of the branch 3459246-add-a-selectall to hidden.
- Status changed to Needs review
about 1 month ago 11:34am 30 July 2024 - đ©đȘGermany LRWebks Porta Westfalica
I have implemented some fixes, as the current behavior did not work in any form, as well as the addition of general tests for the Checkboxes field, since they didn't exist before. This should be fine now!
- đ©đȘGermany Anybody Porta Westfalica
Made PHPCS happy. This looks really good, thank you @LRWebks!
Regarding
Create follow-up to add a setting to checkboxes field widget to enable / disable this functionality e.g. for entity reference checkboxes, lists, etc.
I think it makes sense to also implement this Field API addition here. If needed, we could sort it out later, but it's based on this issue so implementing it elsewhere will make things a lot harder.
So I think you should go for it here.
- Status changed to Needs work
about 1 month ago 11:54am 30 July 2024 - đ©đȘGermany Anybody Porta Westfalica
Some tests are failing, e.g.
There was 1 error: 1) Drupal\Tests\views_ui\Functional\DisplayFeedTest::testFeedUI Error: Call to a member function hasAttribute() on null /builds/issue/drupal-3459246/core/modules/views_ui/tests/src/Functional/DisplayFeedTest.php:68 /builds/issue/drupal-3459246/core/modules/views_ui/tests/src/Functional/DisplayFeedTest.php:40 ERRORS! Tests: 1, Assertions: 16, Errors: 1.
Furthermore, I can't see a
#check_all
option being handled anywhere?! Please re-read the issue summary and comments. - đ©đȘGermany Anybody Porta Westfalica
MR!8678 by @MaxPah had the required lines of code btw.!
- đ©đȘGermany Anybody Porta Westfalica
Anybody â changed the visibility of the branch 3459246-add-a-selectall to active.
- đ©đȘGermany Anybody Porta Westfalica
Anybody â changed the visibility of the branch 3459246-add-a-selectall to hidden.
- đ©đȘGermany Anybody Porta Westfalica
Anybody â changed the visibility of the branch 11.x to active.
- đźđłIndia Prashant.c Dharamshala
@Anybody @LRWebks thanks for your efforts, this would be a great addition.
The "Check all" and "Uncheck all" buttons working as expected for the "#checboxes" element.
While interacting with it I realized a few things, that are most related to user experience:
- "Wouldn't it be more convenient if we had a single button that could check and uncheck all the checkboxes? The button label would change based on the state: 'Check all' when none are selected, and 'Uncheck all' when all are selected.
- Would it make sense to add a property, something like
#hide_check_all => TRUE
, to hide these buttons when needed, with the default set to FALSE so they are shown by default?
- đźđłIndia Prashant.c Dharamshala
As per the comment https://www.drupal.org/project/drupal/issues/3459246#comment-15705249 âš Add a '#selectall' option for checkboxes element Active
There should be option
#check_all
which I would say should be TRUE by default and display the button to toggle check-all/uncheck-all andFALSE
should not render this/these buttons.Based on what we are implementing (
#select_all
,#selectall
or#check_all
or#checkall
) we should change in the issue title as well :). - đ©đȘGermany LRWebks Porta Westfalica
Regarding #16 and the lines that I had previously removed from @MaxPeh's MR - it seems that there might have been a misunderstanding regarding the functionality on my end.
Please explain in greater detail what the
#check_all
option should be and where it would reside. Is this supposed to be handled via the Form render tree as a separate attribute like#type, #title, #check_all, #value
, etc.? That would make sense to me.Also, regarding #22, yes, I agree that we should reduce this option to a single button if possible. "Check all" if none are checked, "Uncheck all" if all are checked. The question remains what we want to do if only of the checkboxes are checked - do we use "Check all", "Uncheck all", do we show both buttons then or do we hide the button completely since we cannot really know what the user wants to do?
- đ©đȘGermany Anybody Porta Westfalica
@LRWebks:
Usage example:$form['favorites']['colors'] = [ '#type' => 'checkboxes', '#options' => ['blue' => $this->t('Blue'), 'red' => $this->t('Red')], '#check_all' => TRUE, // This makes the "Check all" functionality appear! '#title' => $this->t('Which colors do you like?'), ... ];
(https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...)
The option must fallback to
FALSE
to not introduce a breaking change (opt in).Regarding the single button I'd just use a button with
(Un)check all label.The logic is:
If *any* checkbox is unchecked, check all. If all checked, uncheck all.I think that's what would be expected.
- đ©đȘGermany LRWebks Porta Westfalica
I have added the
#check_all
property, reduced the two buttons to a single one according to @Prashant.c and @Anybody's suggestions and updated theCheckboxesTest.php
to the new changes.Credit definitely goes to @MaxPah for laying out the solid groundwork, which I had definitely misinterpreted before :)
It's good to know that the newly added test also found some cases in
Checkboxes.php
where we forgot to check if a render array key was set before using it, so I changed these two lines as well (not really unrelated, since we have just now created the tests for the Checkboxes element as a whole). - đ©đȘGermany LRWebks Porta Westfalica
I'm going to take a look at what's causing the other tests to fail now!
- đ©đȘGermany Anybody Porta Westfalica
@LRWebks thanks!
Better suggestion for the label: "Check all / none" - I think that's clear and short.
Afterwards please implement the field api checkbox functionality / ui. :)
Could you assign this task to you please, while you're working on it actively? - Assigned to LRWebks
- đ©đȘGermany LRWebks Porta Westfalica
Do you want me to fully implement the checkboxes field for the Field API or add this function to an existing field? Because as far as I can see there is only a field for a single checkbox and not for multiple. We would need to build this field completely, right?
- đźđłIndia Prashant.c Dharamshala
@LRWebks
You have created a new branch instead you should have pushed your changes to the existing branch https://git.drupalcode.org/issue/drupal-3459246/-/tree/3459246-add-a-sel... itself.
We do not need this branch https://git.drupalcode.org/issue/drupal-3459246/-/tree/11.x, you can hide it.
- đźđłIndia Prashant.c Dharamshala
- Custom for element "Check all/none" not displaying by default which is correct
- In the Checkboxes field widget the "check all/none" setting is also available and working fine, tested with the content types. Should not be an issue with other entity types as well.
- We need to fix the existing failed tests and write new tests for this.
Adding
'#check_all' => TRUE
displays the button to toggle check all, which is also as expected
- đ©đȘGermany Anybody Porta Westfalica
We need to fix the existing failed tests
Fixed in latest commit!
So the last part is to write additional tests. Any experience here @Prashant.c?
- đ©đȘGermany LRWebks Porta Westfalica
LRWebks â changed the visibility of the branch 11.x to hidden.
- đŹđ§United Kingdom joachim
Iâd assumed the UI would be an extra checkbox at the top of the column, the same way as for tableselect.
- đ©đȘGermany Anybody Porta Westfalica
@joachim
Iâd assumed the UI would be an extra checkbox at the top of the column, the same way as for tableselect.
Do you know about any best practices or widely used libraries solving it that way (or in another way)?
For tables, it's a widely known pattern to have a "Check all / none" checkbox in the heading column. Here we don't have a heading column, but a list of checkboxes and I can hardly imagine how it would look like having "just another" checkbox at the top or bottom to check them all, even if we make the "Check all / none" checkbox text or the whole row styled differently.
Furthermore, it might lead to misunderstandings in meaning, if that checkbox is checked (but others not maybe). We don't run into these cases using a button.So I'm absolutely not against other better solutions than using the button (which might also get some styling or icon maybe), but I haven't seen a better one yet, so I think there are more good reasons for the button than for another checkbox in this situation.
What do you think how to proceed in that point, is that a blocker for you?
- đ©đȘGermany Anybody Porta Westfalica
PS: Styling this button less prominent and maybe smaller, perhaps introduce a global class for that button would make sense to me.
- Status changed to Needs review
about 1 month ago 10:18am 1 August 2024 - đ©đȘGermany LRWebks Porta Westfalica
Added the test for the field widget and works! Now we still need to discuss #37, I would be fine with both keeping it as is or introducing a new global class (I used the
button--small
class as that seemed like the most fitting class that already existed in the UI). - đźđłIndia Prashant.c Dharamshala
@LRWebks Great work. In the test, you have covered the widget on the entity but I think we need tests for the custom form element also.
- Issue was unassigned.
- Status changed to RTBC
about 1 month ago 1:19pm 1 August 2024 - đ©đȘGermany Anybody Porta Westfalica
@Prashant.c will you do that please?
I just reviewed and tested the functionality and it works great - as expected.
Setting this RTBC now to probably get final maintainer feedback. I think the time is right for that.Some points from my side:
- Additional tests for the form element needed? (#39)
- Maybe rename the library & js file from check-all(.js) to checkboxes-check-all(.js) or something like that to make the context more clear?
- Do we need to support disabled JS? That would mean, we should hide the button initially (display: none) and show it on JavaScript init. I wouldn't add the button in JS entirely, as the Form API is much cleaner here.
But yes in general this is excellent and super helpful for many checkboxes!
- Status changed to Needs work
27 days ago 3:10am 12 August 2024 - đłđżNew Zealand quietone New Zealand
The proposed resolution here is to "implement a proof of concept for this functionality in contrib". Clearly this is more than than. Adding tag for an issue summary update to clarify exactly what this is implementing.
This is changing the UI so there should be before and after screenshots available from the issue summary. Also, tagging Usability. Speaking of usability has the this had a review of the new text? I don't see one say adding tag for that. For that, I suggest asking in #ux how to get that review.
I read the comments in the MR and left some comments. I did not do a core review or test the change.
- Status changed to Needs review
19 days ago 11:18am 20 August 2024 - đ©đȘGermany LRWebks Porta Westfalica
@quietone @Anybody
Current Status:
- Corrected the test comment mistakes
- Renamed
check-all.js
tocheckboxes-check-all.js
as the previous name was a bit too ambiguous to be in themisc
folder (see #40 for the idea) - Regarding the tests for the Form element (#39, #40), these are already in place, see
CheckboxesTest.php
- Updated the issue summary
- Added "before" and "after" screenshots as @quietone requested
Stuff that is left:
- Usability review
- Final discussions
- Status changed to RTBC
19 days ago 11:32am 20 August 2024 - đ©đȘGermany Anybody Porta Westfalica
Back to RTBC, all points look addressed! Thank you @LRWebks!
How to get the usability review
- đźđłIndia Prashant.c Dharamshala
Not sure what I think it would be great to do some accessibility review as well?
- đ©đȘGermany rkoller NĂŒrnberg, Germany
The issue would need a rebase. on a none case sensitive file system ( i am on a mac) i run into the following error:
$> git checkout -b '3459246-add-a-selectall-11.x' --track drupal-3459246/'3459246-add-a-selectall-11.x' error: The following untracked working tree files would be overwritten by checkout: core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Connection.php core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Install/Tasks.php core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Connection.php core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Install/Tasks.php core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Connection.php core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Install/Tasks.php Please move or remove them before you switch branches. Aborting
it happens when the issue fork was created or last rebased before this issue went in https://git.drupalcode.org/project/drupal/-/commit/8b368d712d83900765744... on the 13th of august fixing spelling errors. the switch to camel case is the problem where the none case sensite file system in combination with git struggles and leads into an error.
- đźđłIndia Prashant.c Dharamshala
@rkoller I have rebased it, hope it won't show some test failures :).
- đ©đȘGermany rkoller NĂŒrnberg, Germany
thanks for the quick rebase @prashant.c (don't wanted to rebase myself in case an conflict turn up on rebase). tests are green, the error is gone, and i was able to checkout the feature branch now.
and in regards of the usability review, thanks to @quietone for adding the need usability review tag. i've added the issue to our shortlist ( #3468437-3: Drupal Usability Meeting 2024-08-23 â ). but just a headsup, not sure if we will be able to get to it this friday. at the moment is vacation time and we already had only three attendees last week and one of them is away this week. so not sure how many attendees will show up on friday and we should have at least three as the bare minimum to have a reasonable discussion with enough different perspectives. but i will have the issue prepared in case enough people show up.
and i've already taken a brief look. I'll add the tag for the WCAG success criterion relevant to this issue: https://www.w3.org/WAI/WCAG22/Understanding/name-role-value.html. Just a brief note based on the discussion happened in #35 and #36. No matter if going with a button- or a checkbox-solution to check and uncheck all checkboxes, the aural interface has to properly communicate the state to people with screenreaders. I've attached a quick video illustrating the current MR with voiceover in safari (button.mp4). in the current state the state of the toggle button is not communicated at all. adrian rosselli elaborated about the topic a few weeks ago: https://adrianroselli.com/2024/03/check-all-expand-all-controls.html . According to him no matter if you go with a button or a checkbox you have to use something that supports tristate. in his example he provided a demo for the checkbox solution: https://adrianroselli.com/demos/do-undo-all/ (see checkbox.mp4). It is definitely a good issue to discuss in an ux meeting. nothing i want to give any recommendation on at this point on my own. just wanted to illustrate the potential issue in regards of screenreaders and state upfront, which is imho one of the tricky aspects for this issue. will have to think about it for a bit.
- đ©đȘGermany Anybody Porta Westfalica
@rkoller thanks a lot!!
Regarding the options, my favourite would still be to go with a button, perhaps we can dynamically change the button label / title:
Check all (if one unchecked) <> Uncheck all (in all checked)
At least I personally dislike the checkbox tri-state approach for UXWould be great so see if there are best practices on this field!
- đ«đźFinland simohell
On the subject of checkbox vs button and old issue has a 9 year old comment #36 that is still IMO valid: "The functionality is already in Core, it's just about applying this consistently across the UI."
The functionality is in admin/content, nut missing from other similar lists.
I think we should aim to make the functionality available "everywhere" and decide on a unified style of UI for it across the admin UI. - đ©đȘGermany rkoller NĂŒrnberg, Germany
i agree with simo. the only problem, on
admin/content
the "check all"-checkbox is placed in the table header. so it is not directly applicable to the pattern used on a list field. I've done some research in preparation when we find time to discuss the issue in ux meeting. Hope we get to the issue tomorrow. - Status changed to Needs work
about 2 hours ago 11:37pm 7 September 2024 - đ©đȘGermany rkoller NĂŒrnberg, Germany
Usability review
We discussed this issue at đ Drupal Usability Meeting 2024-09-06 Needs work . That issue will have a link to a recording of the meeting. For the record, the attendees at today's usability meeting were @benjifisher, @rkoller, @simohell, and @worldlinemine.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
We have taken a look at the current state of MR8985. Overall we had the consensus that adding a check-all option would be a good thing. First a few details weâve noticed during our explorations:
- Completely out of scope: While setting up the select field, weâve realized it is not communicated anywhere in the admin UI how to choose between checkboxes or radio buttons. The option on the form display widget is simply called âCheck boxes/radio buttonsâ, but it is not communicated anywhere that things are controlled via the cardinality/âallowed number of valuesâ-setting on the field settings page. A cardinality limited to one switches to radio buttons, while anything bigger than one or
Unlimited
switches to checkboxes. - On a related note relevant to this issue, if you set the allowed number of values to one for the select field, the
Show "Check all / noneâ button
checkbox or better the form display widget settings in general is shown anyway, even though it is irrelevant without any purpose in this context. - The check-all button has a too low color contrast (#d3d4d9 against #ffffff => 1.5:1 -> 3:1 necessary), but there is already an issue for Core: đ Grey button's background color has a too low contrast ratio against page background Needs review .
- The button is no toggle button, so the current as well as the future state is unknown which is potentially confusing for screenreader users (see voiceover.mp4). Even though
aria-pressed
is supporting tri-state with offering themixed
state (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...), the support across screenreaders and browsers is subpar: https://a11ysupport.io/tech/aria/aria-pressed_attribute. Plus there is no component to communicate a pressed button state in the Drupal Design System at this point, and then there would also be the need for a âmixedâ button state as well. And how to communicate such an intermediate state for a button visually? - With the button state unknown, the button label adds another level of confusion - it contains both possible future states within the
Check all/none
label and readability wise it is sort hard to process with the slash separating the two possible states . For sighted users with the visual context, it might make them think for a second, but after using that pattern once it is manageable. Screenreader users instead donât have the state nor the visual context available, just based on the button label they donât know if pressing the button will check or uncheck everything and which of the available options is already ticked? Strictly speaking a screenreader user would have to check every available option before going back to the check-all button. The only shortcut might be by pressing the button, check the first option and based on its checkbox state go back to button in case the state is the opposite of the desired state. And switching the button label based on the state as suggested in #50 â has also its downsides for screenreader users as described for the âshow and hide passwordâ pattern by Leonie Watson in this webinar by the Smashing Magazine: https://youtu.be/OUDV1gqs9GA?si=b4cf_sUtRiF5DJ1f&t=3220 - Visually, the button and the set of checkboxes underneath feel disconnected, it feels like two separate sets of controls, one button and a set of eight checkboxes that are combined underneath a headline. Despite the spatial proximity it does not seem that the two are one functional unit.
After weâve gained an initial overview weâve turned towards the fundamental question, which of the two options should be selected, the button or the checkbox based pattern? In the run-up to the meeting Iâve done some research looking for resources about the check-all button and check-all checkbox pattern in the context of UX related resources. It has to be noted that all links in the following are about checkboxes, I was unable to find any article nor example about the check-all button pattern:
https://ux.stackexchange.com/questions/98089/nested-checkbox-behavior-ch...
https://www.nngroup.com/articles/checkboxes-design-guidelines/
https://app.uxcel.com/courses/ui-components-n-patterns/selection-control...
https://css-tricks.com/indeterminate-checkboxes/
https://uxdesign.cc/selection-controls-ui-component-series-3badc0bdb546
https://blog.logrocket.com/ux-design/checkbox-ui-design-best-practices-e...
https://forum.figma.com/t/select-all-checkbox/48623/5
https://www.setproduct.com/blog/checkbox-ui-designThen there are few examples of design system that are using the nested checkbox list pattern described in the aforementioned list of resources:
https://carbondesignsystem.com/components/checkbox/usage/
https://m2.material.io/components/checkboxes
https://atlassian.design/components/checkbox/examplesSo overall the nested checkbox list pattern seems to be the solution of choice from an UX perspective. This also corresponds to the accessibility perspective reflected in the following resources:
https://adrianroselli.com/2024/03/check-all-expand-all-controls.html
https://adrianroselli.com/demos/do-undo-all/
https://dequeuniversity.com/library/aria/checkbox-tri
https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/examples/checkbox-mixed/
(*the behavior in the w3.org example is odd and confusing since it remembers which checkboxes of the children were ticked for the indeterminate state across checkbox presses)At the moment, the favorite and preferred option for solving this issue is to go with a check-all button, according to #50 â . But based on the fact that the majority of resources we've found during the research are utilizing checkboxes to check other checkboxes, Drupal already using check-all checkboxes in other contexts within the admin UI, and us trying to keep the admin UI consistent not using different interface components for the same kind of functionality (see #51 â ). Unfortunately all these points are not in line with a check-all button. :( So we would recommend the following points:
- If the allowed number of fields is set to
1
on the field settings page of a select field, hide the settings for theCheck boxes/radio buttons
option on the corresponding form display widget. - Replace the check-all button with a checkbox. Visually distinguish the check-all checkbox from the checkboxes underneath by either indentation seen in the various link examples above or by adding a separation line/border.
- Replace
Show "Check all / none" button
withSelect all
as the label for the check-all checkbox. - Open a follow-up issue that is adding the tri-state checkbox pattern for Core in general. So an
intermediate
state is added to the already existingchecked
andunchecked
state for check-all checkboxes. Aside select fields on node edit forms we have found the following other places that are using this check-all checkbox pattern (there might be even more):admin/content
,admin/content/comment
,admin/content/media
,admin/content/media-grid
,admin/people
, and on a content moderation workflow page within the dialog modal forOperations
.
For the point mentioned in the beginning which is out of the scope for this issue I will open up a new issue. I'll remove the "needs usability review" tag and set the issue back to "needs work".
- Completely out of scope: While setting up the select field, weâve realized it is not communicated anywhere in the admin UI how to choose between checkboxes or radio buttons. The option on the form display widget is simply called âCheck boxes/radio buttonsâ, but it is not communicated anywhere that things are controlled via the cardinality/âallowed number of valuesâ-setting on the field settings page. A cardinality limited to one switches to radio buttons, while anything bigger than one or