Opened 10 years ago
Closed 9 years ago
#13284 closed Bug (fixed)
Cannot drag'n'drop widget if a text caret is placed just after widget instance
Reported by: | Pawel Pecio | Owned by: | Szymon Cofalik |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.4 |
Component: | Core : Selection | Version: | 4.4.6 |
Keywords: | Blink Webkit | Cc: |
Description
User is unable to drag and drop widget instance using its handler if a text caret is placed just after the widget (example: some text [Inlinewidget]| rest of the text). On mouse down click there is an exception:
Uncaught IndexSizeError: Failed to execute 'extend' on 'Selection': 1 is larger than the given node's length.
Bug was reported only for Chrome browsers (all the latest versions).
Steps to reproduce:
- On CKEditor widgets demo page go to the mathematical formulas demo.
- Select a formula widget instance by click
- Type right arrow key once to unfocus the widget instance, text caret should appear just after the widget instance
- Try to drag the widget using drag handler, the exception should be thrown
I've found that the bug is connected with "bookmarks". The "source" of the problem is finalizeNativeDrop
function when saveSnapshot
event is triggered. One of its handler(s) uses bookmarks and call moveNativeSelectionToBookmark
function. Stack trace reports call from instanceReady
handler in selection component. There are following code:
editor.on( 'beforeUndoImage', beforeData ); editor.on( 'afterUndoImage', afterData ); editor.on( 'beforeGetData', beforeData, null, null, 0 ); editor.on( 'getData', afterData ); (... and in afterData function ...) if ( fillingChar ) { fillingChar.setText( fillingCharBefore ); if ( selectionBookmark ) { moveNativeSelectionToBookmark( editor.document.$, selectionBookmark ); selectionBookmark = null; } }
By unknown reason (for me), selectionBookmark is an empty textnode with offset property set to 1. This causes an exception in
sel.extend( bm[ 1 ].node, bm[ 1 ].offset );
which cannot move selection to position 1 when the node length is 0.
Change History (18)
comment:1 Changed 10 years ago by
Keywords: | Blink Webkit added |
---|---|
Status: | new → confirmed |
Version: | 4.4.7 → 4.4.6 |
comment:2 Changed 9 years ago by
There are three related issues: #13389 #13307 #13284
Another one set of steps to reproduce.
- Open http://presets.ckeditor.dev/4.4.8/full-all/ckeditor/samples/plugins/placeholder/placeholder.html
- Select text "are"
- Make some placeholder on the selection.
- Removed first placeholder (Click on it to select, and press backspace/delete)
- Try D&D placeholder to the left
There is an error in console:
Uncaught IndexSizeError: Failed to execute 'extend' on 'Selection': 1 is larger than the given node's length.
comment:3 Changed 9 years ago by
Milestone: | → CKEditor 4.5.1 |
---|
Let's add this ticket to 4.5.1 so we remember to check all the related issues. Perhaps it is a single issue.
comment:4 Changed 9 years ago by
Owner: | set to Szymon Cofalik |
---|---|
Status: | confirmed → assigned |
comment:5 Changed 9 years ago by
Owner: | Szymon Cofalik deleted |
---|---|
Status: | assigned → confirmed |
comment:6 Changed 9 years ago by
Owner: | set to Szymon Cofalik |
---|---|
Status: | confirmed → assigned |
comment:7 Changed 9 years ago by
Partial, ugly fix available at branch:t/13284. It fixes #13389 and probably #13307. With this fix error doesn't occur but there is other problem which prevents user from dragging, so this is not a full fix.
Below is a description of my struggle with this part of the code:
I tried to debug this weird case and really weird things are happening:
function moveNativeSelectionToBookmark( document, bm ) { var sel = document.getSelection(), range = document.createRange(); range.setStart( bm[ 0 ].node, bm[ 0 ].offset ); range.collapse( true ); sel.removeAllRanges(); //#1 sel.addRange( range ); //#2 sel.extend( bm[ 1 ].node, bm[ 1 ].offset ); }
Comments added by me.
So in scenario described in this ticked, at #1
bm[ 1 ].node.textContent
equals \u200B
(ZWS). But after executing sel.addRange( range )
it suddenly becomes empty! It looks like te content was cleared upon addRange()
. Since it is empty, extending selection results in error about wrong node's length.
So I decided to comment all lines in the project that has something to do with selectionchange
- just to be sure. And the bug still persist.
So I thought this is some kind of a bug in Chrome. I created short script that tries to recreate what is happening in our buggy function:
<div id="myDiv"></div> <script> var d = document.getElementById('myDiv'); var span = document.createElement('span'); var tn = document.createTextNode('\u200B'); span.appendChild(tn); d.appendChild(span); var sel = document.getSelection(); var range = document.createRange(); range.setStart(tn, 1); range.collapse(1); sel.removeAllRanges(); sel.addRange(range); console.log(tn.textContent == '\u200B'); </script>
But the console showed "true" so ZWS was not replaced. I tried similar code in editor's iframe with no luck.
In order to debug it precisely I set window.xx
in mousedown
event and then put if (window.xx) debugger;
in moveNativeSelectionToBookmark()
- so I would stop in correct call (that's because this function is being called dozens of time). I think that you are already predicting what happened... when console made breakpoint everything worked well...
So the only thing I could do is just "ugly" workaround which is available on branch:t/13284.
comment:8 Changed 9 years ago by
DnD is prevented thanks to moveNativeSelectionToBookmark
. To summarize it: after mousedown on a drag handler, in "fillingChar mechanism", the selection is moved to the text node with ZWS. This breaks dragging of the drag handler. When I move the mouse cursor, I select whatever was in bookmarked (ZWS) node.
comment:9 Changed 9 years ago by
Status: | assigned → review |
---|
Pushed some commits to branch:t/13284
git:e191f4be
Fixes drag and drop prevention when selection was on ZWS. I had to prevent moving selection to a fillingChar
in an elegant way and this is best solution I came up with. If I clear the selection, "fillingChar" mechanism will not fire.
Does it break UX for a user? Let's assume that the user have selection other than on dragged widget. Then:
- if they just click on drag handler, they focus the widget,
- if they mousedown and click, they focus the widget.
So no matter what the user does, they will "lose" current selection anyway. The only difference might be for snapshots but from my tests it doesn't affect undo manager.
Note: this commit alone fixes bug described in this ticket, but the first commit also fixes a bug (and fixes #13389)
Note: if you know there might be other similar cases in other parts of code, let me know.
Also added test.
comment:10 Changed 9 years ago by
This looked similar to issue #13515 so I grabbed a copy of the fix and tested it to see if that issue was also resolved. It does appear to fix the problem so that the javascript exception does not occur, however, it introduced a minor annoyance where it now returns a \u200b character when calling getData().
comment:12 Changed 9 years ago by
@jbalinski: Could you provide a more expanded description of what exactly you do?
I took a sample provided in attachment from #13513 (the ticket you linked) and checked out to branch:t/13284. Then, I paste an image, fire .getData().indexOf('\u200b')
and it returns -1
-- seems like it's alright. If you are still experiencing the issue you described, please consider making a new ticket with detailed but minimal scenario and link it in comment to this ticket.
comment:13 Changed 9 years ago by
I know that I've seen it but I also can't seem to reproduce it anymore. I did find a couple other workflows though when the exception described above is thrown, even with the patch applied. I'm documenting them in #13513.
comment:14 Changed 9 years ago by
Milestone: | CKEditor 4.5.2 → CKEditor 4.5.3 |
---|---|
Status: | review → review_failed |
Unfortunately I cannot review this ticket and #13513. It's too many changes, too many mentioned TCs, but too few tests supporting them.
The rule (especially important in case of so complex systems) is that if possible (and in this case it must be possible if you were able to reproduce all the problems) each change should have a test. It must be clear why it was made and it must be testable, otherwise I have no way to verify whether it's good and whether it's the only possible way.
For instance, there's git:47ba21f103. How am I supposed to verify it? To verify that such case really takes place? I need a MT (or AT if possible, but I don't think it will be possible in this case) where I can see that this specific change fixed something.
The same here: git:e614c0a1. It's good that it's a separate commit, because AFAICS it is a follow-up for the previous one, so again, there should be some test which verifies that this code works.
And then we've got git:e191f4be. It's absolutely unclear what this code does.
Final note – this and #13513 are very hard tickets and the touched system is very tricky and also very important. All changes must be covered with all possible automated tests and some manual tests for final verification.
PS. It seems that this ticket fixes two things – general issues in the selection system and later DnD of widgets. Consider splitting it into two parts. You could merge #13513 into the general part because I can see you touch the same code.
comment:15 Changed 9 years ago by
Status: | review_failed → review |
---|
I've pushed branch:t/13284b.
1) I dropped git:47ba21f103 and git:e614c0a1 commits that you have mentioned. As you've written, these were general fixes to the selection mechanism. Since #13513 I think that these are overcomplicated. Solution from #13513 achieves the same.
2) As for git:e191f4be - I described it in one of previous comments. I expanded comment in the code so it is more insightful but in-depth explanation is too long so I will leave it on a trac.
3) Since now this branch fixes only the problem with dragging a widget when cursor is exactly after it, I hope that the single manual test for this case is enough. (I've added some more tests for #13513).
4) I will verify if general fix from #13513 fixes also #13389 and #13307 and take appropriate steps.
comment:16 Changed 9 years ago by
Milestone: | CKEditor 4.5.3 → CKEditor 4.5.4 |
---|
comment:18 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed on master with git:623ade3.
Thanks for comment:7, because I would need to ask you to perform exactly these steps if you hadn't written it :).
I have been able to reproduce this issue from CKEditor 4.4.6 in Blink and Webkit browsers.