Allow <details> element to be placed without <div>

We've been using this patch in some of our production sites for a few months now with no issues. Just tested it again with version 2.0.3 against drupal/core:9.4.5, works as intended.

The patch seems to change the indentation, which makes it difficult to see what code changes are actually being proposed. Can we get an updated patch that doesn't change the indentation? Also, it would be useful to know if the same changes are needed for the CKEditor5 version of the plugin introduced in the 2.1.x branch.

Patch for CKEditor 5 (only) support

I've tested 3244311-6.patch, running into a few issues.

The order of the elements changes on load of the editor.
To reproduce:

1. Place a details element in the editor
2. Add multiple elements (<p>) to it
3. Change to 'source editing'
4. Add some characters
5. Change back to normal editing
6. See the order of your items has changed.

Video demonstrating the issue (also attached)

Some elements get stripped from the content of the details, such as heading elements.
To reproduce:

1. Place a details element in the editor
2. Add a heading in the content of the editor
3. Change to 'source editing'
4. Add some characters
5. Change back to normal editing
6. See the heading has been converted to a <p>

Video demonstrating the issue (also attached)

Please note that this not only happens when switching from source editing to regular editing, also when re-opening the edit page after saving. Without switching in between the values in the database are 'correct' but get swapped when opening the editor again. Headings are still allowed, just disappear when opening the editor again.

I tested patch #6, was running into the same issues reported in #7. In my case, I have a large migration, and migrated <details> elements didn't have the contents wrapped in a <div>. This would result in the plugin removing all contents of the detailWrapper except the <summary> element.

My new patch extends #6 and fixes the reported issues in #7. Now the migrated content works as expected and the plugin handles the markup correctly.

Outline of changes

Upcast Converter

Introduces explicit handling of <summary> using convertItem to differentiate between <summary> and other child elements. This ensures <summary> maps to detailSummary and all other children map to detailWrapper, maintaining the correct order.

Data Downcast Converter

Explicitly removes <div> from the serialized output by using createSlot for detailWrapper. Adds logic to serialize <summary> first and then process all other child elements of detailWrapper in their original order.

Editing Downcast Converter

Dynamically appends child elements to the editable details-wrapper div in their original order, ensuring no reordering during editing.

I just tested a complex details element containing <p> tags, <div> tags, h2 heading, and nested <details> elements and the changes seem to be working as expected.

I've tested #9 with drupal/core:10.3.7 and drupal/ckeditor_details:2.1.0-beta1.

The issues mentioned in #7 are resolved. I've created a details element with a few different tags and they all stayed in the expected order after switching between the 'source editing' modus.

Back to RTBC.

Need this patch again on a upgrade from D9-D10. Existing details content is not wrapped in a single <div>, so without this patch all body content of the details element is removed in node edit for CKE5.

Thanks a lot everyone for the great work on this issue!

Quick follow-up on #5:

I've created an initial merge request above at #13 for the 2.1.x branch, based on the patch above at #9:

• Created the branch / issue fork from the links at the top of this issue/page based on 2.1.x.
• Checked out locally and applied patch from #9 without error.
• Recompiled assets locally: build.js with webpack.
• Committed and pushed the changes to issue fork.

 
I have tested a bit the changes myself locally and they seemed to work pretty well, great job! 👍
 
One thought though:
Could this be something we would like to be able to configure? Enable/disable, for example, or maybe select a tag (from a dropdown list)?
Or do you it be fine to just switch over to use the <p> tag?
 
Therefore, at this point, moving back issue to Needs review, as an attempt to collect more feedback, comments and reviews.
 
Feel free to let us know if you have any questions, suggestions or concerns on any aspects of the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! 🙂

Hi
have manually tested MR 9 on Drupal 2.1.x version.
The MR is applied Successfully...

The CKEditor 's detail element is able to place without

Attaching screenshot for reference

RTBC+1

@dydave this should not be configurable, we should just allow any markup inside the <details> element. In a few of my projects where we have migrated content, we have <details> elements with different elements inside, like <p>, <ul>, <div>, etc... And this is valid markup so we should allow this. By default, if you create a <details> element in CKE5 with this patch, and type some text, it will wrap in a <p> tag as expected.

Need patch #9 on yet another project. Client is used to entering HTML manually in CKE4, but complains that in CKE5 the contents of their details element are being stripped. I told them a workaround is to add their content within a <div class="details-wrapper"> but they were keen to get this patch included so they can continue to author with valid markup as they did in CKE4.

Can we get this included in a new release? I have used this patch on 3 projects now and it's working as expected.

Patch #18 contains a snapshot of MR!9 containing the recompiled assets

The div wrapper was intentionally added in #3159065: Wrap details content in a div.details-wrapper to match the expected markup by drupal/collapse to match the markup expected by the shim in Drupal core. I recognize that was years ago and caniuse now lists browser support for details elements at around 96%. TBH I'm not sure if core even contains this shim anymore, but keeping the shim should allow details elements created by this module to match any created by core.

I would personally lean towards making this configurable, but I'm also open to other perspectives.

Thanks Martin (@mandclu)!
Same comment I had above at #14.

Stephen (@smulvih2) is this something that could be added to the MR?

Stephen, this would perhaps allow a smoother/easier integration of this feature to the module by allowing to keep the existing/current behavior, while still being able to extend/override with specific site configurations.

Thanks again everyone for the great work on this feature!

From my experience on the three projects noted above, completely removing the wrapping <div> elements fixes all issues across the following scenarios:

• Details element created in CKE4, loaded into CKE5
• Details elements created with CKE5 pre-patch, loaded into CKE5 post-patch (contains the wrapping div)
• Details elements migrated from other platforms, where the body is made of multiple elements all nested directly in the <details> element (p, div, h2, ul, etc...)

From testing on these projects, no issues reported across browsers. I have used this patch on both Drupal 10 and Drupal 11.

I'm not sure what benefit it would give to allow the nesting under the single div again as an option. Maybe we can get this into an alpha/beta version and get some feedback from users? Not sure of best way to move forward besides keep using this patch on my projects :)

CKEditor was failing to load due to CSPs because patch #9 replaced js/build/detail.js with a webpack build that uses eval(...).

Our Content Security Policies does not allow 'unsafe-eval', and CKEditor’s docs say self-hosted CKEditor 5 should work with script-src 'self' without requiring it.

Patch #24 keeps the same Drupal behavior change, but rebuilds detail.js without eval(...).

Rerolled.

I have added the eslint fixes from #3611707: Fix issues raised by eslint. That issue should be merged before this. It has now been merged. The eslint checks are now passing.

MR !9 is working for me. We had a similar situation as #9 where we migrated in a large amount of these elements and won't have the requisite wrapper and enabling this without the patching strips the details altogether. Thanks! RTBC to me...

Thanks everyone!