Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13429 closed Bug (fixed)

Caret placed in wrong place after autoembedding content

Reported by: Szymon Cofalik Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.3
Component: General Version: 4.5.0
Keywords: Cc:

Description

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

Change History (18)

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

Keywords: opera ie ff chrome removed
Milestone: CKEditor 4.5.1
Status: newconfirmed

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

comment:2 Changed 3 years ago by Szymon Cofalik

Keywords: opera ie ff chrome autoembed added

comment:3 Changed 3 years ago by Piotrek Koszuliński

Keywords: opera ie ff chrome autoembed removed

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.

comment:4 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:5 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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

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.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:6 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

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.

  1. Instead of
    if ( range != null ) {
        selection.selectRanges( [ range ] );
    }
    
    you can use shorter
    range && range.select();
    
  2. 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.
  3. 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.
  4. 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).

comment:7 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.2CKEditor 4.5.3

comment:8 Changed 3 years ago by Szymon Cofalik

Status: review_failedassigned

comment:9 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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.

comment:10 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

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.

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

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:12 Changed 3 years ago by Piotrek Koszuliński

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

comment:13 Changed 3 years ago by Szymon Cofalik

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

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

comment:15 Changed 3 years ago by Szymon Cofalik

Status: review_failedreview

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

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:16 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:17 Changed 3 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged git:824cbc6 into master.

Good job!

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

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