Opened 4 years ago

Closed 4 years ago

#12484 closed Bug (fixed)

DOM is changed outside of editor area

Reported by: Boris Lykah Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.6
Component: General Version: 4.4.2
Keywords: Blink Webkit Cc:

Description

  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.

Attachments (3)

bug.html (735 bytes) - added by Boris Lykah 4 years ago.
bug.2.html (698 bytes) - added by Wiktor Walc 4 years ago.
Simplified sample, without <div> in editing area
bug.3.html (493 bytes) - added by Artur Delura 4 years ago.
Even more simpified sample.

Download all attachments as: .zip

Change History (25)

Changed 4 years ago by Boris Lykah

Attachment: bug.html added

comment:1 Changed 4 years ago by Jakub Ś

Keywords: Bkink Webkit added
Status: newconfirmed
Version: 4.4.2

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.

Changed 4 years ago by Wiktor Walc

Attachment: bug.2.html added

Simplified sample, without <div> in editing area

comment:2 Changed 4 years ago by Wiktor Walc

Milestone: CKEditor 4.4.6

comment:3 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

Changed 4 years ago by Artur Delura

Attachment: bug.3.html added

Even more simpified sample.

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

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:5 Changed 4 years ago by Jakub Ś

Possible duplicate was reported here #12570.

Please check that ticket when fixing this one.

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

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:7 Changed 4 years ago by Artur Delura

Status: assignedreview

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

Keywords: Blink added; Bkink removed
Status: reviewreview_failed

comment:9 Changed 4 years ago by Artur Delura

Status: review_failedreview

Sorry, I misinformed you about branch name.

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

Status: reviewreview_failed

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

comment:11 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:12 in reply to:  10 Changed 4 years ago by 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?

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

comment:14 in reply to:  13 Changed 4 years ago by 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.

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

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

comment:17 in reply to:  16 Changed 4 years ago by 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.

comment:18 in reply to:  10 Changed 4 years ago by Artur Delura

Replying to Reinmar:

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

Last edited 4 years ago by Artur Delura (previous) (diff)

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

comment:20 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes in the same branch:t/12484b.

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

Status: reviewreview_passed

After some minor improvements everything is ok.

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

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:7eb88f9.

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