Allow tableselect element options to be disabled

+1 for this

For some reason, the attributes of the table get put on the checkbox in the processTableselect function, things like classes etc.
eg:

$element[$key] = [
            '#type' => 'checkbox',
            '#title' => $title,
            '#title_display' => 'invisible',
            '#return_value' => $key,
            '#default_value' => isset($value[$key]) ? $key : NULL,
            '#attributes' => $element['#attributes'],
            '#ajax' => isset($element['#ajax']) ? $element['#ajax'] : NULL,
          ];

Should it not be something like this?

$element[$key] = [
            '#type' => 'checkbox',
            '#title' => $title,
            '#title_display' => 'invisible',
            '#return_value' => $key,
            '#default_value' => isset($value[$key]) ? $key : NULL,
            '#attributes' => (isset($choice['#attributes'])) ? $choice['#attributes'] : [],
            '#ajax' => isset($element['#ajax']) ? $element['#ajax'] : NULL,
          ];

For a tableselect I needed to implement with disabled options, I added a custom process function to the tableselect render array and just copy and pasted the original processTableselect and made these changes, it worked for me.

I've opened #3026624: Disabled rows in tableselect elements but, at that moment, I was not aware about this ticket. Yes, #3026624: Disabled rows in tableselect elements is a duplicate of this one. Let's move the work here as this is older.

Done:

• Updated IS.
• Moved the issue under "render system" component.
• Moved the patch from #3026624: Disabled rows in tableselect elements, as that one is polished and contains also tests.

Adding a workaround in IS for anyone interested.

And, this is a good candidate for backporting.

Needs also a change record.

Looks like a random failure.

Tested the patch on Drupal 8.6.7, works like a charm.

@strakkie, glad to hear. Could please also do a review of the code from #8?

the "attributes" for the table are still being put on the checkbox. Which is why I did it the way I did in #7.
Surely you want to be able to put attributes on the table and not have the exact same attributes on the checkboxes.
The way I did it allows for specifying attributes for the table itself and each individual checkbox (including disabled).
Maybe I missing something?

