Text set with writer.insertText is getting removed when editing. / `button` in GHS malfunction

[tlo450]

I was unable to select the type:question label, so sorry for the incorrect label here.

So I have a Drupal site that is using ckeditor5, and I'm seeing an issue where text is getting removed when I go back in to edit. I have a custom plugin that adds in a <button> that contains a few attributes. For example, the HTML that gets stored in the editor looks like this:

<button data-align="align-right" data-href="/node/123" data-type="primary">Click me!</button>

I can toggle in between the source view and ckeditor view, and the 'Click me!' text remains.

However, if I save the page, then go back into the page to edit it, I check the 'source' again, and the 'Click me' text is now removed. It is replaced with a space: <button data-align="align-right" data-href="/node/123" data-type="primary">&nbsp;</button>

When the edit page first loads (before the ckeditor loads), I can see (for a split second) that my 'Click me!' text is there. However something is removing that text once the ckeditor5 js loads up.

I can't figure out why this is happening. It seems to be something with this code:

conversion.for('upcast').elementToElement({
  model: 'buttonPreview',
  view: {
    name: 'button',
  },
});

conversion.for('dataDowncast').elementToElement({
  model: 'buttonPreview',
  view: {
    name: 'button',
  },
});

// Convert the model into an editable <button> widget.
conversion.for('editingDowncast').elementToElement({
  model: {
    name: 'buttonPreview',
     attributes: [ 'ButtonAlign' ],
  },
  view: (modelElement, { writer: viewWriter }) => {
    const button = viewWriter.createEditableElement('button', {
      class: modelElement.getAttribute('ButtonAlign'),
    });
    return toWidgetEditable(button, viewWriter);
  },
});

Is there a way I can retain that 'Click me!' text?

If it's helpful, the code that adds in the 'Click me!' text looks like this:

function createButton(writer, attributes) {
  const button = writer.createElement('button');
  const buttonPreview = writer.createElement('buttonPreview', attributes);

  writer.insertText(attributes.ButtonText, buttonPreview);
  writer.append(buttonPreview, button);

  // Return the element to be added to the editor.
  return buttonPreview;
}

Anyone have any ideas?

Thanks!

[tlo450]

Seems like the issue is more specifically with the upcast conversion:

conversion.for('upcast').elementToElement({
  model: 'buttonPreview',
  view: {
    name: 'button',
  },
});

Not really sure how to re-work this code though to make sure the 'click me!' text is retained...

[dpagini]

I've actually ran into a similar problem as this. Following.

[lal65]

Exact same thing here trying to implement an accordion widget.

Changing the element type from button to span causes it to work, so it does seem to be related to the button-ness of the element type...

[lauriii]

Can you also post how does your schema for <button> look like so that it's possible to try to reproduce this? 🙏

[lal65]

Schema:

   const schema = this.editor.model.schema;

    schema.register('accordion', {
      isObject: true,
      allowWhere: '$block',
    });

    schema.register('accordionButton', {
      isLimit: true,
      allowIn: 'accordion',
      allowContentOf: '$block',
    });

    schema.register('accordionContent', {
      isLimit: true,
      allowIn: 'accordion',
      allowContentOf: '$root',
    });
  }

Insert Command:

function createAccordion(writer) {
  const accordion = writer.createElement('accordion');
  const accordionButton = writer.createElement('accordionButton');
  const accordionContent = writer.createElement('accordionContent');

  writer.append(accordionButton, accordion);
  writer.append(accordionContent, accordion);
  writer.insertText('Accordion label', accordionButton);
  writer.insertText('Accordion content', accordionContent);

  return accordion;
}

Upcast:

    conversion.for('upcast').elementToElement({
      model: 'accordion',
      view: {
        name: 'div',
        classes: 'accordion',
      },
    });

    conversion.for('upcast').elementToElement({
      model: 'accordionButton',
      view: {
        name: 'button',
        classes: 'accordion__button',
      },
    });

    conversion.for('upcast').elementToElement({
      model: 'accordionContent',
      view: {
        name: 'div',
        classes: 'accordion__expandable-content',
      },
    });

Downcast:

    conversion.for('dataDowncast').elementToElement({
      model: 'accordion',
      view: {
        name: 'div',
        classes: 'accordion',
      },
    });

    conversion.for('dataDowncast').elementToElement({
      model: 'accordionButton',
      view: {
        name: 'button',
        classes: 'accordion__button',
      },
    });

    conversion.for('dataDowncast').elementToElement({
      model: 'accordionContent',
      view: {
        name: 'div',
        classes: 'accordion__expandable-content',
      },
    });

[lauriii]

I wrote a test plugin to try to reproduce this and it seems like the upcast and downcast are working as expected. I'm wondering if there's something in the Drupal integration part that could be causing this. It would be helpful if someone could post a Drupal module where the bug can be reproduced.

[lal65]

I can link you to it after #dayjob, Lauri.

[lal65]

Actually, had 20 between meetings, here goes!

https://github.com/lal65/ooe_ckeditor5

Ultimately, I need to model this: https://psu-ooe.github.io/?p=viewall-molecules-accordion -- the upstream HTML structure cannot change.

/cc @lauriii

[tlo450]

@lal65 I haven't had a chance to look more into this, but I pulled down your code into Drupal and the issue you are seeing is the same thing that I reported when I opened up this issue.

When saving, the html is saved as this:

<button class="accordion__button">Accordion label</button>

when going back into the node (and checking the source), the html is now changed to this:

<button class="accordion__button">&nbsp;</button>

Just wondering if perhaps you have been able to figure out what is causing that to happen?

@lauriii for awareness.

[lal65]

@tlo450 - unfortunately not. I highly doubt that CKE5 will be able to support our design system based on what I've seen so far :-(. Maybe in the future when it's a bit more stable.

We'll probably go with a more heavy-handed approach that only uses ckeditor for very basic things like bolding text, applying classes to spans, etc...

[lauriii]

Sorry for not following through on this. I had some issues with my CKEditor 5 development environment when I tried to debug this and then once I got that resolved, I forgot that I promised to take a look at this. 😞 Either way, thank you @tlo450 for pinging me since that's how I ended up noticing this. 🙏

Good news is that I think I have an idea what is causing this. It looks like it's a General HTML Support related bug. Drupal registers lang attribute support for a list of supported HTML elements. This results in GHS registering a schema definition and a upcast converter for button. Besides these things, it also registers a raw content matcher which prevents other plugins from processing the children of button, or any other object elements.

I created a failing test case for this to make it easier to understand what is happening: https://github.com/lauriii/ckeditor5/commit/3be95a3f15edce86c9bd6bddc1196014a8e19309. If you modify the test to not register button with the data filter, the test should pass.

[lauriii]

I'm trying to think of side-effect free workaround to this problem but I'm having hard time with that. The simplest workaround is to tell GHS not to process button element, but this come with the side-effect that GHS won't process additional attributes for the button element. If that's okay, here's how that change would look like:

diff --git a/js/ckeditor5_plugins/accordion/src/accordionediting.js b/js/ckeditor5_plugins/accordion/src/accordionediting.js
index 9f4aaee..65a0f3c 100644
--- a/js/ckeditor5_plugins/accordion/src/accordionediting.js
+++ b/js/ckeditor5_plugins/accordion/src/accordionediting.js
@@ -18,7 +18,8 @@ export default class AccordionEditing extends Plugin {
   }
 
   _defineSchema() {
-    const schema = this.editor.model.schema;
+    const { editor } = this;
+    const schema = editor.model.schema;
 
     schema.register('accordion', {
       isObject: true,
@@ -36,6 +37,13 @@ export default class AccordionEditing extends Plugin {
       allowIn: 'accordion',
       allowContentOf: '$root',
     });
+
+    // If GHS is enabled, ensure that `button` is not being processed.
+    // @see https://github.com/ckeditor/ckeditor5/issues/13268
+    if (editor.plugins.has('DataFilter')) {
+      const dataFilter = editor.plugins.get('DataFilter');
+      dataFilter.disallowElement('button');
+    }
   }
 
   _defineConverters() {

[lal65]

Thanks, @lauriii - my only question at this point is:

Is this something that I can set up for a client and expect not to break moving forward?

[tlo450]

I've been taking another look at this again this week...

@lauriii what is interesting is originally it looked like this was a general html support related bug...but I noticed this is happening for non html elements as well. My original example uses a button (which obviously is a valid html element), however my project also has a ckeditor button that adds a custom <callout> (which is not a valid html element). This works fine in ckeditor4 though.

It behaves the same way as a button....it is placed into the ckeditor in this format:

<callout data-attr-a="test" data-attr-b="testing">callout text</callout>

And just like the button, if I go back into the source view and make any sort of edit in the editor itself, toggling out of source view and then back into source view will remove 'callout text' and replace it with a space  .

In my original post I mentioned I had to save the Drupal node first, and then go back in and make an edit to see this issue. However this can be reproduced without saving the node at all. When the <button> or <callout> is first placed, the text in between the tags will remain (I can toggle in and out of source view multiple times and it's fine). However, once I make any sort of edit (even if it's as simple as adding 1 space or letter anywhere in the editor) then the text between the tags is removed.

I guess in the end, the same thing is happening....it's just quicker to reproduce without having to save the Drupal node first.

I do need to keep the attributes in place so my buttons/callouts don't break when I move to ckeditor5. I'm just wondering if there are any other ways to get around this problem? The only thing I can think of is adding another attribute to the elements already placed in ckeditor4 that contain the text that already exists between the tags...not exactly ideal.

@lauriii Any other ideas? Thanks!

[rgpublic]

Came here from #15567. Okay, just to understand it better:

• This problem happens only in conjunction with Drupal
• The file responsible in Drupal is "assets/vendor/ckeditor5/html-support/html-support.js" (right?)
• Within this JS file, there is a special GHS configuration for some specific HTML elements
• This is the reason, why the problem occurs with some HTML elements and not with others
• It can be worked around by removing these elements from the GHS/DataFilter plugin
• GHS/DataFilter in Drupal is always enabled and the above-mentioned file is always loaded no matter what (correct? why? actually I don't want to allow all arbitrary HTML and there's even Drupal's allowed HTML tags filter/restriction on it)

[tlo450]

The simplest workaround is to tell GHS not to process button element, but this come with the side-effect that GHS won't process additional attributes for the button element.

@lauriii So this does fix the problem where the button text was disappearing (thanks for that!), However I need to retain the class attribute on my button. I'm having trouble figuring out a way to do that. Are you aware of any way other ways to keep those additional attributes?

[sascha-meissner]

Having the same issue, actually while trying to work on the same accordion module ;-)

@tlo450 Using the workaround from @lauriii i kinda figured out that you can retain attributes of the button when you dont explicitly set it as name property in your upcast, therefore you just need some attribute to match it, in my case a class name, at least for my class attribute it retains e.g:

conversion.for('upcast').elementToElement({
  model: 'accordionTriggerElement',
  view: {
    classes: 'ckeditor-accordion-trigger-element',
  },
});

instead of

...
view: {
   name: 'button'
   classes: 'ckeditor-accordion-trigger-element',
}
...

While having it set for the dataDowncast

conversion.for('dataDowncast').elementToElement({
  model: 'accordionTriggerElement',
  view: {
    name: 'button',
    classes: 'ckeditor-accordion-trigger-element',
  }
});

[lal65]

Very interesting, @sascha-meissner!

I wonder if this works by design, or if that's rather a happy accident that can/will break randomly in a future version?

I've been hesitant to implement any ckeditor5 customization for clients because it's unclear exactly how resilient to breakage a lot of these workarounds actually are.

[sascha-meissner]

@lal65 well i´m having a realy hard time trying to figure that out 😄 . At least syntactically it does´nt make much sense to me to disallowElement to be able to "use" that element and i would rather categorize this as ducktape.

IMHO it would make sense that when setting converterPriority: 'high', or similiar in my upcaster, that GHS doesnt try to apply its own models, but right now it seems to not be like that.

[tlo450]

@sascha-meissner Very clever! That does work for me, but one problem I'm still trying to solve using that method is how to dynamically get an attribute value to use as the class instead of hardcoding it.

For example I have something like this:

conversion.for('upcast').elementToElement({
  model: 'buttonPreview',
  view: {
    classes: 'class-here',
  },
});

conversion.for('dataDowncast').elementToElement({
  model: 'buttonPreview',
  view: {
    name: 'button',
    classes: 'class-here'
  },
});

Now I'm attempting to do some sort of getAttribute() to get the class name instead (similar to what I have below):

// Convert the model into an editable <button> widget.
conversion.for('editingDowncast').elementToElement({
  model: {
    name: 'buttonPreview',
    attributes: [ 'ButtonAlign' ],
  },
  view: (modelElement, { writer: viewWriter }) => {
    const button = viewWriter.createEditableElement('button', {
      class: modelElement.getAttribute('ButtonAlign'),
    });
    return toWidgetEditable(button, viewWriter);
  },
});

I would like the pass the value of the ButtonAlign attribute as the class instead of hardcoding it, however I'm having some trouble with the syntax here. Anyone have any ideas on how to do that via the upcast and dataDowncast?

@lauriii