Changing CKEditor configurations by changing text formats causes markup to be lost: warn the user when performing this advanced action

Thanks for the detailed, constructive feedback. I really appreciate that :)

I can answer your questions, but you might not like the answers. Let's discuss this and figure out whether the CKEditor implementation in Drupal 8 should be changed because it is indeed an issue, or whether you have incorrect expectations.

1. Changing text format is destructive.

   When switching between formats it seems like disallowed tags are removed on switching. This is killing my UX. And this is not like Drupal 7 is it? The only fix is to reload the page.

   1. In Full HTML add a <pre>My pre text</pre>
   2. Now switch to 'Basic Format'
   3. Your <pre>My pre text</pre> is converted into <p>My pre text</p>
   4. Now switch to 'Full Html' and nothing happens.

   You're right. And this is intentional.

   The reason this filtering (in CKEditor) is being applied, is to ensure the output looks like expected. Without this, a user might think that his <p><font color="red">Important text</font></p> would also work on the front-end, even though it would be stripped away.

   See #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings for details.

   Text format switching was often abused in the past, e.g. to "preview" content in a WYSIWYG editor. But changing the text format has never been an action that was explicitly supported. That's also why in Drupal 8, 99% of users will only ever be able to use one text format: anonymous users can use "Restricted HTML", authenticated users can use "Basic HTML". So the "text format switching problem" is limited to administrators. Administrators should know better: switching text formats is a dangerous operation. You also can't switch from a Markdown to a BBCode to a HTML-based text format and expect everything to continue to work.

   In other words: yes, this experience will change, but only for 1% of users, and for good reason.

2. Backspace does not go to previous line

   This happened a few times but cannot reproduce right now.

   That would be a bug in CKEditor itself. I also can't reproduce this. If you can reproduce it again, please open a separate issue for it, and provide exact steps to reproduce :)

3. Pressing enter on the last character in a <pre>my text</pre> aka Formatted text format starts a new paragraph. It requires a shift-enter instead.

   You're the first to ever bring this up :) This is configurable using CKEDITOR.config.enterMode. Drupal uses the default, which indeed creates new paragraphs on pressing enter. This is the behavior most text editors on computers for decades have used. Shift-enter for soft return (remain within the same paragraph/block element) and enter for hard return (start a new paragraph/block element.)

I will address #1 here, please open new issues for #2 and #3 when you can reproduce them.

I agree that for certain scenarios, this might lead to frustration and/or despair. Of course I want to prevent that. An "undo" quickly enters tricky territory though. Since this will only affect advanced users, I think a warning when switching (and only when there's an actual danger) might be appropriate: A) it prevents the frustration/despair and B) it explicitly shows that hopping from one text format to another is in fact not a trivial or acceptable action, it should only be done with meticulous precision and understanding.

In other words: I propose a modal dialog that will only show up when you're switching to a text format that both has a text editor associated with it and that text editor supports "allowed content only" filtering. That way, if people use a different text editor than CKEditor, they won't get this message. (And those who want to, can simply disable CKEditor's ACF and alter the CKEditor text editor plugin's annotation, to remove these warnings.)
It looks like this (ignore the ugly styling, that's the Seven theme's responsibility, out of scope for this issue):

• Tagged "Usability", to get usability approval. Pinging Bojhan.
• Tagged "JavaScript", because I also refactored quite a bit of editor.js, since it wasn't up to the latest coding standards yet. Pinging nod_.
• Tagged "API addition", because this new text editor plugin annotation key is technically an API addition.

I can't really think of the despair scenarios, but its acceptable to have a message for this. The message is a little confusing. Its to bad we can't actually say, what is being removed - as that would inform the user much more, however I assume this is too much of an engineering challenge. Here is a stab at making it more concise.

Title = Text format: Full HTML text > Filtered HTML

Changing to Filtered HTML, will filter away disallowed content (tags). This might cause you to lose parts of your content.

"Continue" or "Cancel"

I can't install D8 HEAD at the moment, so I can't test this/hopefully it works… but this reroll adjusts the wording for better usability. Bojhan and I came up with this jointly in chat.

Core committer: please also credit Bojhan!

1. +++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorManager.php
   @@ -81,6 +82,7 @@ public function getAttachments(array $format_ids) {
            'editor' => $editor->editor,
            'editorSettings' => $plugin->getJSSettings($editor),
   +        'editorSupportsContentFiltering' => $plugin_definition['supports_content_filtering'],
          );
        }
   

   Is there a good default for supports_content_filtering in the case that it isn't defined for a plugin?

2. +++ b/core/modules/editor/js/editor.js
   @@ -3,11 +3,113 @@
   +    var confirmationDialog = Drupal.dialog('<div>' + Drupal.t('Changing the text format to %text_format will remove content that is not allowed in that text format. This might cause you to lose parts of your content.<br><br>You might want to save before continuing.', { '%text_format': $select.find('option[selected]').text()}) + '</div>', {
   

   Alternate text for "Changing the text format to %text_format will remove content that is not allowed in that text format. This might cause you to lose parts of your content.
   
   You might want to save before continuing."

   "Changing the text format to %text_format will permanently remove content that is not allowed in that text format.
   
   Save your changes before switching the text format to avoid losing data."

1. Every text editor plugin must define it. Most text editors would use FALSE. It's not an option to not define the full annotation. Imagine if every part of every annotation was optional: that would just not work :)
2. Okay, updated.

Also fixed tests.

JSHint error: var $select = $(this); should be var $slect = $(event.target);

If we really need a choice to be made as closeOnEscape: false shows we might want to be using the native confirm functionality of JS? On top of showing a confirmation it freeze everything like alert so we're sure nothing else is going on (and it removes like 30 lines of dialog configuration).

Also we might want to keep the close on escape and the close button and assume it is a cancel action. When they see the format didn't change they'll read the message.

Chatted with nod_ about this, we concluded that only fixing the JSHint error is enough.

jQuery UI Dialog implements the close on dialog logic in such a way that our code would have to listen for the close event and then check if it was a keyboard event and then check whether the ESC key was pressed. Leaky abstraction much. We decided that was not worth it, at least not for now.

The confirmation message lists the text format that the user is change from, not the one the user is changing to.

Hah! Great catch :)

Great, that satisfies my feedback and everyone else's was addressed in previous comments.

This puts an important check-step between the end user and lost data. Sure, we could probably take steps in the future to be better about not losing the data in the first place, but for now, this confirmation at least puts some friction between the user and disaster.

Buttons looks like crap, missing some classes maybe?

#17: no, this happens for *all* dialogs that are defined on the client-side. The only reason most dialogs look fine (e.g. Views or CKEditor's link/image dialogs) is because they're generated on the server side.
Try stopping in-place editing when you've made a change, the confirmation modal dialog you get then also looks like crap.

That's something Drupal.dialog will have to solve.

Umm, how about no?

Needs broader discussion, ignore #20 and back to RTBC

Ok, committed and pushed to 8.x, but lets get a follow-up for the dialog styling cos I agree that looks like pants.

Great catch on this btw.

There's already #2173129: Modal dialog style update for updating the dialog styling :)