ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.66k stars 3.71k forks source link

Unlink Command Not Removing Data Attributes? #15872

Open matbk49751 opened 9 months ago

matbk49751 commented 9 months ago

Hey!

I crafted the following command to detach a text node. The intended outcome is to detach the text node and eliminate the associated data attributes. However, only the link is being removed, and the data attributes persist. I have specified the configuration in the schema provided below. Am I missing an Upcast converter for the removal of the attribute? Any insights on this issue?

Schema:

  init() {
    const editor = this.editor;
    const schema = editor.model.schema;
    const conversion = editor.conversion;

    schema.extend('$text', {
      allowAttributes: [
        'data-file',
        'data-uuid',
      ],
    });
}

Command:

 unlinkFileCommand() {
    const { editor } = this;
    const { model } = this.editor;
    const { selection } = model.document;

    // Execute the 'unlink' command to remove the link from the selected text.
    editor.execute('unlink');

    model.change((writer) => {
      let ranges;

      const attributeNames = ['data-file', 'data-uuid'];

      if (selection.isCollapsed) {
        ranges = [
          findAttributeRange(
            selection.getFirstPosition(),
            attributeNames,
            selection.getAttribute(attributeNames[0]),
            model,
          ),
        ];
      } else {
        ranges = model.schema.getValidRanges(
          selection.getRanges(),
          attributeNames,
        );
      }

      // Remove the extra attribute from specified ranges.
      for (const range of ranges) {
        attributeNames.forEach((attr) => {
          writer.removeAttribute(attr, range);
        });
      }
    });
  }
Witoso commented 9 months ago

Hey, sorry for the late reply! I didn't run the code, but the first thing I found is that getValidRanges takes a string, and you're passing an array. Second one, yes, you're missing any converter. Meaning, just extending the schema won't work. Think about it as a table definition in SQL. You still need to define a converter. In simple cases, attributeToAttribute should be enough.

matbk49751 commented 9 months ago

Hey, sorry for the late reply! I didn't run the code, but the first thing I found is that getValidRanges takes a string, and you're passing an array. Second one, yes, you're missing any converter. Meaning, just extending the schema won't work. Think about it as a table definition in SQL. You still need to define a converter. In simple cases, attributeToAttribute should be enough.

Hi Witoso!

No worries! Thanks for getting back. Would the below converters be able to handle the adding and removal of data-file and data-uuid?

conversion.for('upcast')
  .attributeToAttribute({
    view: {
      name: 'a',
      key: 'data-uuid'
    },
    model: {
      key: 'DataUuid',
      value: viewElement => viewElement.getAttribute('data-uuid')
    },
  })
  .attributeToAttribute({
    view: {
      name: 'a',
      key: 'data-file'
    },
    model: {
      key: 'DataFile',
      value: viewElement => viewElement.getAttribute('data-file')
    },
  })
  .attributeToAttribute({ // Remove data-type attribute
    view: {
      name: 'a',
      key: 'data-type'
    },
    model: false,
  });

conversion.for('downcast')
  .attributeToElement( {
    model: 'DataFile',
    view: ( attributeValue, {writer} ) => {
      const linkViewElement = writer.createAttributeElement('a', {
        'data-file': attributeValue
      }, { priority: 5 });

      writer.setCustomProperty( 'link', true, linkViewElement );

      return linkViewElement;
    },
  })
  .attributeToElement( {
    model: 'DataUuid',
    view: ( attributeValue, {writer} ) => {
      const linkViewElement = writer.createAttributeElement( 'a', {
        'data-uuid': attributeValue
      }, { priority: 5 });

      writer.setCustomProperty( 'link', true, linkViewElement );

      return linkViewElement;
    },
  });
Witoso commented 8 months ago

I don't see anything that could be wrong here at the first sight, I'm a bit lost on the use case though, so maybe I don't see something. As far as I can see, you want to create a links when those attributes are present?

matbk49751 commented 8 months ago

I don't see anything that could be wrong here at the first sight, I'm a bit lost on the use case though, so maybe I don't see something. As far as I can see, you want to create a links when those attributes are present?

Hi Witoso!

Correct. Create a links with those attributes and to have the ability to remove them. But, would this solve the removal?

  .attributeToAttribute({ // Remove data-type attribute
    view: {
      name: 'a',
      key: 'data-type'
    },
    model: false,
  });
DavidMCarek commented 7 months ago

Any update on this? I'm running into a similar issue where I'm pretty sure those attributes are causing the 2 step caret movement plugin to bug out as part of the link plugin. For context, when using the arrow keys I noticed some strange behavior where a duplicate a tag gets inserted to the front of a link when stepping out of the link and I think its caused by selection attributes still hanging around. So to remove those selection attributes the easiest option is to not have them show up on the model all together since they get added after save.

I've tried the above approach but those attributes are still showing in the model.