Ticket #8978 (closed Bug: fixed)

Opened 3 years ago

Last modified 2 years ago

IE: Editor selection is intermittently lost when an item is selected from a toolbar menu

Reported by: tmonahan Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.4
Component: UI : Floating Panel Version: 3.0
Keywords: IE IBM Cc: damo, satya

Description

To reproduce, use the attached test case:

  • unzip linkmenu_testcase.zip. Copy the linkmenu folder to the _source/plugins directory and the config.js file to the root ckeditor folder.
  • Open any CKEditor sample in IE with debugging enabled as this sample logs content to the console.
  • Enter some text into the editor and select some of the content.
  • Open the toolbar menu item and click the 'Edit Link' option. This will open the link dialog and should log the selected text to the console.

Problem: The selected text is often an empty string even though there was text selected in the editor.

This occurs intermittently so it may take a few attempts. If you do not reproduce the issue at first, change the selection in the editor and try opening the link dialog from the toolbar menu again.

Note: If you use the link icon directly from the toolbar instead of the menu item, the selected text is always correct.

Attachments

8978.patch (500 bytes) - added by tmonahan 2 years ago.
Proposed fix
8978_2.patch (578 bytes) - added by garry.yao 2 years ago.
linkmenu_testcase.zip (1.2 KB) - added by tmonahan 2 years ago.
8978_3.patch (476 bytes) - added by garry.yao 2 years ago.
8978_4.patch (1.9 KB) - added by garry.yao 2 years ago.

Change History

comment:1 Changed 3 years ago by garry.yao

Open dialog from menu item will result in a wrong selection lock in IE in some situation, we're not likely to provide a solution to the current issue while this bug is already addressed in v4.

comment:2 Changed 3 years ago by j.swiderski

  • Status changed from new to closed
  • Keywords IE added
  • Version changed from 3.6.4 (SVN - trunk) to 3.0
  • Resolution set to wontfix

A workaround that always worked for me in IE is adding editor.focus(); before getting the selection

var editor = this.getParentEditor();
editor.focus();
var selection = editor.getSelection();   
console.log('Selection is: ', selection);
console.log('Selected Text is: ', selection.getSelectedText());

This is a won't fix in v3 but should be handled properly in v4

Changed 2 years ago by tmonahan

Proposed fix

comment:3 Changed 2 years ago by tmonahan

The suggested workaround of focusing the editor before getting the selection does not work for us. If this issue is due to a wrong selection lock in IE as per comment 1, would the attached patch be a viable solution for this? It just removes the IE specific code in the onShow() of the menu plugin which locks the selection. Why is this code in place? Would there be any undesired consequences of this change?

This issue is something our users will encounter on a very regular basis so we would really appreciate any help you can give us.

Changed 2 years ago by garry.yao

comment:4 follow-ups: ↓ 5 ↓ 7 Changed 2 years ago by j.swiderski

I'm not sure why editor.focus() didn't work in your case - could you give me a comment here. I remember testing it with default CKEditor in IE and it worked. I have tried to test this once but it seems that you have removed the plugin from the bug (not sure why).

Anyway @garry.yao has provided some patch that might work (I wasn't tested) for your case.

Changed 2 years ago by tmonahan

comment:5 in reply to: ↑ 4 Changed 2 years ago by tmonahan

Replying to j.swiderski:

I'm not sure why editor.focus() didn't work in your case - could you give me a comment here. I remember testing it with default CKEditor in IE and it worked. I have tried to test this once but it seems that you have removed the plugin from the bug (not sure why).

Anyway @garry.yao has provided some patch that might work (I wasn't tested) for your case.

I didn't remove the sample plugin from the testcase but I see that it is missing. I have just re-attached it.

When I added editor.focus() directly before the getSelection() call as you suggested, there was no difference. The selection was still lost intermittently.

The patch that @garry.yao provided does seem to be working well though. We will continue to test it further to make sure it doesn't have any adverse effects.

Thanks to you both for your help with this.

Last edited 2 years ago by tmonahan (previous) (diff)

comment:6 Changed 2 years ago by tmonahan

We have found some regressions in IE with 8978_2.patch​ applied e.g.

  • JavaScript error thrown when we try to edit Table Properties
  • JavaScript error when we delete a table cell/row/column or the entire table
  • It is not possible to change list style or start value of lists after initial style or start value is set

Therefore we need to revert this patch and find an alternative fix for the issue reported in this ticket.

Can you provide comments on 8978.patch​ which I previously suggested as a fix for this issue (details are included in comment:3 above)? Can you think of any adverse effects of this change? Do you have any other suggestions for a fix for this issue?

As mentioned previously, this is a very common usecase in our products so we really need your help with this.

comment:7 in reply to: ↑ 4 Changed 2 years ago by tmonahan

Replying to j.swiderski:

I'm not sure why editor.focus() didn't work in your case - could you give me a comment here. I remember testing it with default CKEditor in IE and it worked. I have tried to test this once but it seems that you have removed the plugin from the bug (not sure why).

Anyway @garry.yao has provided some patch that might work (I wasn't tested) for your case.

Hi, the patch proposed by @garry.yao introduced regressions as discussed in comment:6. Can you please provide feedback on my original proposed fix (8978.patch)?

Changed 2 years ago by garry.yao

comment:8 Changed 2 years ago by garry.yao

  • Status changed from closed to reopened
  • Component changed from General to UI : Floating Panel
  • Resolution wontfix deleted
  • Milestone set to CKEditor 3.6.4

comment:9 Changed 2 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from reopened to review

comment:10 follow-up: ↓ 11 Changed 2 years ago by garry.yao

Thanks for the review, would u check the 8978_3.patch is enough to eliminate this issue?

comment:11 in reply to: ↑ 10 Changed 2 years ago by tmonahan

Replying to garry.yao:

Thanks for the review, would u check the 8978_3.patch is enough to eliminate this issue?

I tried 8978_3.patch but the original issue reported in this ticket still occurs. I also tried applying 8978_2.patch and 8978_3.patch together but this still results in the regressions mentioned in comment:6.

Changed 2 years ago by garry.yao

comment:12 Changed 2 years ago by garry.yao

8978_3.patch is confirmed to not work, 8978_4.patch is supposed to fix the issue, review again?

comment:13 Changed 2 years ago by tmonahan

8978_4.patch seems to address the issue in this ticket. Can you please test this to verify that it does not introduce any regressions. Thanks.

comment:14 Changed 2 years ago by garry.yao

Providing a simpler way to reproduce for the reviewer:

  1. Open replacebyclass sample
  2. Select the word "are"
  3. Click to open "SCAYT" menu button
  4. Without clicking on any menu item, close it with ESC key
  • Actual: Cursor collapsed to the start of the selected text;
  • Expected: Text selection remains.

There's no easy way to come out a automatic test for it though, since this bug relies on mouse/keyboard event.

comment:15 Changed 2 years ago by fredck

  • Status changed from review to review_passed

comment:16 Changed 2 years ago by garry.yao

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [7518].

comment:17 Changed 2 years ago by garry.yao

Re-fixed with [7560], check #9116 for more details.

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy