DOM is changed outside of editor area

[Boris Lykah]

1. Open the attached file in Chrome and place the caret into the editable area.
2. Mark all contents either with Ctrl+A or mouse selection.
3. Delete the content with backspace or del key.
4. Press Insert/Remove Numbered List button on the toolbar.

As the result, tag <div>other content</div> following the editable area is turned into a list.

[Jakub Ś]

This issue looks very similar to #11974. While I can no longer reproduce the former ticket, this one seems to be reproducible from CKEditor 4.4.2 in Blink and Webkit browsers.

To be more specific; this bug affects Safari 7, Chrome and Opera. I wasn't able to reproduce this problem in Safari 5 on Windows. I haven't checked Safari 6.

Current issue seems to be more serious as it is enough to remove contents in inline editor and then insert list.

[Wiktor Walc]

Simplified sample, without <div> in editing area

[Artur Delura]

Even more simpified sample.

[Artur Delura]

I noticed problem in iterator. From what I see it does not respect whether while iterating nodes it is still in editable area. To fix this issue I added extra check which prevent iterating out of editor scope in ​here. I already fixed it in ​commit. But it's hard to restore issue in tests - it just simply pass but should fail.

There are differences between test code (executing 'selectAll' command) and selecting all content by mouse:

editor = CKEDITOR.instances.editor1;

// This make diffrences while simulating in tests - make tests pass when they should fail.
setTimeout( function () {
  editor.execCommand( 'selectAll' );
}, 1000 );

// Simulating this one work fine.
setTimeout( function () {
  console.log( 'a' );
  editor.editable().fire( 'keydown', new CKEDITOR.dom.event( { keyCode: 8 } ) ); // Backspace.
  editor.editable().fire( 'keyup', new CKEDITOR.dom.event( { keyCode: 8 } ) ); // Backspace.
  editor.execCommand( 'numberedlist' );
}, 3000 );

Here is another code snippet which I used to trying to simulate use case:

// Select all content and press backspace before timeout fires.

setTimeout( function() {
  var editor = CKEDITOR.instances.editor1,
      range = editor.getSelection().getRanges()[ 0 ],
      iterator = range.createIterator(),
      block,
      blocks = [];

  while ( ( block = iterator.getNextParagraph() ) ) {
    blocks.push( block.$ );
  }
  console.log( blocks );
}, 1000 );

Both tests are in ​branch:t/12484.

[Jakub Ś]

Possible duplicate was reported here #12570.

Please check that ticket when fixing this one.

[Artur Delura]

I removed test which I have written before and write another ones (including manual). Created a new branch for it: ​branch:t/12484b.

[Piotrek Koszuliński]

Why did you add an ignored test? ​https://github.com/cksource/ckeditor-dev/commit/23c099303c3811b5f699a17fd09214ff319fe9d0#diff-0bac97b49c5acd6ffd898b939836546dR601

[Artur Delura]

Sorry, I misinformed you about branch name.

[Piotrek Koszuliński]

Still some stuff needs to be improved:

1. Note the order of @param tag arguments. The type goes always first.
2. I would like to see more unit tests for the changes in the iterator. You fixed some issue there, but there's no test for that and it should be feasible to create it (the test for getNextSourceNode doesn't check enough IMO). Also, there's no test for the new method setStartContainer - at least a basic one should exist.
3. There's code style issue in test/core/dom/iterator.js and grammar error in the test name ("iterator does not").
4. As for the manual test - "delete" is a key, not a button. And the steps should be as simple as possible - e.g. instead: "Click proper button to create ordered list." better write: "Click the ordered list button.".
5. There are code style issues in the setStartContainer method (missing spaces before comments) and it's not marked with the @since tag.
6. It would be better to keep private methods of the iterator together. First go the public ones then the private ones.
7. Instead of using %REMOVE_LINE% so many times use %REMOVE_START/END%.

[Artur Delura]

Replying to Reinmar:

Still some stuff needs to be improved:

1. Note the order of @param tag arguments. The type goes always first.

Did you missed ​this commit, didn't you?

[Piotrek Koszuliński]

No, I didn't:

​https://github.com/cksource/ckeditor-dev/compare/t/12484b#diff-ce752e8e969f622a6178fd15858b88bdR2526

But actually I haven't noticed that you added this method to the range class. I'm not sure if this is a good idea. We do not perform any validation right now and it's hard to predict how other developers use the range class and I don't think that it's even documented that containers should be in the root (root is not part of the W3C's spec). Additionally, we would need to cover the end container as well.

So, I'd be ok if this validation was done from the very beginning, but adding it today, considering that I don't remember any other problems with leaking from the root, seems to be superfluous and dangerous.

[Artur Delura]

Replying to Reinmar:

No, I didn't:

​https://github.com/cksource/ckeditor-dev/compare/t/12484b#diff-ce752e8e969f622a6178fd15858b88bdR2526

But actually I haven't noticed that you added this method to the range class. I'm not sure if this is a good idea. We do not perform any validation right now and it's hard to predict how other developers use the range class and I don't think that it's even documented that containers should be in the root (root is not part of the W3C's spec). Additionally, we would need to cover the end container as well.

So, I'd be ok if this validation was done from the very beginning, but adding it today, considering that I don't remember any other problems with leaking from the root, seems to be superfluous and dangerous.

But this method is marked as a @private one. Which means that shouldn't be called beyond range class - that's the reason why this tag exists. Errors should be catched as early as possible. I suggest to cover setEndContainer as well.

[Piotrek Koszuliński]

It's not about this. It's about that this method is being used when you use other range's methods. And there's nothing wrong with validation in such critical places BUT it must not appear somewhere in CKEditor 4.4.6. 7 years after this API was introduced.

Anyway, let's ask Fred about his opinion too.

[Piotrek Koszuliński]

Fred'd be ok with adding validation (although the methods should be prefixed with the underscore), but after more time I still don't feel that it's not right to change existing APIs in a minor release.

So:

• Please remove the validation.
• Please prefix the new method in the iterator class with a prefix.

[Artur Delura]

Replying to Reinmar:

• Please remove the validation.

Okay so we end up with decision that we are not going to remove this validation because it won't break nothing - it simply log information in console instead of throwing error.

• Please prefix the new method in the iterator class with a prefix.

To be honest it's quite weird that getter is private.

[Artur Delura]

Replying to Reinmar:

2. I would like to see more unit tests for the changes in the iterator. You fixed some issue there, but there's no test for that and it should be feasible to create it (the test for getNextSourceNode doesn't check enough IMO)

Test which I have written checks exactly what I've implemented. If you will comment out my code it will fail. This method heavily rely on node.getNextSourceNode which unfortunatelly does not have lot's of tests and have no documentation :(. I will try to fix it.

Edit:

Okay, I found this ​task. I think this is the first step we should do.

[Piotrek Koszuliński]

I meant adding at least one test for the whole iterator.getNextParagraph method. You only checked that the new method works ok in this case, but you haven't checked if the iterator does as well. That wouldn't be a problem if we had unit tests for the iterator, but that's not the case here.

As for node.getNextSourceNode I know that it was very odd and needs tests. I think that we can raise the priority of #12191, but it does not block the current ticket at the moment.

[Artur Delura]

Changes in the same ​branch:t/12484b.

[Piotrek Koszuliński]

After some minor improvements everything is ok.

[Piotrek Koszuliński]

Fixed on master with ​git:7eb88f9.