+++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
@@ -254,6 +257,7 @@ public static function processTableselect(&$element, FormStateInterface $form_st
+              '#disabled' => isset($element['#options'][$key]['#disabled']) ? $element['#options'][$key]['#disabled'] : FALSE,

Will this be a bit simpler?
+ '#disabled' => !empty($element['#options'][$key]['#disabled']);

Also we may put some handy CSS class on disabled rows to ease styling.

@2pha, the original title of the issue is Allow to disable a Tableselect option and the actual Disabling tableselect element options. IMHO, the #attributes issue should be split in a separate issue. Let's not solve all the problems here.

@Chi, thank you, good remarks. Fixed in this new patch.

Sorry, forgot in an unused statement.

Oh, and the XPath has to check for disabled="disabled" attribute.

@claudiu.cristea I disagree that attributes should be separate to this.
disabled is an attribute.
If this patch is applied, then later the attributes are fixed, this patch will have to be undone.
Might as well do the other attributes at the same time.
It's also going to be confusing if 1 attribute can be set on a checkbox, but the others can't.

If this patch is applied, then later the attributes are fixed, this patch will have to be undone.

I don't think supporting attributes will make this patch obsolete. #disabled property is not just for setting HTML attribute. Drupal form API applies special treatment for elements with such property.

https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

Setting #disabled to TRUE results in user input being ignored, regardless of how the element is themed or whether JavaScript is used to change the control's attributes.

However with this path it does not work for me. If I remove the disabled attribute manually through Chrome inspector and then
check/uncheck the checkbox the new value is accepted on form submission. This does not happen on checkboxes that are outside tableselect element.

@Chi, isn’t that a normal behaviour? That would allow JS to enable/disable checkboxes in the tableselect, on client side, and allow the user to submit them.

isn’t that a normal behaviour?

I do not know. But that behavior differs by some reason from the one used for checkboxes outside table select.

A note from FormBuilder::doBuildForm() documentation.

As a security measure, user input is used for an element's #value only if the element exists within $form, is not disabled (as per the #disabled property), and can be accessed (as per the #access property, except that forms submitted using self::submitForm() bypass #access restrictions).

And from form builder itself.

With JavaScript or other easy hacking, input can be submitted even for elements with #access=FALSE or #disabled=TRUE. For security, these must not be processed. Forms that set #disabled=TRUE on an element do not expect input for the element, and even forms submitted with self::submitForm() must not be able to get around this.

Well, #checkboxes element also set values for disabled elements. Given that TableSelect is just a styled form of Checkboxes I think it should be ok.

As of setting attributes I think we could take the same approach as in Checkboxes.php.
This would allow to gain full control on child elements.

See #1349432: Mention checkboxes/radios options pre-setting in forms_api_reference.html.

1. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -33,9 +33,13 @@
   + * Selecting a row could be prevented by adding a boolean #disabled property set
   + * to TRUE, just like in the 3rd row from the above example. This property is
   + * ignored when #multiple is FALSE.
   + *
   

   I think this can be improved. Something like Prevent users from selecting a row by adding a #disabled property set to TRUE. This property is ignored when #multiple is FALSE.

   Also we're in a code block so this should be a PHP comment. Or needs to be moved outside the the @code block.

2. Why is this option ignored when #mutliple is FALSE? Also I'm pondering about the UX implications of this change. I think there should be some screenshots so people can see what this looks like.

3. +++ b/core/tests/Drupal/KernelTests/Core/Element/TableselectTest.php
   @@ -0,0 +1,94 @@
   +/**
   + * Tests the tableselect element.
   + *
   + * @group Form
   + */
   +class TableselectTest extends KernelTestBase implements FormInterface {
   

   I think this should be added to either \Drupal\Tests\system\FunctionalJavascript\Form\ElementsTableSelectTest or \Drupal\Tests\system\Functional\Form\ElementsTableSelectTest

   Kernel tests are not really meant for testing UX improvements like this.

Addressed all from #29, fixed the IS, added screenshots, added change notice, added release note snippet.

Unrelated failure, does now pass.

#31 works well for me.

Sorry for the delay in getting feedback on this. My first thought is that it's hard to imagine what this is going to be used for. Would it be possible to get an explanation of the use case where this is needed?

This seems like a potentially confusing UX pattern, which I think we should validate with a usability expert. We should also get an accessibility review from someone on this.

Came here to drop our potential use case for this, as we are using already similar list that shows subordinates of supervisor, who can perform actions for selected ones.

We would have some use cases for this already, but if it would work with #states as well, then we could do the disabling according to the selected action.

As a more general example for this would be in People-page, where you see bunch of users, and you can perform various actions for them. For example if you select "Block user" from the dropdown menu, it could disable out those users that have already been blocked. This probably isn't needed in many cases, as blocking blocked users is not likely to cause any harm. However if such user would get notification of some sort, it could be confusing to get it again later.

@rbosscher well our use cases seem to be quite close the same, I just tried to explain it with example everyone could see and feel. :)

In our exact case where this would be super helpful is when supervisor is going to enroll subordinates to training by bulk operation, supervisor does already see the status of each subordinate in the list whether they have already enrolled or not, we also could filter out those subordinates from that list who already have enrolled, but we've the exact same conclusion that it wouldn't be clear why some of the subordinates isn't showing up on the list.

The way I see this could help up the supervisors is that they wouldn't have to check line by line if subordinate has indeed enrolled already.

Also the re-enroll is actually intended option in our case, but it does also mean that enrolled person gets new position in case there's queue for that training and doing this accidentally could be harmful, as well as new mail from the status change etc. For the list I mentioned earlier, it would be fine even if the re-enroll option would be blocked out completely, but if it would support #states, there could be two different bulk operations, one for enrolling and one for re-enrolling, and that way one wouldn't accidentally (at least not that likely) someone they didn't mean to.

@lauriii,

My first thought is that it's hard to imagine what this is going to be used for. Would it be possible to get an explanation of the use case where this is needed?

I don't see what could be confusing here. There are tons of UIs where you'll find list of checkboxes where some are disabled. Simple lists or in table rows. I can show a lot of use cases. For me it's obvious: Don't allow the user to select some options because, as an effect of form business logic, but let him see that the options exist and might be available in other context..

This seems like a potentially confusing UX pattern,

Could you explain why this would be confusing? Don't we have disabled form elements all around? The fact that the user should be aware about why some rows are disabled is out-of-the scope here. That should be accomplished in a very specific way, given the business logic of each form where the element is used:

• In some cases there will be a message on top of the table.
• Other cases will prefer a tooltip/balloon when hovering.
• Or just add an explanation on other column of the same row.
• Etc, etc, etc.

Reroll for 8.8.x

On 8.9.10 applied patch #42. This is working great for my use case!

Patch #42 works for me on 8.9.13. Thanks a lot!

Patch #42 works for me on 9.3.0. Thanks!

Was frustrated to find this didn't exist.

As a use case, in the Marketo module it has a page that lists all the fields defined in your marketo instances and then allows you to enable a subset of them to available for various sync and activity integrations. I wants to _list_ all fields because you need to be able to see that the field was visible to the integration and allow the admin to see various other details about the field, but it also convey that some fields can't be selected for a write list because marketo has them flagged as read only.

I think I agree with claudiu. I'll go a bit farther and say I don't think there's an inherent in-accessible behavior associated with doing this. Any _use_ of it should probably review how it might end up causing a problem but the patch just exposes a tool. We already allow checkboxes to be disabled, we're just exposing that functionality in bulk.

In fact it turns out you can kinda break tableselect today to "disable" rows today. Found this hack in the current module's code.

    foreach ($this->service->getReadOnly() as $field_key => $field) {
      // This doesn't actually disable it, the existence of a value at the location breaks the checkbox generation.
      $form['field_enabled_fields'][$field_key]['#disabled'] = TRUE;
    }

There's probably some other way for hacking the theme system to mangle it in place too. So developers gonna develop and at this point I think we've just made this arbitrarily hard for developers.

This patch worked perfect for providing the expected functionality so marking it back to RTBC.

A little further googling, the code above is probably related to this discussion on how to accomplish this functionality in Drupal 7.

https://api.drupal.org/api/drupal/includes%21form.inc/function/theme_tab...

Patch still works (I'm testing on 9.2.10). Are there any blockers for this?

re-apply around #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator. Nothing scary though surprised the re-apply checks didn't catch it the failure.

Looking back through the comments, I'm not sure I agree with #29.3. If I'm reading the test right, we install a site and setup some routing boiler plate so we can run the same assertions after they've gone over the network? Seems like a lot of heavy lifting that's not doing anything.

Anyways, agree this is probably RTBC just not sure that change was needed.

Looks like a Composer 2.2 issue

Unrelated bot failure

bot flux

bot

Moving to 10.0.x and testing.

Why is this moved to 10? Shouldn't new features go in 9.5.x (and 10)? Tentatively, moving back to 9

It seems that 9.5.x doesn't accept new features (?) Moving back to 10

Committed and pushed 8a36ace53d to 10.1.x and 9bc14dc2ba to 10.0.x and 00567fe61e to 9.5.x. Thanks!

Backported to 9.5.x because this new feature is small and only and addition and well tested.

Fixed help test to be consistent with the rest of the example

diff --git a/core/lib/Drupal/Core/Render/Element/Tableselect.php b/core/lib/Drupal/Core/Render/Element/Tableselect.php
index 4b8d525edd..6258500c00 100644
--- a/core/lib/Drupal/Core/Render/Element/Tableselect.php
+++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
@@ -35,7 +35,7 @@
  *   2 => ['color' => 'Green', 'shape' => 'Square'],
  *   // Prevent users from selecting a row by adding a '#disabled' property set
  *   // to TRUE.
- *   3 => ['first_name' => 'Super', 'last_name' => 'Man', '#disabled' => TRUE],
+ *   3 => ['color' => 'Blue', 'shape' => 'Hexagon', '#disabled' => TRUE],
  * ];
  *
  * $form['table'] = array(

Following on from #3265574: Convert remaining assertions involving use of xpath to WebAssert, where possible converted use of xpath and ran test locally.

diff --git a/core/modules/system/tests/src/FunctionalJavascript/Form/ElementsTableSelectTest.php b/core/modules/system/tests/src/FunctionalJavascript/Form/ElementsTableSelectTest.php
index a4fb109f44..2cfb76dbd0 100644
--- a/core/modules/system/tests/src/FunctionalJavascript/Form/ElementsTableSelectTest.php
+++ b/core/modules/system/tests/src/FunctionalJavascript/Form/ElementsTableSelectTest.php
@@ -66,16 +66,14 @@ public function testAjax() {
   public function testDisabledRows() {
     // Asserts that a row number (1 based) is enabled.
     $assert_row_enabled = function ($delta) {
-      $rows = $this->xpath("//table/tbody/tr[$delta]");
-      $row = reset($rows);
+      $row = $this->assertSession()->elementExists('xpath', "//table/tbody/tr[$delta]");
       $this->assertFalse($row->hasClass('disabled'));
       $input = $row->find('css', 'input[value="row' . $delta . '"]');
       $this->assertFalse($input->hasAttribute('disabled'));
     };
     // Asserts that a row number (1 based) is disabled.
     $assert_row_disabled = function ($delta) {
-      $rows = $this->xpath("//table/tbody/tr[$delta]");
-      $row = reset($rows);
+      $row = $this->assertSession()->elementExists('xpath', "//table/tbody/tr[$delta]");
       $this->assertTrue($row->hasClass('disabled'));
       $input = $row->find('css', 'input[value="row' . $delta . '"]');
       $this->assertTrue($input->hasAttribute('disabled'));
@@ -99,8 +97,7 @@ public function testDisabledRows() {
     $assert_row_enabled(3);
 
     // Table select with checkboxes allow selection of all options.
-    $checkboxes = $this->xpath('//table/thead/tr/th/input');
-    $select_all_checkbox = reset($checkboxes);
+    $select_all_checkbox = $this->assertSession()->elementExists('xpath', '//table/thead/tr/th/input');
     $select_all_checkbox->check();
 
     // Check that the disabled option was not enabled or selected.