Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When you remove all configurable plugins (ie. Heading, Source, Language) from the toolbar, the "CKEditor5 plugin settings" vertical tabs section (#plugin-settings-wrapper) disappears completely from the form. This makes sense as there are no settings to configure.
Now, when you place the Heading, Source or Language plugin back on the toolbar, the plugin settings remain hidden. I think this happens because the AJAX command tries to update the no-longer-existing #plugin-settings-wrapper element.
Steps to reproduce
- Enable CKEditor5 module.
- Create a new text format (/admin/config/content/formats/add).
- Choose CKEditor 5 as Text Editor.
- Remove all configurable buttons (ie. having a corresponding vertical tab under "CKEditor5 plugin settings") from the toolbar. For a new format, this is only the "Heading" plugin.
- Verify that the "CKEditor5 plugin settings" vertical tabs disappear from the form.
- Drag the "Heading" button back onto the toolbar.
- Notice how "CKEditor5 plugin settings" does not reappear. This is the bug.
Proposed resolution
T.b.d.
Remaining tasks
- Write a failing test
- Fix the bug and the test
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Not needed
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3259443-13-test-only.patch | 3.66 KB | bnjmnm |
| #12 | 3259443-test-only.patch | 3.66 KB | bnjmnm |
| |||
| #6 | 3259443-after_patch.gif | 32.91 MB | Abhijith S |
| #6 | 3259443-before_patch.gif | 15.04 MB | Abhijith S |
Issue fork drupal-3259443
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3259443-plugin-settings-element
changes, plain diff MR !1693
1 hidden branch
Comments
Comment #2
marcvangendComment #3
Wim LeersThis does not happen when adding buttons with configuration for the first time — if it did, our tests would be failing. So clarified the issue title for that 🤓
Great catch though! 👍
Comment #5
marcvangendI must say it isn't the most elegant fix. It works but I will not be offended if someone comes up with a better solution ;-)
Comment #6
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied MR 1693 and it fixes the issue.
Before patch:
After patch:
RTBC +1
Comment #7
Wim Leers@bnjmnm knows this code best, I think it'd be great if he could review it :)
Comment #8
lauriiiGetting review on this from @bnjmnm would be great, but I'm unassigning this to not discourage others from reviewing this.
Comment #9
Wim LeersComment #10
Wim LeersI think this should be a stable blocker; no intermediate state should cause this problem. This is different from #3259892: Removing all buttons from the toolbar should not be considered an error, where we're trying to save a CKEditor 5 configuration with a state that arguably makes no sense.
Comment #12
bnjmnmThis is looking good! Adding a test-only patch so there's objective confirmation the test targets this bug.
Comment #13
bnjmnmdidn't format the drupalci.yml edit properly in #12
Comment #15
bnjmnmThe test only patch in #13 demonstrates the test is doing it's job, as is the fix!
The solution is straightforward - create an empty
#plugin-settings-wrapperdiv if$form['editor']['settings']['subform']['plugin_settings']doesn't exist, so it is always available to the AJAX callbacks expecting it.Comment #16
Wim LeersYay, thanks @bnjmnm! 🚢
And thank you especially, @marcvangend!
Comment #18
Wim LeersThe merge request is RTBC, not the patch.
Comment #22
lauriiiCommitted 7a1c45f and pushed to 10.0.x. Also cherry-picked to 9.4.x and 9.3.x because CKEditor 5 is experimental. Thanks!