[Blink/Webkit] Can not select all when widget is last element

[Piotr Jasiun]

• open Chrome or Opera,
• go to ​http://ckeditor.dev/plugins/image2/samples/image2.html
• copy an image as a last element of content,
• try to select all (CTRL+A).

You should be not able to do this.

On IE10 and FF everything works fine.

You can also reproduce that bug using:

<p>AA</p>
<p contenteditable="false">BB</p>

Do not forget to set allowedContent: true on editor to use this sample code.

---

Related browsers bugs:

• Blink's bug: ​https://code.google.com/p/chromium/issues/detail?id=415474
• Webkit's bug: ​https://bugs.webkit.org/show_bug.cgi?id=136911

[Jakub Ś]

This issue seems to be a little bit older than widgets.

With TC provided by @pjasun I wasn't able to select and copy text from CKEditor 3.0 (in both 3.x and 4.x).

[Jakub Ś]

Since we no longer develop 3.0 I'm setting start version to 4.0.

[Marek Lewandowski]

Most likely due to the same issue you can't also use ctrl+end hotkey which should move cursor to the end of the document.

[Piotrek Koszuliński]

DUP reported: #12422.

[Piotrek Koszuliński]

DUP reported: #12454.

I also noticed that we haven't reported this on Blink's and Webkit's bug trackers.

[Piotrek Koszuliński]

• Blink's bug: ​https://code.google.com/p/chromium/issues/detail?id=415474
• Webkit's bug: ​https://bugs.webkit.org/show_bug.cgi?id=136911

[Jakub Ś]

#14404 was marked as duplicate.

[Jakub Ś]

#14405 was marked as duplicate.

[kkrzton]

This one is tricky and required a quite complex workaround. In Blink/WebKit browsers it is possible to select all content with non-editable element at the beginning/end only when there is non empty element before/after (so in fact non-editable is no longer at the beginning/end).

The proposed solution adds such element on Ctrl/Cmd + A and reselects the content. The filler element is invisible and positioned absolutely so it does not influence how the content looks in any way but makes it possible to select it all. Filler element is removed when selection changes or key is used.

As agreed earlier the whole workaround was created as a separate plugin.

Pushed fix to ​t/11064.

[Marek Lewandowski]

Functionality

Not able to select all with widget focused

1. Open an editor with a widget at the end, e.g. ​basic test End with a widget.
2. Focus the editor by clicking it.
3. Press CTRL+A.

Expected: Filler gets inserted, all the content get selected.

Actual: The filler is not created, wrong selection.

It also affects "Only non-editable" case.

Reason

The reason for it is that the fake selection container is created (div[data-cke-hidden-sel=1]) and placed at the end of the document.

The simplest starting solution is to check:

var lastChild = editable.getLast( function( elem ) {
	return elem.getName && !elem.hasAttribute( 'data-cke-temp' );
} ).$

Going further theres a ​centralized temp method.

Empty paragraph gets inserted

1. Open an editor with a widget at the end, e.g. ​basic test End with a widget.
2. Focus the editor by clicking it.
3. Press CTRL+A.
4. Press CTRL+X to cut the content.
5. Press CTRL+V to paste the contnet.

Expected: Same content gets pasted.

Actual: An empty paragraph gets inserted at the end. As a result widgetselection won't kick in for ctrl+a afterwards, because you have an empty paragraph tailing.

Code

