backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

Selected yet disabled individual options from checkboxes element don't persist through save

Open klonos opened this issue 6 years ago • 13 comments

This is the respective issue for https://www.drupal.org/project/drupal/issues/2911473

The problem manifested itself here:

@BWPanda:

...I can't get the 'plain text' option to work properly. It keeps getting unset, despite being disabled.


PR by @klonos (backport of https://www.drupal.org/project/drupal/issues/2911473#comment-14046536): https://github.com/backdrop/backdrop/pull/2897

klonos avatar Sep 24 '19 20:09 klonos

Hmm, not sure if this works properly. I'm testing it with my PR from https://github.com/backdrop/backdrop-issues/issues/2615, and if I set the default values such that plain_text => 0, creating a new field correctly shows all formats enabled except plain_text (which is disabled and unchecked), but after saving and editing the field again, plain_text is still disabled but now checked...

Can you confirm @klonos before I mark this as 'needs work'?

ghost avatar Sep 25 '19 18:09 ghost

if I set the default values such that plain_text => 0

Sorry, I don't get this; why would you do that?

Anyway, here's what I've done before raising this issue, and filing the PR:

  1. cloned backdrop afresh
  2. pulled your PR from #2615
  3. set up my local
  4. research -> d.org issue -> backport the latest patch (on the same local environment)
  5. tested the problem where saving the form would save the plain text option with a value of 0 in config -> things were being saved properly, and the checkbox remained disabled + ticked 👍
  6. raised this ticket here -> added PR

So, here's my field.instance.node.page.body.json w/o the patch here:

    "settings": {
        "text_processing": "1",
        "allowed_formats": {
            "filtered_html": "filtered_html",
            "full_html": "full_html",
            "plain_text": 0
        },
        "display_summary": 1,
        "user_register_form": false
    },

...and with this PR:

    "settings": {
        "text_processing": "1",
        "allowed_formats": {
            "filtered_html": "filtered_html",
            "full_html": "full_html",
            "plain_text": "plain_text"
        },
        "display_summary": 1,
        "user_register_form": false
    },

klonos avatar Sep 25 '19 20:09 klonos

...another way to test this (or a workaround to the problem if you like), is to set the '#default_value' of the checkbox that is disabled to be the same as its value (following line where you disable the checkbox).

So change https://github.com/backdrop/backdrop/pull/2894/files#diff-eb50570cff15fd9fbac5df810e408e7aR121 to something like this:

  ...
  $fallback_format = filter_fallback_format();

  ...

  $form['allowed_formats'][$fallback_format]['#disabled'] = TRUE;
  // Workaround for https://github.com/backdrop/backdrop-issues/issues/4083. If
  // this is omitted, then the option for this checkbox is always saved as 0.
  $form['allowed_formats'][$fallback_format]['#default_value'] = $fallback_format;

klonos avatar Sep 25 '19 20:09 klonos

Sorry, I don't get this; why would you do that?

Ok, so here's a test module I wrote just for this issue: checkboxes_test.tar.gz

And here's how I used it to test this:

Default site (no PR):

  1. Install & enable module
  2. Go to /checkboxes-test
  3. Note checkbox status
    [x] Enabled, checked
    [ ] Enabled, unchecked
    [x] Disabled, checked
    [ ] Disabled, unchecked
    
  4. Submit form (without changing anything)
  5. Note new checkbox status
    [x] Enabled, checked
    [ ] Enabled, unchecked
    [ ] Disabled, checked
    [ ] Disabled, unchecked
    

(they shouldn't have changed from before the form was submitted, that's the goal here)

Now apply the PR/patch and test again:

Patched site (with PR):

  1. Uninstall module (to remove existing config)
  2. Install & re-enable module
  3. Go to /checkboxes-test
  4. Note checkbox status
    [x] Enabled, checked
    [ ] Enabled, unchecked
    [x] Disabled, checked
    [x] Disabled, unchecked
    
  5. Submit form (without changing anything)
  6. Note new checkbox status
    [x] Enabled, checked
    [ ] Enabled, unchecked
    [x] Disabled, checked
    [x] Disabled, unchecked
    

So it seems that the core issue is that checked, disabled checkboxes are being unchecked on save. This PR fixes that by making disabled checkboxes keep their default value on save, however it also sets unchecked, disabled checkboxes to checked, which is the problem.

ghost avatar Sep 26 '19 10:09 ghost

...however it also sets unchecked, disabled checkboxes to checked, which is the problem.

Gotcha! Thanks 👍

PS: I take it that in the previous-to-last code block in your last comment [x] Disabled, unchecked was meant to be [ ] Disabled, unchecked. Right?

klonos avatar Sep 26 '19 17:09 klonos

I take it that in the previous-to-last code block in your last comment [x] Disabled, unchecked was meant to be [ ] Disabled, unchecked

It's meant to be unchecked in terms of what's expected from the code, but what I wrote there (that it was initially checked) was correct in terms of what happened on the site (e.g. the PR incorrectly changed it to checked, even before submitting the form).

ghost avatar Sep 26 '19 17:09 ghost

...even before submitting the form

Gotcha 👍

klonos avatar Sep 26 '19 17:09 klonos

@klonos I found the issue and submitted a review requesting the fix.

ghost avatar Aug 10 '20 05:08 ghost

@BWPanda I've updated my PR with the latest patch provided in the d.org issue, and I'm gonna start testing things.

Having said that, we should be mindful of https://github.com/backdrop-ops/docs.backdropcms.org/issues/132 when using '#disabled' in forms 🤔

klonos avatar May 15 '21 13:05 klonos

@klonos you didn't update the labels after your last PR update. Is this ready for testing/review?

herbdool avatar Dec 30 '21 18:12 herbdool

I'll need to revisit this. Hope to do so soon.

klonos avatar Feb 17 '22 12:02 klonos

@klonos What else is needed here? I've tested the PR and it passes the 'checkbox test' ™️ , so marking this WFM.

ghost avatar Mar 16 '22 02:03 ghost

Changing to NW based on @klonos's last coment

jenlampton avatar Dec 02 '22 21:12 jenlampton

A bit of relevant insight from @quicksketch in https://github.com/backdrop/backdrop-issues/issues/2615#issuecomment-1530090559 (which I wasn't aware of):

...#disabled checkboxes don't submit a value, but you can force them by setting #value => TRUE.

klonos avatar May 01 '23 22:05 klonos