Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#11171 closed Bug (fixed)

No checkWidgets after editor#insertHtml and #insertText

Reported by: Alfonso Martínez de Lizarrondo Owned by: Piotrek Koszuliński
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.3.1
Component: General Version: 4.3
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

TC1

Use the API sample and add the image2 plugin, now insert an image with the textarea to test the API and that image can't be edited because it isn't a widget.

TC2

Go to placeholder sample and execute:

CKEDITOR.instances.editor1.insertText( '[[foo]]' )

Text will be upcasted to widget, but widget won't be initialized on it.

Workaround

After inserting HTML/text call:

editor1.widgets.fire( 'checkWidgets' )

Change History (19)

comment:1 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)
Milestone: CKEditor 4.3.1
Status: newconfirmed
Summary: Images inserted with the API can't be edited if image2 is usedNo checkWidgets after editor#insertHtml

Another case - insert template with image - image won't be a widget (actually, it will be gone, but due to: #11170).

comment:2 Changed 6 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:3 Changed 6 years ago by Olek Nowodziński

cc

comment:4 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:5 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:6 Changed 6 years ago by Piotrek Koszuliński

Priority: NormalHigh

comment:7 Changed 6 years ago by Piotrek Koszuliński

Summary: No checkWidgets after editor#insertHtmlNo checkWidgets after editor#insertHtml and #insertText

comment:8 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Besides inserting content with insertHtml, another way to add content to the editor is by directly creating the DOM elements and inserting them.

In that case the images don't get the wrapping span and all those properties and the call to widgets.checkWidgets() doesn't do anything.

Is there a way to fully initialize a widget on a specific DOM element? (ie: I call something like editor.widgets.initialize(element) and the editor takes care to check if that element must be converted to a widget)

comment:9 Changed 6 years ago by Piotrek Koszuliński

There are two ways to wrap element into a widget (we call this upcasting).

  • First is by using data processor if upcast method was defined - this happens on insertHtml and insertText, but that means operating on HTML string.
  • Second way is by using editor.widgets.wrapElement - this method can be called on detached or wired element, both on dom.element and htmlParser.element. After wrapping, widget still has to be initialised.
  • And there's a third way which reuses the second way - editor.widgets.initOn. This method will wrap element and then initialise widget on it.

Regarding checkWidgets - it has only two jobs - destroying vanished widgets and initialising widgets on wrapped (upcasted) elements which don't have instances yet.

comment:10 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Thanks, but the problem with editor.widgets.wrapElement (or editor.widgets.initOn) is that besides the element, it requires to know the widget that I want to use and my request is to say to CKEditor:

Hello, I've inserted this element, I've finished. Can you check now if some widget wants to do something about it?

I don't want to reimplement any logic to find out which widgets are active, which elements they want to work on and which elements they avoid, I want an API call that will detect what to do with that element.

comment:11 Changed 6 years ago by Piotrek Koszuliński

To reuse widgets' upcast methods you have to use insertHtml() rather than insertElement(). I understand your point, but this is a limitation which couldn't be overcome without a code duplication in widgets definitions. Widgets' upcast methods work only with htmlParser.element, so they cannot be used in insertElement().

comment:12 Changed 6 years ago by Jakub Ś

#11210 was marked as duplicate.

comment:13 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:14 Changed 6 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/11171 on dev and tests.

I extended checkWidgets capabilities with two new options and reused it in insertText and insertHtml events listeners. I was also able to remove the afterPaste listener in favor of insertHtml - now all insertions are handled the same way.

Additionally - I removed checkWidgets on keyup, because it will be enough when widgets will be cleaned up during insertions.

Regarding review - I strongly recommend checking commit after commit, because I did a code reorganisation which makes combined diff unreadable.

comment:15 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_failed

3 tests failing on IE8.

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:16 Changed 6 years ago by Piotrek Koszuliński

Status: review_failedreview

Two tests were broken and one test is ok, but is failing because of #11055 which I accidentally partially fixed on other browsers.

comment:17 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:18 Changed 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:918bcc4 on dev and 918bcc4 on tests.

comment:19 Changed 6 years ago by Piotrek Koszuliński

PS. Most likely we'll include in 4.3.1 #11244.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy