backdrop-issues
backdrop-issues copied to clipboard
Selected yet disabled individual options from checkboxes element don't persist through save
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
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'?
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:
- cloned backdrop afresh
- pulled your PR from #2615
- set up my local
- research -> d.org issue -> backport the latest patch (on the same local environment)
- tested the problem where saving the form would save the plain text option with a value of
0in config -> things were being saved properly, and the checkbox remained disabled + ticked 👍 - 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
},
...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;
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):
- Install & enable module
- Go to
/checkboxes-test - Note checkbox status
[x] Enabled, checked [ ] Enabled, unchecked [x] Disabled, checked [ ] Disabled, unchecked - Submit form (without changing anything)
- 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):
- Uninstall module (to remove existing config)
- Install & re-enable module
- Go to
/checkboxes-test - Note checkbox status
[x] Enabled, checked [ ] Enabled, unchecked [x] Disabled, checked [x] Disabled, unchecked - Submit form (without changing anything)
- 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.
...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?
I take it that in the previous-to-last code block in your last comment
[x] Disabled, uncheckedwas 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).
...even before submitting the form
Gotcha 👍
@klonos I found the issue and submitted a review requesting the fix.
@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 you didn't update the labels after your last PR update. Is this ready for testing/review?
I'll need to revisit this. Hope to do so soon.
@klonos What else is needed here? I've tested the PR and it passes the 'checkbox test' ™️ , so marking this WFM.
Changing to NW based on @klonos's last coment
A bit of relevant insight from @quicksketch in https://github.com/backdrop/backdrop-issues/issues/2615#issuecomment-1530090559 (which I wasn't aware of):
...
#disabledcheckboxes don't submit a value, but you can force them by setting#value => TRUE.