Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widget selection is not removed when widget is inside of a table cell #522

Closed
plblum opened this issue Jun 18, 2017 · 4 comments · Fixed by #1104
Closed

Widget selection is not removed when widget is inside of a table cell #522

plblum opened this issue Jun 18, 2017 · 4 comments · Fixed by #1104
Labels
plugin:tableselection The plugin which probably causes the issue. plugin:widget The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:bug A bug.
Milestone

Comments

@plblum
Copy link

plblum commented Jun 18, 2017

Are you reporting a feature or a bug?

Bug

Widgets have a selection mode. When they are clicked on, they get selected. When the insertion point or selected range is changed, they become unselected. This worked in 4.6.x.
When a widget is inside of a table cell (td), the selection mode fails to unselect when the creates an insertion point selection (range = collapsed). It fails to select when the cell ONLY contains the widget.

To reproduce:

  1. Go to http://sdk.ckeditor.com/samples/captionedimage.html
  2. Insert a 2 column, 1 row table as shown in the supplied image. Include text in each cell.
  3. Select the image widget and use the copy command.
  4. Move the insertion point into the first cell.
  5. Paste.
    The widget is established correctly.
  6. Select the widget. It becomes selected.
  7. Click the insertion point into the text of the second cell.

widget inside table cell 1

Expected:
Widget becomes unselected.

Actual:
Widget stays selected, while the insertion point is active where you clicked.

  1. Type.
    Expected:
    Typing to appear at the insertion point.
    Actual:
    Typing is ignored.

Other details

If you make a selected range in the other cell, unselection happens correctly.
If you omit text in the first cell, after pasting, you can click on the widget but it refuses to establish the selection.

I looked into the source code and can point out the issue.
selection.js/checkSelectionChange. This function now checks for fake selection support on tables through a call to isRealTableSelection. It does not test for fake selection used on widgets.
This is a proposed change:
if ( !realSel || ( !realSel.isHidden() && ((this.widgets.selected.length > 0) || !isRealTableSelection( realSel, sel )) ) ) {

However, I don't think its the right implementation because the fake selection engine should support user supplied selections. (I have created one for my installation.) Instead, I believe each consumer of fake selections should register a hook that is executed when realSel.isHidden() returns false. This will benefit users who are not using the tableselection plugin too.

  • Browser: Chrome Version 58.0.3029.110 (64-bit)
  • OS: Windows 7 Professional SP 1
  • CKEditor version: 4.7.0
  • Installed CKEditor plugins: While I have the tableselection plugin, the config has it in removePlugins.
@msamsel msamsel self-assigned this Jun 29, 2017
@msamsel
Copy link
Contributor

msamsel commented Jun 29, 2017

Hi,

you're I tried to reproduce mentioned problem, but without proper effect. :(
Since reporting the issue there is already Chrome 59, and we just release CKEditor 4.7.1.
I made such steps, can you confirm is this proper way of reproduction this error?

  1. Open page http://sdk.ckeditor.com/samples/captionedimage.html
  2. Create table 1 row 2 columns
  3. write something in both cells
  4. Copy image widget (Cmd+C)
  5. Paste it to table cell (Cmd+V)
  6. Click into second cell
  7. Start to typing on keyboard

Now selection should remain on the widget in 1st cell and text should not appear in 2nd cell? Am I understand you correct?
Because different things happen for me: cell text is disappearing and image widget in cell cannot be selected at all.

Could you confirm that my approach to reproduction this problem is correct? Maybe I missed something?

Browser: Chrome 59
OS: MacOS
CKEditor version: 4.7.1
plugins: full-all

@msamsel msamsel removed their assignment Jun 29, 2017
@plblum
Copy link
Author

plblum commented Jun 30, 2017

I am now using Chrome 59 and see the same problem using my original test steps.
In step 2, I typed "abc" into the first cell and "def" into the second cell.
I moved the insertion point after "abc" and used Ctrl+V to paste the image.
The image becomes selected with the blue frame that is used by widgets.
I click in "def" and the insertion point appears there but the blue frame remains around the image.
Type "g" and you expect "g" to appear as part of "def". However, nothing happens.

Another variation.
In step 2, only type "def" into the second cell and leave the first cell empty.
Click in the first cell and type Ctrl+V to paste the image.
Now try to select the image by clicking on it. It never gets the blue frame. I cannot select it and I cannot put an insertion point next to it to type in more text in the first cell.

I plan on installing 4.7.1 into my app today. Hopefully the page with the demo is already running that version.

@msamsel
Copy link
Contributor

msamsel commented Jul 3, 2017

Hi,

thank you for better description. I was able to reproduce this situation here: https://codepen.io/msamsel/pen/BZrXxj?editors=1010.
Info to reproduce it in my example:

  1. Select image in table to have blue border around it
  2. Click into 2nd cell, to put carer in it
  3. Start typing

Result:
Selection is kept on previously selected image even though caret is visible in another place. You cannot type in it.

@msamsel msamsel added status:confirmed An issue confirmed by the development team. type:bug A bug. and removed status:pending labels Jul 3, 2017
@msamsel
Copy link
Contributor

msamsel commented Jul 3, 2017

This issue: #520 might be related to this one.

@mlewand mlewand added plugin:tableselection The plugin which probably causes the issue. plugin:widget The plugin which probably causes the issue. labels Aug 8, 2017
@mlewand mlewand added this to the Backlog milestone Aug 8, 2017
@mlewand mlewand added the target:minor Any docs related issue that can be merged into a master or major branch. label Aug 30, 2017
@f1ames f1ames modified the milestones: Backlog, 4.8.0 Nov 27, 2017
@mlewand mlewand added target:major Any docs related issue that should be merged into a major branch. and removed target:minor Any docs related issue that can be merged into a master or major branch. labels Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:tableselection The plugin which probably causes the issue. plugin:widget The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants