#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:
- Write some text.
- Paste
https://www.google.dk/maps/place/Zygmunta+S%C5%82omi%C5%84skiego+15,+Warszawa
- While autoembedded content is loading, type more text.
- 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 10 years ago by
Keywords: | opera ie ff chrome removed |
---|---|
Milestone: | → CKEditor 4.5.1 |
Status: | new → confirmed |
comment:2 Changed 10 years ago by
Keywords: | opera ie ff chrome autoembed added |
---|
comment:3 Changed 10 years ago by
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 10 years ago by
Owner: | set to Szymon Cofalik |
---|---|
Status: | confirmed → assigned |
comment:5 Changed 10 years ago by
Status: | assigned → review |
---|
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.
comment:6 Changed 9 years ago by
Status: | review → review_failed |
---|
branch:t/13429d looks like the right solution for me. However:
- Instead of
insertRange.setStartAt( anchor, CKEDITOR.POSITION_BEFORE_START ); insertRange.setEndAt( anchor, CKEDITOR.POSITION_AFTER_END );
usesetStartBefore
andsetStartAfter
. - 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.
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:- Visit http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed
- Set the following selection
<p>Foo bar.</p> <p>Fo^o bar.</p>
- Paste the twitter link https://twitter.com/reinmarpl/status/573118615274315776 and wait for embed to finish.
- Set the following selection
<p>Fo^o bar.</p> <p>Fo</p> <twitter widget> <p>o bar.</p>
- Paste another link http://i.imgur.com/Z3ilPBI.jpg
- Select
<twitter widget>
before embedding finishes. - 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.
- Instead of
if ( range != null ) { selection.selectRanges( [ range ] ); }
you can use shorterrange && range.select();
- What if instead
if ( anchor.contains( range.startContainer ) || anchor.equals( range.startContainer ) || anchor.equals( range.endContainer ) || anchor.contains( range.endContainer ) ) { range = null; }
we usedCKEDITOR.dom.range.intersects
from #13453 likeinsertRange.intersects( range )
? We could merge #13453 before this ticket to use it. - 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 conditionelse 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 inCKEDITOR.env.ie && CKEDITOR.env.version < 9
condition. I don't know which cases it fixes though. - You mix assertion messages like
assert.isTrue( range.collapsed, 'selection after paste is not collapsed' );
vsassert.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 9 years ago by
Milestone: | CKEditor 4.5.2 → CKEditor 4.5.3 |
---|
comment:8 Changed 9 years ago by
Status: | review_failed → assigned |
---|
comment:9 Changed 9 years ago by
Status: | assigned → review |
---|
I rebased branch:t/13429d with master and pushed some commits.
Some comment on review_failed:
- Done.
- You are right. I haven't thought of all scenarios. I changed the algorithm so it uses bookmarks now.
- 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.
- 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). - This got changed and is, hopefully, cleaner now.
- 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 9 years ago by
Status: | review → review_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 9 years ago by
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 )
?
comment:12 Changed 9 years ago by
Clueless reply: endNode may not exist if bookmark was made for a collapsed selection.
comment:13 Changed 9 years ago by
endNode
is set to startNode
if it was null
. Its made like this to simplify further code.
comment:14 Changed 9 years ago by
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 9 years ago by
Status: | review_failed → review |
---|
I pushed a commit to branch:t/13429.
comment:16 Changed 9 years ago by
Status: | review → review_passed |
---|
comment:17 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged git:824cbc6 into master.
Good job!
comment:18 Changed 9 years ago by
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?
Yup. Upon widget insertion a bookmark should be made and restored afterwards.