Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom inline element disappear after setData() if it is the only or first element in tableCell #6698

Closed
marcofanch opened this issue Apr 28, 2020 · 5 comments · Fixed by #7787
Assignees
Labels
squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@marcofanch
Copy link

marcofanch commented Apr 28, 2020

I target to create a plugin to insert FontAwesome icon to the editor.

(I understand this is not a bug, but my lack of knowledge on the editor, anyhow when creating the ticket "bug" was the closest option from the list.)

Code

// fontsymbolediting.js
// init()
    //.....

    schema.register('fontSymbol', {
      allowWhere: '$text',
      isInline: true,
      isBlock: true,
      isObject: true,
      allowAttributes: ['name'],
    });

    conversion.for('dataDowncast').elementToElement({
      model: 'fontSymbol',
      view: (modelItem, viewWriter) => {
        console.log('dataDowncast fontSymbol')
        const symbolName = modelItem.getAttribute('name');
        const symbolColor = modelItem.getAttribute(FONT_COLOR);

        const symbolElement = viewWriter.createUIElement(
          'i',
          {
            class: `m-font-symbol fas fa-${symbolName}`,
            style: `color: ${symbolColor}`,
          },
        );

        return symbolElement;
      },
    });

    conversion.for('downcast').elementToElement({
      model: 'fontSymbol',
      view: (modelElement, viewWriter) => {
        console.log('editingDowncast fontSymbol')
        const symbolName = modelElement.getAttribute('name');
        const symbolColor = modelElement.getAttribute(FONT_COLOR);

        const symbolElement = viewWriter.createUIElement(
          'i',
          {
            class: `m-font-symbol fas fa-${symbolName}`,
            style: `color: ${symbolColor}`,
          },
        );

        const viewWrapper = viewWriter.createContainerElement('span', {
          class: 'm-font-symbol-container',
          name: symbolName,
        });
        viewWriter.insert(viewWriter.createPositionAt(viewWrapper, 0), symbolElement);

        return toWidget(viewWrapper, viewWriter);
      },
    });

    conversion.for('upcast').elementToElement({
      view: {
        name: 'i',
        classes: ['m-font-symbol'],
      },
      model: (viewElement, modelWriter) => {
        const cssClasses = viewElement.getAttribute('class') || '';
        const name = (cssClasses.match(/fa-([\w-]+)/) || [])[1];

        if (!name) {
          return null;
        }

        const fontSymbolModel = modelWriter.createElement('fontSymbol', { name });

        return fontSymbolModel;
      },
    });

I took reference from the horizontal rule plugin, everything worked fine until I saved the data and reloaded it into the editor.

The icon was not showing up.

upcast() was called, model element returned, but the element did not appear.

I looked into table cell, and realized that table cell accept only block element.

I cannot find any example of putting inline element inside table cell.

However, if I pad an empty space before the icon, it can display properly, with the padded space of course.

✔️ Expected result

The inline icon appears in the table cell after setData()

❌ Actual result

Nothing shows up in the table cell

📃 Other details

  • Browser: Chrome
  • CKEditor version: 18
  • Installed CKEditor plugins: font, table
@marcofanch marcofanch added the type:bug This issue reports a buggy (incorrect) behavior. label Apr 28, 2020
@Reinmar
Copy link
Member

Reinmar commented May 18, 2020

Hi!

There are a couple of things that I spotted that can cause issues:

  • In the schema, you marked this element to be a block and inline one at the same time. It can't be both. In this case, inline would be the way to go.
  • You're downcasting this element to a UIElement (I mean dataDowncast here). That's unsafe and may not work. Every model element must be mapped to something in the view. While, a UI element is treated as "nothing". It's an element that's considered to be zero-length content. Its purpose is to display UI, not content itself. It could only be used inside a container element (check out the media embed feature for an example or https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/using-react-in-a-widget.html). Your downcast definition is hence correct – it uses a UIElement inside a container element. The model element maps to that container element and alls fine.
  • You have data downcast and then you have the general downcast. That's at least confusing. You should either have editingDowncast + dataDowncast or just downcast if the format is identical in both pipelines.

IDK if any of these is the reason of your problems, but you could try resolving them and see.

@Reinmar Reinmar added the pending:feedback This issue is blocked by necessary feedback. label May 18, 2020
@stu43005
Copy link

stu43005 commented Jun 5, 2020

I also encountered a similar problem.

I made a placeholder example on the documentation:
https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html

Insert a placeholder in the table:
image
getData(): <figure class="table"><table><tbody><tr><td><span class="placeholder">{date}</span></td></tr></tbody></table></figure>

Then I saved the data and reload it into the editor:
image
getData(): <figure class="table"><table><tbody><tr><td>{date}</td></tr></tbody></table></figure>

If insert a space before the placeholder, everything will be OK:
image
getData(): <figure class="table"><table><tbody><tr><td>&nbsp;<span class="placeholder">{date}</span></td></tr></tbody></table></figure>

@Reinmar Reinmar removed the pending:feedback This issue is blocked by necessary feedback. label Jul 16, 2020
@Reinmar Reinmar added this to the nice-to-have milestone Jul 16, 2020
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:red labels Jul 28, 2020
@niegowski niegowski self-assigned this Jul 29, 2020
@niegowski niegowski modified the milestones: nice-to-have, iteration 35 Jul 29, 2020
jodator added a commit that referenced this issue Aug 7, 2020
Fix (engine): Upcast conversion will now try to wrap text and inline elements in paragraph if such content is converted in a place where is not allowed but a paragraph is allowed. Closes #7753. Closes #6698.

Internal (paragraph): Auto-paragraphing content conversion moved to the engine package.

Tests (table): Added tests for upcasting inline elements inside a table cell.
@Reinmar
Copy link
Member

Reinmar commented Aug 10, 2020

We fixed the problem and the patch will be included in the next release (planned around the end of August).

Thanks!

@MrMitch
Copy link

MrMitch commented Aug 10, 2020

@Reinmar great news, thank you for the work put into this ! Looking forward to this.

@mhsdesign
Copy link

mhsdesign commented Sep 29, 2022

I happen to be working with the older Ckeditor version 16 (this bug was fixed with 22) and i cant upgrade.
But luckily i found out that there was a "hacky" idea (option 2) for a few seconds as WIP implemented which is ideal for me ^^

just add this as a fix and i think youre good to go ;)

/**
 * "Decorates" splitToAllowedParent() to extend it with auto-paragraphing
 * 
 * CkEditor before v22 has this bug: https://github.com/ckeditor/ckeditor5/issues/6698, https://github.com/ckeditor/ckeditor5/issues/7753
 * 
 * to fix it we use the second proposal of https://github.com/ckeditor/ckeditor5/issues/7753
 * which was implemented as WIP here https://github.com/ckeditor/ckeditor5/pull/7787/commits/56ff10c5e9cc24f3c3397a97f7927c1708e00581#diff-fa5058ba57a9a2b39fb0d473a22492bff2b09abe2b2bb63c15629e75fb05215fR73-R95
 */
_fixUpcastConversionAndAutoParagraphing() {
    // https://github.com/ckeditor/ckeditor5/blob/56ff10c5e9cc24f3c3397a97f7927c1708e00581/packages/ckeditor5-paragraph/src/paragraph.js#L210
    const isParagraphable = ( node, position, schema ) => {
        const context = schema.createContext( position );
        // When paragraph is allowed in this context...
        if ( !schema.checkChild( context, 'paragraph' ) ) {
            return false;
        }
        // And a node would be allowed in this paragraph...
        if ( !schema.checkChild( context.push( 'paragraph' ), node ) ) {
            return false;
        }
        return true;
    }

    const conversionApi = this.editor.data.upcastDispatcher.conversionApi;
    const originalSplitToAllowedParent = conversionApi.splitToAllowedParent;

    conversionApi.splitToAllowedParent = ( node, modelCursor ) => {
        const result = originalSplitToAllowedParent( node, modelCursor );

        if ( result ) {
            return result;
        }

        if ( !isParagraphable( node, modelCursor, conversionApi.schema ) ) {
            return null;
        }

        const paragraph = conversionApi.writer.createElement( 'paragraph' );

        conversionApi.writer.insert( paragraph, modelCursor );

        return {
            position: conversionApi.writer.createPositionAt( paragraph, 0 )
        };
    };
}

Edit: i just run the ckeditor test suite with this fix in version 21 and it works perfectly 14827 tests completed (besides one unrelated error which also exists without this fix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
6 participants