Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#11064 closed Bug (fixed)

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

Reported by: Piotr Jasiun Owned by: Marek Lewandowski
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.6.1
Component: General Version: 4.0
Keywords: Blink Webkit Cc:

Description (last modified by Marek Lewandowski)

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:

Change History (25)

comment:1 Changed 10 years ago by Jakub Ś

Keywords: Blink added
Status: newconfirmed
Version: 3.0

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

comment:2 Changed 10 years ago by Jakub Ś

Version: 3.04.0

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

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

Summary: [Blink]Can not select all when widget is last element[Blink] Can not select all when widget is last element

comment:4 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by Piotrek Koszuliński

DUP reported: #12422.

comment:6 Changed 10 years ago by Piotrek Koszuliński

DUP reported: #12454.

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

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

Keywords: Webkit added
Summary: [Blink] Can not select all when widget is last element[Blink/Webkit] Can not select all when widget is last element

comment:9 Changed 10 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:10 Changed 8 years ago by Jakub Ś

#14404 was marked as duplicate.

comment:11 Changed 8 years ago by Jakub Ś

#14405 was marked as duplicate.

comment:12 Changed 7 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1
Priority: NormalHigh

comment:13 Changed 7 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:14 Changed 7 years ago by kkrzton

Milestone: CKEditor 4.6.1
Priority: Must have (possibly next milestone)Normal
Status: assignedreview

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.

comment:15 Changed 7 years ago by kkrzton

Priority: NormalMust have (possibly next milestone)

comment:16 Changed 7 years ago by kkrzton

Milestone: CKEditor 4.6.1

comment:17 Changed 7 years ago by Marek Lewandowski

Status: reviewreview_failed

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.

comment:18 Changed 7 years ago by kkrzton

Status: review_failedreview

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.

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

comment:20 Changed 7 years ago by kkrzton

Status: reviewreview_failed

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?

comment:21 in reply to:  20 Changed 7 years ago by Marek Lewandowski

Owner: changed from kkrzton to Marek Lewandowski
Status: review_failedreview

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.

comment:22 Changed 7 years ago by kkrzton

Status: reviewreview_passed

Ok, looks good :)

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

comment:23 Changed 7 years ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with c08aa2c.

comment:24 Changed 7 years ago by kkrzton

Created follow up ticket mentioned earlier - #16719.

comment:25 Changed 7 years ago by Marek Lewandowski

Description: modified (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy