Block commands break on widget

[Piotrek Koszuliński]

Follow up for #10822.

Umbrella ticket for issues like:

• List commands sometimes clone contents, so references are lost and widgets are destroyed.
• Block with only non-editable content are not turned into list items.
• Blocks with only non-editable content are not idented.
• Indent on fake selection (e.g. mathjax) throws an error.
• List command on fake selection throw an error.
• Blockquote does not work with non-editable elements.
• Indent stops on first non-editable element.
• Perhaps more...

[Piotrek Koszuliński]

I pushed simple commit to ​t/11027 which causes that blockquote, indentblock, indentlist, list and justify correctly apply when non-editable content is in selection.

What remains though is:

• List commands sometimes clone contents, so references are lost and widgets are destroyed. I noticed that list plugin clones a lot and often.
• Block commands applied on fake selection made on inline element throw an exception.

[Piotrek Koszuliński]

I pushed additional commit (plus test) for the error being thrown when applying block commands.

It's a very complicated situation:

1. We've got fake selection and it's locked.
2. After blockquote is applied it restores the remembered selection.
3. This causes another selection.fake() call.
4. This causes hideSelection method (from core/selection.js) to be executed.
5. It wants to lock a undo manager, because it's going to add a hidden selection container.
6. To lock snapshot, undo manager was creating images which store the selection in form of bookmarks2.
7. However, it takes the locked (fake) selection and fresh content.
8. And the content has been changed in the meantime... Odd thing - why only on Chrome? (tc was not reproducible on FF).
9. This means that there's a conflict between selection and contents and that's why createBookmark2 throws an error.

Now, there are two things which should be fixed:

1. Undo manager does not have to create full images with selection - it can only create faster contents images. This way, we'll not call createBookmark2 at all. And that's what I pushed in t/11027.
2. But... is this situation correct at all? Shouldn't be selection invalidated (reset) before new one is done? On the other hand - it may be possible that lockSnapshot (or other selection&content sensitive method) is called after DOM is changed while selection is fake. Won't it throw errors then?
3. Additional question - why second hideSelection call does not create another hidden selection container :|? When is the hiddenContainer for initial selection removed?

[Piotrek Koszuliński]

Additional question - why second hideSelection call does not create another hidden selection container :|? When is the hiddenContainer for initial selection removed?

Answer: Because when faking new selection and creating new container for it we use selection.setRanges() to place the real selection in its container. This causes reset() which removes old container... I guess it should not be so tangled, so I pushed a commit which adds this.reset() at the beginning of fake() method. It also (this commit) fixes the error thrown from createBookmark2, so we've got two patches for this one issue.

Although, I think that there's still a scenario in which it may blow up, but let's not waste time on trying to find it.

[Piotr Jasiun]

I fixed list issue. Now list operations which use list-array transformation fire domInvalidated event which call widget.repo.checkWidgets. I also extend checkWidgets function to recreate broken widgets.

Tested with list indent and changing list type and everything seems to be fine now.

Changes in ​t/11027 and tests in corresponding branch.

[Piotrek Koszuliński]

Missing space:

if( !this.getByElement( wrapper ) ) {

Also, getByElement accepts 2nd argument, which limits check to wrappers, so it's a lot faster.

---

I don't like the name of the event - domInvalidated. I cannot say why, but I don't like it :D Something more positive maybe? domReloaded?

---

Mentioning in event's docs about widgets does not clarify event's meaning unless someone knows widget architecture. I would simply remove that part.

---

Why did you put event's docs in widget plugin? It is a core widget.

---

There are no comments in the code. For example - how would someone know why cke_widget_new is added?

---

this.initOn( wrapper.findOne( '.cke_widget_element' ) );

findOne is not very lightweight method (check its code to learn why), so I would use getFirst( isWidgetElement2 ):

See similar code in initOnAll:

this.initOn( newWidgets.getItem( i ).getFirst( isWidgetElement2 ) );

[Piotrek Koszuliński]

I don't like the name of the event - domInvalidated. I cannot say why, but I don't like it :D Something more positive maybe? domReloaded?

I just remembered about contentDom and contentDomUnload. The new proposed event is similar to them, so in my opinion should start with contentDom. E.g. contentDomReloaded.

[Piotr Jasiun]

• renamed domInvalidated to contentDomInvalidated,
• docs improvements,
• performance improvements.

[Piotrek Koszuliński]

1. There are no tests for changes in widgets.repository#checkWidgets.
2. Test for widget reloading on lists change should be split into two separate parts:
   1. tests that list features fires contentDomInvalidated,
   2. tests that e.g. after editable().setHtml( editable.getHtml() ) and firing contentDomInvalidated widgets are reloaded.

Also, I changed my mind regarding checkWidgets event. It should be used instead of direct method usage. I think we should keep both checkWidgets and checkSelection events, because the latter one already proved that it's usefull.

[Piotr Jasiun]

• I've split existing tests and added new ones.
• contentDomInvalidated event now fire checkWidgets event.

[Piotrek Koszuliński]

I cannot find changes in tests repo. Have you pushed them?

[Piotr Jasiun]

Yep. I've pushed it now.

[Piotr Jasiun]

I've added tests for first part of this ticket (blockquote, indent, list and justify).

[Piotr Jasiun]

• ​git:7b210e8
• tests:82fb749