Caret placed in wrong place after autoembedding content

[Szymon Cofalik]

Steps to reproduce:

1. Write some text.
2. Paste https://www.google.dk/maps/place/Zygmunta+S%C5%82omi%C5%84skiego+15,+Warszawa
3. While autoembedded content is loading, type more text.
4. Autoembedded content is inserted.

Expected result: Caret is where it was before autoembedded content is put into the editor.
Result: Caret is placed before autoembedded content.

I think the current result is wrong because the place where caret is inserted seems random and it breaks typing flow for user (if they was in the middle of sentence and is still typing, they type in the wrong place).

[Piotrek Koszuliński]

Yup. Upon widget insertion a bookmark should be made and restored afterwards.

[Piotrek Koszuliński]

I removed these keywords intentionally. We use them in special cases only and their list is limited. Usually - we only specify them if a bug is reproduced on some browser only.

[Szymon Cofalik]

I had a few ideas on how to fix this bug, however two seemed the best:

• ​branch:t/13429d (main solution)
• ​branch:t/13429 (alternative solution, no tests or refactor for this).

Main idea is, of course, to remember current selection before inserting content and then recreate it. That was easy part.

However, there is a problem when the selection is partially or wholly in the anchor that is being replaced. The best idea I've come up with is to use insertElement() instead insertElementIntoRange(), which takes care of different cases and sets up caret nicely after paste.

In alternative solution I don't use insertElement() but select pasted content instead. The solution is simpler but it breaks good user experience.

*Note*: I didn't use bookmarks because we don't need them: if selection is valid we can use range and if it is invalid we wouldn't use them anyway and they caused problems with snapshots.

*Note*: Main soultion also fixes #13430, but I would need to write a test for that bug.

[Olek Nowodziński]

​branch:t/13429d looks like the right solution for me. However:

1. Instead of

   insertRange.setStartAt( anchor, CKEDITOR.POSITION_BEFORE_START );
   insertRange.setEndAt( anchor, CKEDITOR.POSITION_AFTER_END );    
   

   use setStartBefore and setStartAfter.
2. As for

   I didn't use bookmarks because we don't need them: if selection is valid we can use range and if it is invalid we wouldn't use them anyway and they caused problems with snapshots.

   the insertion

   editor.editable().insertElement( wrapper, insertRange );
   

   followed by

   // If the selection was valid, rebuild it after inserting embedded content. (#13429)
   if ( range != null ) {
      selection.selectRanges( [ range ] );
   }
   

   is a very weak solution. For example:
   1. Visit ​http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed
   2. Set the following selection

      <p>Foo bar.</p>
      <p>Fo^o bar.</p>
      

   3. Paste the twitter link ​https://twitter.com/reinmarpl/status/573118615274315776 and wait for embed to finish.
   4. Set the following selection

      <p>Fo^o bar.</p>
      <p>Fo</p>
      <twitter widget>
      <p>o bar.</p>
      

   5. Paste another link ​http://i.imgur.com/Z3ilPBI.jpg
   6. Select <twitter widget> before embedding finishes.
   7. Expected

      <p>Fo</p>
      <imgur widget>
      <p>o bar.</p>
      <p>Fo</p>
      [<twitter widget>]
      <p>o bar.</p>
      

      Actual

      <p>Fo</p>
      <imgur widget>
      <p>[o bar.]</p>
      <p>Fo</p>
      <twitter widget>
      <p>o bar.</p>
      

Generally speaking, selecting a range which, once obtained, witnessed any mutation in DOM (like editor.editable().insertElement( wrapper, insertRange );) is dangerous. It's quite impossible to predict all the possible mutation scenarios so we implemented and use bookmarks API (i.e. range.createBookmark()), which is much safer. To deal with undo, saveSnapshot, updateSnapshot and (un)lockSnapshot events are commonly used.

3. Instead of

   if ( range != null ) {
       selection.selectRanges( [ range ] );
   }
   

   you can use shorter

   range && range.select();
   

4. What if instead

   if ( anchor.contains( range.startContainer ) || anchor.equals( range.startContainer ) || anchor.equals( range.endContainer ) || anchor.contains( range.endContainer ) ) {
       range = null;
   }    
   

   we used CKEDITOR.dom.range.intersects from #13453 like

   insertRange.intersects( range )
   

   ? We could merge #13453 before this ticket to use it.
5. Are you totally sure that let's say

   <p>x<embed anchor>y^</p>
   

   is always anchored in "y" text node (offset:1) but never in <p> (offset:3) in every browser? I'm asking because the condition

    else if ( range.startContainer.equals( anchor.getParent() ) || range.endContainer.equals( anchor.getParent() ) ) {
      range = null;
    }
   

   looks very unsafe to me and could possibly reset a valid range. If that happens on IE8, it should be wrapped in CKEDITOR.env.ie && CKEDITOR.env.version < 9 condition. I don't know which cases it fixes though.
6. You mix assertion messages like

     assert.isTrue( range.collapsed, 'selection after paste is not collapsed' );
   

   vs

     assert.isTrue( areSame, 'selection before paste is same' );
   

   The standard of CKEditor tests is that we describe what is expected in assertion messages. So the 2nd one is correct.

I didn't review tests thoroughly because the implementation may change. I'd also wish to see @reinmar chime in (like about #13453).

[Szymon Cofalik]

I rebased ​branch:t/13429d with master and pushed some commits.

Some comment on review_failed:

1. Done.
2. You are right. I haven't thought of all scenarios. I changed the algorithm so it uses bookmarks now.
3. Not in code anymore. BTW. I am on the edge when it comes to code like this. It is simpler but may be less clean at first sight, especially to someone who is not used to it.
4. This was the first idea, but at the time CKEDITOR.dom.range.intersects was not implemented and we came to the conclusion that it is an overkill to write it only for this fix. Then it got needed in other places so I implemented it. But it probably won't get included in API after all (@Reinmar is against it -- see comments in #13453).
5. This got changed and is, hopefully, cleaner now.
6. I understand it now but this is a pretty old ticket and I was a bit confused about messages then :).

I think it might be a good idea to rebase ​branch:t/13429d a bit before merging with master. For now I kept it as is so it is clear to see what got changed in recent commits.

[Olek Nowodziński]

I rebased ​​branch:t/13429d on master and pushed 2 commits, mostly improving tests.

The solution is right but the manual test is missing, which would prove to be helpful while testing the release.

[Szymon Cofalik]

I can get behind most fixes, however I do not agree with two things:

1)

startNode && startNode.remove();
endNode && endNode.remove();

These will always be nodes so why complicate with extra conditions? Is there a scenario where it would prevent an error?

2)

Also you changed too much in the last test. AFAIR I wanted to have a non-collapsed selection for a reason. At the very least, we have an additional case, which makes tests richer. I know that line 466 looks clunky (I can fix it by using RegExp, this was one of my ideas).

BTW. What's the difference between findOne() and find().getItem( 0 ) ?

[Piotrek Koszuliński]

Clueless reply: endNode may not exist if bookmark was made for a collapsed selection.

[Szymon Cofalik]

endNode is set to startNode if it was null. Its made like this to simplify further code.

[Olek Nowodziński]

As for 1., I played a lot with your code (like allowed endNode to be null) and forgot to remove it. Feel free to correct it.

As for 2. I see no reason to complicate things but if you feel it would be the right thing to do, just go ahead. Still please, leave the following assertion

assert.areSame( '<div data-oembed-url="' + pastedText + '"><img src="' + pastedText + '" /></div><p>foo</p>', editor.getData(), 'right editor data after paste' );

because it checks data unlike the the next one, which is insufficient (tests the selection only).

---

BTW. What's the difference between findOne() and find().getItem( 0 ) ?

The difference is readability. It's easy to forget that findOne retrieves the ​first matching element in DOM order, so if called findOne( 'p' ), and there are 2 <p> in DOM, one could be unsure which one has been retrieved. It's a nuance I'm comfortable with but if you find it annoying, also feel free to correct it.

[Szymon Cofalik]

I pushed a commit to ​branch:t/13429d.

[Olek Nowodziński]

Merged ​git:824cbc6 into master.

Good job!

[Piotrek Koszuliński]

The difference is readability. It's easy to forget that findOne retrieves the ​first matching element in DOM order, so if called findOne( 'p' ), and there are 2 <p> in DOM, one could be unsure which one has been retrieved. It's a nuance I'm comfortable with but if you find it annoying, also feel free to correct it.

Just for the future – this can be mentioned in a comment and then using findOne() is absolutely acceptable. In fact – the same question could be raised by someone seeing find().getItem( 0 ). Does find() return elements in DOM order?