Opened 6 years ago

Closed 6 years ago

#11027 closed Bug (fixed)

Block commands break on widget

Reported by: Piotrek Koszuliński Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3
Component: General Version:
Keywords: Cc:

Description

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...

Change History (17)

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

Owner: set to Piotrek Koszuliński
Status: newassigned

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by 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?

comment:4 Changed 6 years ago by 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.

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

Owner: Piotrek Koszuliński deleted
Status: assignedconfirmed

comment:6 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:7 Changed 6 years ago by Piotr Jasiun

Status: assignedreview

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.

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

Status: reviewreview_failed

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 ) );

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by Piotr Jasiun

Status: review_failedreview
  • renamed domInvalidated to contentDomInvalidated,
  • docs improvements,
  • performance improvements.

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

Status: reviewreview_failed
  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.

comment:12 Changed 6 years ago by Piotr Jasiun

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

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

Status: reviewreview_failed

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

comment:14 Changed 6 years ago by Piotr Jasiun

Status: review_failedreview

Yep. I've pushed it now.

comment:15 Changed 6 years ago by Piotr Jasiun

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

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

Status: reviewreview_passed

comment:17 Changed 6 years ago by Piotr Jasiun

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