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

Cannot type after inline widget #1049

Closed
lucasriondel opened this issue May 31, 2018 · 42 comments
Closed

Cannot type after inline widget #1049

lucasriondel opened this issue May 31, 2018 · 42 comments
Assignees
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@lucasriondel
Copy link

lucasriondel commented May 31, 2018

I tried to create a plugin that uses an inline widget. The plugin should insert an inline, non-editable HTML element with some text into the editor content. I used toWidget to widgetize the element. However I have a weird issue:

  • if I put the widget straight after some text (blablabla<variable>myVar</variable>) without any whitespace between text and widget, I am able to type text after the widget,
  • but if I put a whitespace (blablabla <variable>myVar</variable>), I cannot type anything after the widget, I only can put a caret there and press Enter.

Here's a video of what's happening : https://streamable.com/2l4hz

And here's the code:

My plugin:

function downcastInsertVariable(options: { asWidget?: boolean } = {}) {
  return dispatcher =>
    dispatcher.on(
      'insert:variable',
      (evt, data, conversionApi) => {
        const variable = data.item

        if (!conversionApi.consumable.consume(variable, 'insert')) {
          return
        }

        conversionApi.consumable.consume(variable, 'attribute:id:variable')

        const asWidget = options && options.asWidget

        const variableElement = conversionApi.writer.createContainerElement('variable')

        let variableWidget

        if (asWidget) {
          variableWidget = toWidget(variableElement, conversionApi.writer)
        }

        const viewPosition = conversionApi.mapper.toViewPosition(data.range.start)

        conversionApi.mapper.bindElements(variable, asWidget ? variableWidget : variableElement)
        conversionApi.writer.insert(viewPosition, asWidget ? variableWidget : variableElement)
      },
      { priority: 'normal' }
    )
}

class InsertVariable extends Plugin {
  public init() {
    const editor = this.editor

    editor.model.schema.register('variable', {
      inheritAllFrom: '$text',
      allowAttributes: ['id'],
      isObject: true
    })

    editor.model.schema.extend('$text', {
      allowIn: 'variable'
    })

    editor.conversion.for('upcast').add(upcastElementToElement({ model: 'variable', view: 'div' }))
    editor.conversion.for('editingDowncast').add(downcastInsertVariable({ asWidget: true }))
    editor.conversion.for('dataDowncast').add(downcastInsertVariable())
    editor.conversion.attributeToAttribute({ model: 'id', view: 'id' })

    editor.ui.componentFactory.add('insertVariable', locale => {
      const view = new ButtonView(locale)

      view.set({
        label: 'Insert variable',
        icon: imageIcon,
        tooltip: true
      })

      view.on('execute', () => {
        editor.model.change(writer => {
          const variableElement = writer.createElement('variable', {
            id: 'some-id'
          })

          writer.insertText('variable', { bold: true }, variableElement, 'end')

          editor.model.insertContent(variableElement, editor.model.document.selection)
        })
      })

      return view
    })
  }
}

And here's my editor initialization (with a bit of React code, sorry about that) :

  public async initEditor() {
    this.editor = await DecoupledEditor.create(
      ReactDOM.findDOMNode(this.editorRef.current as HTMLDivElement),
      {
        plugins: [
          Essentials,
          Bold,
          Italic,
          Underline,
          Paragraph,
          Heading,
          HeadingUI,
          Alignment,
          InsertVariable
        ],
        toolbar: [
          'heading',
          'bold',
          'italic',
          'underline',
          'alignment:left',
          'alignment:right',
          'alignment:center',
          'insertVariable'
        ],
        fontFamily: {
          options: 'Montserrat sans-serif'
        },
        heading: {
          options: [
            { model: 'heading1', view: 'h4', title: 'Titre de section', class: 'section-title' },
            { model: 'paragraph', title: 'Texte', class: 'ck-heading_paragraph' }
          ]
        }
      }
    )
    const el = ReactDOM.findDOMNode(this.toolbarRef.current as HTMLDivElement)
    if (el instanceof Element) {
      el.appendChild(this.editor.ui.view.toolbar.element)
    }
  }