1. ​Key checks should be moved outside of the addFillers method, to keep clear responsibility.
2. Can't we use ​keyup event here instead of adding a timeout?
3. Instead of using ​getChild to obtain first/last children(​https://github.com/cksource/ckeditor-dev/blob/3b0a5da873be64d0258b4de92f2b999f656d0729/plugins/widgetselection/plugin.js#L103) you could use [getFirst](​http://docs.ckeditor.com/#!/api/CKEDITOR.dom.element-method-getFirst) / [getLast] methods.

Docs

1. ​Initial comment block should be preceded with a @fileOverview tag, so that it doesn't get picked by jsduck as a global leftover entry.

Other

• I've added ​elementspath to the manual test, so that it's easier to see the path.

Further development

It would be nice also extend this to support expanding selection e.g. with shift + left arrow.

[kkrzton]

Pushed changes to ​t/11064.

Fixed

• @fileOverview added to file description.
• ​Key checks moved outside addFillers method.
• Adjusted manual tests - while the basic and paste manual tests were intended to not use clipboard plugin as I added image2 plugin (which requires widget which requires clipboard plugin) it was included anyway. I fixed those tests so all combinations: without clipboard and undo, with only clipboard, with only undo, with both clipbaord and undo could be tested

Issue 1

This is the funny one. Actually, the cause was a little different and I was able to recreate this issue on a master branch. When you select the widget, the fake selection container is created. Then, when you press Ctrl/Cmd + A the whole content gets selected. It gets selected because of fake selection container (it works same as widgetselection filler in this situation). But when you release the keys, selectionChange (or selectionCheck) is fired - the selection was changed so fake selection container is removed (by widget plugin which manages it). When fake selection container gets removed it breaks again the selection (due to bug causing this issue) and caret is supposed to be moved to the last element which is non-existent fake selection container.

The good part is I adjusted the solution to ​be aware of temporary elements as you suggested and it solved this issue.

Issue 2

Solved by ​filtering fillers on paste. Also added ​paste unit test.

Keyup event

I recheck the possibility to use keyup event instead of deferred keydown, however there are few issues with it. The keyup event is not fired in every situation we would like it to be fired to used it for this fix. Consider the following key sequences (action - event fired):

• Ctrl/Cmd pressed - keydown
• A pressed - keydown
• A released - none
• Ctrl/Cmd released - keyup

and

• Ctrl/Cmd pressed - keydown
• A pressed - keydown
• Ctrl/Cmd released - keyup
• A released - keyup

If we would rely on keyup for the first situation the select all would be fired after A and Ctrl/Cmd key release. For the second one it would be fired after releasing Ctrl/Cmd. If you compare it to a browsers which supports such selection natively (e.g. Firefox) you can clearly see that the whole content gets selected on keydown event (so you could just press Ctrl/Cmd + A without releasing it and content will be selected). To sum up my suggestion will be to stay with a keydown event.

[Marek Lewandowski]

All the test cases I discovered earlier are fixed - great!

Please, check the changes I made on top of yours:

1. We also need to add selectall plugin integration, I already ​added manual test for that, and also integration code in the widgetselection plugin itself.

Code

• I've removed evt parameter from addFiller method as it's no longer in use.
• Removing filler on paste - I think that the safest way is to remove any filler element, despite of it's position - but let's make it as a follow up in 4.6.2.

Tests

• I've added undo to the most manual tests. It will make manual testing easier.
• I removed tests related to not having undo loaded, as missing undo plugin should not affect in any way the solution.
• I've paste.md has become image2.md manual test - the purpose there is to test widgets + clipboard integration with widgetselection.

[kkrzton]

Looks good :)

Just two thing to consider:

1. Fillers are added/removed only when not the whole content is selected. When adding it makes selecting on a whole content possible, when removing it works as a cleanup (meaning that previously the whole content was selected). While ​adding fillers before selectAll command makes perfect sense I have some doubts about ​removing them after command excution.

First of all, the fillers will not be removed because the whole content is selected (the code disallows to do so because it will break the selection again - due to the bug). I think this will make sense for the case when using selectAll command deselects content or changes selection (changes from select all ofc), but I am not sure if such situation may happen with current implementation.

2. I think that the safest way is to remove any filler element, despite of it's position - ​cleanPasteData method removes both fillers from the beginning and end so it should be sufficent, because for now those are the only allowed filler types. Or maybe you meant the g flag in the replace method in case of duplicate fillers?

[Marek Lewandowski]

Replying to k.krzton:

1. Fillers are added/removed only when not the whole content is selected. When adding it makes selecting on a whole content possible, when removing it works as a cleanup (meaning that previously the whole content was selected). While ​adding fillers before selectAll command makes perfect sense I have some doubts about ​removing them after command excution.

First of all, the fillers will not be removed because the whole content is selected (the code disallows to do so because it will break the selection again - due to the bug). I think this will make sense for the case when using selectAll command deselects content or changes selection (changes from select all ofc), but I am not sure if such situation may happen with current implementation.

I see now, in fact this code was effectively doing nothing. Removed.

2. I think that the safest way is to remove any filler element, despite of it's position - ​cleanPasteData method removes both fillers from the beginning and end so it should be sufficent, because for now those are the only allowed filler types. Or maybe you meant the g flag in the replace method in case of duplicate fillers?

Well, I'd say a mix of two - cleanPasteData could be simply modified to remove any elements with any data-cke-filler-webkit attribute value, and the regexp would be global just like you suggested.

In a similar fashion removeFillers would remove _any_ *[data-cke-filler-webkit] element. It would be simpler, and safer in case when some external API adds something to the end of the editable. But it deserves to be extracted into a follow-up ticket in 4.6.2.

On top of that I added minor API docs changes.

[kkrzton]

Ok, looks good :)

I will create the follow-up ticket with the proposed improvements described.

[kkrzton]

Merged to master with ​c08aa2c.

[kkrzton]

Created follow up ticket mentioned earlier - #16719.