Problem/Motivation

Editing content that contains the page break plugin will find it is removed.

Steps to reproduce

* Enable the page break plugin and add it to a text format alongside the "Limit allowed HTML tags and correct faulty HTML" plugin, which is enabled on core's default text formats.
* Create a node and add the page break to the text field; save the node.
* Edit the node.

What actually happens:
* The page break placeholders are removed.

What should happen:
* The page break placeholder is retained.

Proposed resolution

Fix the problem so that the page break plugin isn't removed when editing content that contains it.

Remaining tasks

Work out a fix for the problem.

User interface changes

Edit content where the page break plugin is used will retain the page break after editing.

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#7 ckeditor5_plugin_pack-n3456402-7.patch771 bytesdamienmckenna
#4 page-break-elements.patch678 bytesdolszewski

Issue fork ckeditor5_plugin_pack-3456402

Command icon 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:

About issue forks

Comments

DamienMcKenna created an issue. See original summary.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Related issues: +#3401513: Match the filter_html <> ckeditor5 integration in Drupal core

I suspect the problem is that the "Limit allowed HTML tags and correct faulty HTML" filter removes the HTML structure that the plugin creates.

I think this will be another scenario where core's "Limit allowed HTML tags and correct faulty HTML" filter always removes the "style" attribute, due to its security implications. The Extended HTML Filter module aims to replace core's filter with one that allows the "style" attribute, but it has problems, see #3401513: Match the filter_html <> ckeditor5 integration in Drupal core for details.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

FYI I suspect other plugins might also be affected by this.

dolszewski’s picture

dolszewski commented
StatusFileSize
new page-break-elements.patch678 bytes

Hi @damienmckenna

I've added a patch for this issue. It will not fix the problem completely but the

tag with class should not be removed anymore (probably you have to unselect and select again the "Limit allowed HTML tags and correct faulty HTML" filter.
In terms of style attributes, we can't do anything with that, Drupal does not allow adding style attributes (https://www.drupal.org/project/drupal/issues/3381471).
Any suggestion on how to change the style attribute into something else in the ckeditor5 page break plugin should be added to the official ckeditor5 repo on GitHub.
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Related issues: +#3381471: CKEditor 5: Restricting the style attribute in <span> tag has restricted some modules to work as expected.
damienmckenna’s picture

damienmckenna
Location TN, USA
commented

I think the best way forward would be to put some time into Extended HTML Filter and make it a required dependency for the submodules that require the "style" attribute.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs work
StatusFileSize
new ckeditor5_plugin_pack-n3456402-7.patch771 bytes

I tested patch #4 but it doesn't retain the "class" attribute, something is removing it. I think the definition also needs an entry for the span tag.

scott_euser’s picture

scott_euser commented

Thanks @DamienMcKenna, followed your rabbit hole of related issues. At the moment its a house of cards of patches to get this to work it seems. Most of the patches actually do no harm getting in, and facilitate things working once other bits get merged. I think in summary:

  1. Allow div class and span attributes - ckeditor5_plugin_pack module - #3456402: Page break plugin looses formatting when editing content - ie, this issue, your patch in #7. Ready to merge I think
  2. Fix validation constraint so that * actually does not complain about diffs - drupal core - #3401513: Match the filter_html <> ckeditor5 integration in Drupal core comment #17 but has no actual core issue. Workaround is to save the config manually
  3. Make extended html module match drupal core - extended_html_filter - #3401513: Match the filter_html <> ckeditor5 integration in Drupal core - Probably ready to merge there, but again requires workaround until (2) is fixed.
  4. <* style> is not accepted - drupal core - #3226368: FilterHtml accepts <*> but does not support it, resulting in inaccurate ::getHtmlRestrictions() return value - Not yet ready to merge but contributed to by multiple core maintainers, so we can be confident its going in the right direction at least and has a viable path to merge

So based on that, probably merging this one (1) and merging (3) do no harm and at least would prep these two modules for when core actually supports it, making it easier for people to use things like page breaks without all of the above steps. Do you agree? If so, I'll mark this as RTBC for ckeditor5_plugin_pack maintainers to review.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Related issues: +#3226368: FilterHtml accepts <*> but does not support it, resulting in inaccurate ::getHtmlRestrictions() return value

Thanks for digging into it, Scott. Yes, this is definitely a layered problem and bugs in core are affecting it.

scott_euser’s picture

scott_euser commented
Status: Needs work » Reviewed & tested by the community

Okay I suggest we merge this one given it does no harm and once other related issues get fixed and merged we will eventually have working page breaks :)

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Version: 1.1.0 » 1.2.0

This was partially added in the new 1.2.x branch, but it missed the SPAN tag change from #7.

Dear maintainers,
Please work with the issue queue instead of committing changes elsewhere and then pushing the repository to d.o. Thank you.

DamienMcKenna opened merge request !10

salmonek made their first commit to this issue’s fork.

salmonek’s picture

salmonek commented

The fix has been released in previous (1.2.2) release.
Thank you for the contribution.

@damienmckenna
We're using company repositories for tickets requested by management and bugs reported by QA since people across organization have access there and are familiar with it. In this module we will operate more on Drupal repositories as the internally induced work is mostly done.

salmonek’s picture

salmonek commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.