Any idea?

@scofalik
Copy link
Contributor

scofalik commented Jun 1, 2018

I edited the issue to make it more clear. It looks like you are trying to implement Placeholder plugin, known from CKEditor 4.

We haven't worked on inline widgets yet, although it is something that we want to work on in near future. The issue itself is weird -- whitespace is like any other character so it is very interesting that inserting it changes the editor behavior. Note that the whitespace might have been converted to &nbsp; though, as it is next to a ContainerElement. Still, I don't understand what impact could that have on the editor behavior.

My guess would be that there's something wrong with selection placement or view<->model mapping.

@scofalik scofalik changed the title Inline element not behaving correctly Cannot type after Inline widget Jun 1, 2018
@scofalik scofalik changed the title Cannot type after Inline widget Cannot type after nline widget Jun 1, 2018
@scofalik scofalik changed the title Cannot type after nline widget Cannot type after inline widget Jun 1, 2018
@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed labels Jun 1, 2018
@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

I created a separate ticket to track inline widgets support: #1050.

@lucasriondel
Copy link
Author

a new weird behavior (with the same code) : https://streamable.com/a780v
when i click on my element, i am able to delete text from it using backspace or delete.

@lucasriondel
Copy link
Author

while watching the dom, i have also found what might be a weird behavior related to my issue (it's not happening with images, by example) https://streamable.com/8sv43

@lucasriondel
Copy link
Author

more tests... https://streamable.com/lepds

@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

I removed my comment because it was completely incorrect.

@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

    editor.conversion.for('editingDowncast').add(downcastInsertVariable({ asWidget: true }))
    editor.conversion.for('dataDowncast').add(downcastInsertVariable())

@scofalik, why did we need custom converters here?

but if I put a whitespace (blablabla myVar), I cannot type anything after the widget, I only can put a caret there and press Enter.

It may be related to #1024. cc @pomek

@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

@scofalik, why did we need custom converters here?

I'm asking because it'd be good to have a minimal TC here that we could work on.

@scofalik
Copy link
Contributor

scofalik commented Jun 1, 2018

We need custom converters to widgetize the inserted element. There's no elementToWidget converter AFAIR :)

@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

Can't we provide a callback to elementToElement()?

@Reinmar
Copy link
Member

Reinmar commented Jun 1, 2018

Just like in one of the tests:

		conversion.for( 'editingDowncast' ).add( downcastElementToElement( {
			model: 'fancywidget',
			view: ( modelItem, viewWriter ) => {
				const widgetElement = viewWriter.createContainerElement( 'figure', { class: 'fancy-widget' } );
				viewWriter.insert( ViewPosition.createAt( widgetElement ), viewWriter.createText( 'widget' ) );

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

@scofalik
Copy link
Contributor

scofalik commented Jun 4, 2018

Oh, yeah, we could.

@lucasriondel
Copy link
Author

Any updates ? @scofalik @Reinmar

@lucasriondel
Copy link
Author

lucasriondel commented Jun 4, 2018

I've been able to find a workaround which makes my code work but involves more bugs. It is working if I force a Text element next to my Variable element, and change my schema to extend $block. (but if I remove it, I have the same behavior again...)

editor.model.schema.extend('$block', {
      allowIn: 'variable'
    })
editor.model.insertContent(variableElement, editor.model.document.selection)
editor.model.insertContent(new Text(' '), editor.model.document.selection)

unfortunately, when I click on my variable element, and press enter, it breaks my element in two.

before :
<variable class="ck-widget" contenteditable="false"><strong>Variable1</strong></variable>

after :
<variable class="ck-widget" contenteditable="false"><strong>Vari</strong></variable><variable class="ck-widget" contenteditable="false"><strong>able2</strong></variable></p>

also, if I press backspace while on my element, it breaks in two but differently:

<variable class="ck-widget" contenteditable="false"><strong>Vari</strong><p><br data-cke-filler="true"></p><strong>ble1</strong></variable>

And in that case, if I try to fix it, this error is thrown:

Uncaught CKEditorError: move-operation-node-into-itself: Trying to move a range of nodes into one of nodes from that range. Then, I can't do anything with the editor, it's messed up.

The wanted behavior is close, actually. If only I could stop CKEditor from handling key presses such as enter or backspace when my element is clicked, and disallow the space after the element from being deleted, it would be perfect !

@Reinmar
Copy link
Member

Reinmar commented Jun 4, 2018

OK, to start from the beginning...

I created a small demo of an inline widget: https://github.com/ckeditor/ckeditor5-core/compare/t/ckeditor5/1049

According to my tests it works fine. To my surprise, I haven't found yet any issue :). EDIT: I have now – a placeholder doesn't work well inside another widget's nested editable.

jun-04-2018 17-19-18

All the behaviours which you can see on the screencast (I tested clicking to focus, copy paste, typing after it, backspace and arrow keys behaviour) are as expected.

Am I missing something? Could you test it yourself?

@Reinmar
Copy link
Member

Reinmar commented Jun 4, 2018

I found one more issue – attributes get applied inside this element:

image

And this is tricky... We need to think how to resolve this because so far we didn't have attribute elements used next to container elements. What's more, in the model it's the text inside the <placeholder> element what gets the attribute, while this text shouldn't actually be styleable. It's non-editable. If anything, the <placeholder> should get this attr :D

cc @scofalik @oskarwrobel @pjasiun

@Reinmar
Copy link
Member

Reinmar commented Jun 4, 2018

OK, two ideas:

  1. Disallow attributes on placeholder>$text and allow them on the placeholder itself (via Schema).
  2. Move the text of the placeholder to its attribute to keep this element empty in the model.

The latter seems more correct to me.

@Reinmar
Copy link
Member

Reinmar commented Jun 4, 2018

Buuut... if we want to allow generating data like this:

<p><strong>xx <placehoder>yy</placeholder> yy</strong></p>

we have a problem now because AFAIR a ContainerElement (<placeholder>) cannot be inside an AttributeElement (<strong>). So, should we use UIElement here? It doesn't seem to be right too.

I'm afraid that we'll need some time to figure out things like this, @lucasriondel. As you can see, the feature seems to be working (and I guess it could be used in production), but this is a kinda "beginner's luck" :D Since we haven't focused on inline widgets yet there are certain architectural aspects we need to understand first.

@scofalik
Copy link
Contributor

scofalik commented Jun 5, 2018

  1. I don't think that <placeholder> should be a ContainerElement. I thought EmptyElement but then realized that those cannot have any children (can we change how they are rendered in DOM?). So, either UIElement (mmm... nope), or AttributeElement. I like AttributeElement more than ContainerElement. BTW. shouldn't it be a <span>?
  2. How did you define placeholder in Schema? I thought as inheritAllFrom: '$text'?
  3. Both solutions (disallowing attributes inside placeholder and moving placeholder text to attribute) are fine. As long as placeholder texts are only taken from dropdowns (and are not editable through typing) we will be fine. If they were editable through typing, we won't have a perfect OT then.

@scofalik
Copy link
Contributor

scofalik commented Jun 5, 2018

Okay, I see that your placeholder definition does not inherit attributes allowance from $text. I think it should. Will be easier to allow bold and others on it. We can also move the text to the attribute if it will become problematic. It may be even better in some regard, for example, if one would like to change placeholder text in all documents. We need to somehow make placeholders recognizable in output data so they are easier to replace afterwards.

So, I think that the best final output would be something like:
<strong>[[[placeholder:placeholderId]]]</strong>.

@Reinmar
Copy link
Member

Reinmar commented Jun 5, 2018

I don't think that should be a ContainerElement. I thought EmptyElement but then realized that those cannot have any children (can we change how they are rendered in DOM?). So, either UIElement (mmm... nope), or AttributeElement. I like AttributeElement more than ContainerElement. BTW. shouldn't it be a ?

I don't think that we should use AttrElement here. AttrElements can get merged. You don't want widgets to be merged. Widgets are like containers. Self-contained, non-mergable, untouchable. I recall that @pjasiun was also for using ContainerElements here.

BTW. shouldn't it be a <span>?

It does not matter. There's nothing wrong with using custom elements.

How did you define placeholder in Schema? I thought as inheritAllFrom: '$text'?

It may inherit attrs of $text, so it can be defined this way. But it won't change much here unless we'll disallow attrs on placeholder>$text. We could try that and see wha's going to happen. But AFAIR AttrEl>ContainerEl structure is incorrect anyway (according to our docs)... so we need to rethink this.

Both solutions (disallowing attributes inside placeholder and moving placeholder text to attribute) are fine. As long as placeholder texts are only taken from dropdowns (and are not editable through typing) we will be fine. If they were editable through typing, we won't have a perfect OT then.

Yup. If the placeholder's text was supposed to be editable "inline", it would need to also be defined as a nested editable in the view. That's overcomplicated for this case, so we could assume that this text is editable via some balloon.

@scofalik
Copy link
Contributor

scofalik commented Jun 5, 2018

ContainerElement should not be inside AttributeElement. This will not work. We can prevent merging AttributeElements, we already have mechanisms for that (ids, that we introduced quite recently, we were thinking about them in abbr context, they should work).

If you don't like AttributeElements (and I don't blame you for that), we need a new type of element.

@Reinmar
Copy link
Member

Reinmar commented Jun 5, 2018

OK, let's wait for @pjasiun with this. This is an important decision.

@Reinmar Reinmar added this to the unknown milestone Jun 7, 2018
@lucasriondel
Copy link
Author

Well I still have this issue..

@Reinmar
Copy link
Member

Reinmar commented Jun 11, 2018

Would it be possible that you created a repo with all the code needed for us to reproduce it. Plus, please include the exact steps to reproduce this issue. The devil may be in the details.

@lucasriondel
Copy link
Author

Sure, i'll try.

@lucasriondel
Copy link
Author

@Reinmar
Copy link
Member

Reinmar commented Jun 18, 2018

I'll get back to this ticket once we'll release v10.1.0. It's a busy week, sorry.

@lucasriondel
Copy link
Author

I understand, don't worry. Thanks again for your time.

@Reinmar
Copy link
Member

Reinmar commented Jun 20, 2018

BTW, do you test this in Firefox maybe? I'm asking because this issue may be related to #1083.

@lucasriondel
Copy link
Author

No, I am using chrome.

@dkrahn
Copy link
Contributor

dkrahn commented Sep 4, 2018

@Reinmar I replicated the case by having the inline widget at the end of the paragraph. If the widget has a space(&nbsp;) right before it, no text can be added after it.
It works fine If you already have some text after the widget, or no space right before it.
Hope my description helps.

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

Thanks for the details. That may help us debugging it.

I hoped that we'll be able to look into this right after the previous release but I wasn't able to keep this ticket above the 'fold'. I'll add it to the next iteration and hopefully that'll help.

@Reinmar Reinmar modified the milestones: unknown, iteration 21 Sep 4, 2018
@Reinmar Reinmar modified the milestones: iteration 21, iteration 22 Oct 25, 2018
@jodator
Copy link
Contributor

jodator commented Jan 24, 2019

It's kinda funny to see that if inline widget is glued to some text you can type after it.

But if there is a space before you cannot:

peek 2019-01-24 14-51

@Reinmar
Copy link
Member

Reinmar commented Jan 29, 2019

Should be related to the mutation observer in the typing package. Spaces cause a lot of problems. I guess we get a big children mutation of the paragraph and we end up in this lovely method: https://github.com/ckeditor/ckeditor5-typing/blob/e553afb1592c66a45f0964ccb41d6f1672ad3702/src/utils/injecttypingmutationshandling.js#L92

@jodator
Copy link
Contributor

jodator commented Jan 29, 2019

@Reinmar yep it might be also related to how spaces are handled - it looks like they are not normalized and there's change detected in the 'Hello&nbsp' part alone.

@crypto-dump
Copy link

crypto-dump commented Oct 30, 2019

I've read above. It seems I need to create element using AttributeElement, coz I need to stylize (bold, italic, font, font-size, font color) for the placeholder text.
How can I approach this?

Here's my placeholderediting.js.
`
export default class PlaceholderEditing extends Plugin {
static get requires() {
return [ Widget ];
}

init() {
    this._defineSchema();
    this._defineConverters();

    this.editor.commands.add( 'placeholder', new PlaceholderCommand( this.editor ) );

    this.editor.editing.mapper.on(
        'viewToModelPosition',
        viewToModelPositionOutsideModelElement( this.editor.model, viewElement => viewElement.hasClass( 'placeholder' ) )
    );
    this.editor.config.define( 'placeholderConfig', {
        types: [ 'date', 'first name', 'surname' ]
    } );
}

_defineSchema() {
    const schema = this.editor.model.schema;

    schema.register( 'placeholder', {
        // Allow wherever text is allowed:
        inheritAllFrom: '$text',

        // The placeholder will act as an inline node:
        isInline: true,

        // The inline widget is self-contained so it cannot be split by the caret and it can be selected:
        isObject: true,

        // The placeholder can have many types, like date, name, surname, etc:
        allowAttributes: [ 'name' ]
    } );
}

_defineConverters() {
    const conversion = this.editor.conversion;

    conversion.for( 'upcast' ).elementToElement( {
        view: {
            name: 'span',
            classes: [ 'placeholder' ]
        },
        model: ( viewElement, modelWriter ) => {
            // Extract the "name" from "{name}".
            const name = viewElement.getChild( 0 ).data.slice( 1, -1 );

            return modelWriter.createElement( 'placeholder', { name } );
        }
    } );

    conversion.for( 'editingDowncast' ).elementToElement( {
        model: 'placeholder',
        view: ( modelItem, viewWriter ) => {
            const widgetElement = createPlaceholderView( modelItem, viewWriter );

            // Enable widget handling on a placeholder element inside the editing view.
            return toWidget( widgetElement, viewWriter );
        }
    } );

    conversion.for( 'dataDowncast' ).elementToElement( {
        model: 'placeholder',
        view: createPlaceholderView
    } );

    // Helper method for both downcast converters.
    function createPlaceholderView( modelItem, viewWriter ) {
        const name = modelItem.getAttribute( 'name' );

        const placeholderView = viewWriter.createAttributeElement( 'span', {
            class: 'placeholder'
        } );

        // Insert the placeholder name (as a text).
        const innerText = viewWriter.createText( '{' + name + '}' );
        viewWriter.insert( viewWriter.createPositionAt( placeholderView, 0 ), innerText );

        return placeholderView;
    }
}

}`

@crypto-dump
Copy link

ContainerElement should not be inside AttributeElement. This will not work. We can prevent merging AttributeElements, we already have mechanisms for that (ids, that we introduced quite recently, we were thinking about them in abbr context, they should work).

If you don't like AttributeElements (and I don't blame you for that), we need a new type of element.

Is there any new Element to overcome this?

@stasyabezruk
Copy link

stasyabezruk commented Jun 16, 2020

The cursor doesn't work in chrome in the end of placeholder - for span, in Firefox - it works
I've used the source code from documentation
https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html

@stasyabezruk
Copy link

stasyabezruk commented Jun 16, 2020

in your demo this line is added every time after placeholder
image

and when I use you source code, I have another behavior
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

7 participants