Opened 3 years ago

Closed 3 years ago

#13377 closed Bug (fixed)

Widget plugin issue (typing Korean)

Reported by: HoonKi IM Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.7
Component: General Version: 4.3
Keywords: Cc: ihk626262@…

Description

Browser and OS : Chrome and window 7

Hi, first of all, thanks for this nice Editor

Unfortunately, when we use this, we found some bug in editor,

Actually, our team is korean. so, when we type korean in editor (using bold mode or itelic mode ...), words insert the other way(reverse)

explain detail this situation

  1. select bold mode in editor
  2. typing korean word in editor (I anticipate "ㄱㄴㄷ")
  3. first type word(ㄱ) go last position and second type word(ㄴ) go first position
  4. when I type third word(ㄷ) in editor, this word go first position and first type word(ㄱ) still last position
  5. result words is that "ㄷㄴㄱ"

We start to find reason and know something up to date.

this situation just proceed, when we use 'Widget plug-in'

If we insert extraPlugins(requires widget)in config (ex:placeholder or image2 plugin), This bug proceed.

so please check this situation.

and I attache some record image.

Attachments (2)

trim.1665EE24-839A-45E2-BF63-824F8905F260.MOV (2.5 MB) - added by HoonKi IM 3 years ago.
bug record
trim.24063564-9646-4C8F-966E-85D170419EAF.MOV (2.6 MB) - added by HoonKi IM 3 years ago.
bug record(show This!!!!!!!)

Change History (24)

Changed 3 years ago by HoonKi IM

bug record

Changed 3 years ago by HoonKi IM

bug record(show This!!!!!!!)

comment:1 Changed 3 years ago by HoonKi IM

Cc: ihk626262@… added

Hi, 6/8 I found korean typing bug.

when I typing korean in editor (bold or italic mode)

word insert backwards.

http://dev.ckeditor.com/ticket/13377 (You can see more detail info in this link)

so we find reason in widget plugin.

widget plugin code line 2754 (code : widgetsRepo.editor.fire( 'lockSnapshot' );)

when we remove this line, korean typing Normally!!!!

but, I worried about this activity. it probably effect other plugin....

please check this logic.

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

Status: newconfirmed
Version: 4.5.0 (GitHub - major)4.3

I can confirm that similar issue is reproducible with Hiragana.

  1. Enable placeholder plugin.
  2. Type few characters and accept the composition - should work ok.
  3. Click bold button and try the same - only one character can be typed - composition is broken.

I also confirmed that this line causes it: https://github.com/ckeditor/ckeditor-dev/blob/b6ccc12e6aada32c286cfbc194c519e2f673074f/plugins/widget/plugin.js#L2663

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

After some deeper investigation I ended in the usual place – selection – https://github.com/ckeditor/ckeditor-dev/blob/b6ccc12e6aada32c286cfbc194c519e2f673074f/core/selection.js#L842-L900

And I think that there are two issues or two possible approaches to the problem:

  1. Make commit() in the widget repo to be called less frequently. It does not make sense to execute it when user is only typing.
  2. Somehow block undo manager when user is composing a character. I don't know however how it could work in practice.

I would actually like to work on both topics, but the second will have a greater and more positive impact on editor. The first will be simply a performance optimisation.

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

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

Milestone: CKEditor 4.5.1

comment:5 Changed 3 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:6 Changed 3 years ago by Artur Delura

Same story as previously. We modify content during composition. We shouldn't block it only in undomanager, but we should prevent from modifying content during the composition. According to this we can listen to composition* events and set appropriate property on editor like compositionPending so plugins might check this property and react properly.

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

That would make sense... I'm only worried what happens if the code in the selection module will be clocked during composition. We will still have the ZWS chars leaking. But perhaps it's better than broken composition...

comment:8 Changed 3 years ago by Artur Delura

I pushed solution proposal in branch:t/13377. I don't know what you mean in your last comment @Reinmar.

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

What I meant is that your patch simply block the ZWS-char-fix while user is composing. It means that it doesn't do its job then. And the job of the code that you changed is to filter out ZWS characters when editor.getData() or editor.getSnapshot() run, because otherwise this character will end up in the data or snapshot.

I never liked this implementation, but there are reasons for it. At least partially... The getData() case can be changed IMO. We could filter out the ZWS char in the data processor, however, this means that no one can use this character in they data.

However, it's not that simple with editor.getSnapshot() because with snapshot goes bookmarks. And we would need to change this bookmakrs as well. This sounds feasible, but it means a lot of work.

comment:10 Changed 3 years ago by Artur Delura

I have another solution which we mentioned once. Since now ZWS-char-removal/restore was applied on each user key press. Now it's only restored when needed i.e if there is other characted in ZWS element. Otherwise we don't need it.

Changes in branch:t/13377b.

comment:11 Changed 3 years ago by Artur Delura

I'm not sure whether I'm going the right direction. I pushed another commit to the branch.

It's gonna be broken when we call getData continuesly

setInterval( function() {
    console.clear()
    var e = CKEDITOR.instances.editor;
    console.log( 'content', e.editable().getHtml().replace( /\u200b/g, '@' ) );
    console.log( 'data', e.getData().replace( /\u200b/g, '@' ) );
}, 2000 );

Because ZWS is removed then once. But in general that code fix this issue. Also two tests fail, but I it should http://tests.ckeditor.dev:1030/tests/core/selection/editor, because it checks whether ZWS is restored.

comment:12 Changed 3 years ago by Szymon Cofalik

Owner: changed from Artur Delura to Szymon Cofalik

comment:13 Changed 3 years ago by Szymon Cofalik

Proposed solution: branch:t/13377c.

I tried to find a different approach to the problem. The thing that broke composition was not inserting or removing ZWS itself but selection changes. Selection unfortunately had to be moved when ZWS was removed and then restored in beforeData(). The important note is that the ZWS was removed only for snapshot.

My idea is not to remove ZWS in beforeData() from DOM but instead mark it in non-intrusive way and remove it from snapshot string.

In my solution I check ZWS's position in innerHTML of it's parent and write that value as parent's attribute. This way it can't be confused and does not make any impact on actual contents. There is one danger, though. If the contents of ZWS parent changes after we calculate it's position, it may be invalid. My question is if such situation may happen? If so, the method would need some tweaking (see below) but I think this is may be a way to go if we don't want to make workarounds like suppressing events if composition started.

A further tweak may involve saving ZWS in a globally accessible space (like editor instance) so undo manager can read it on it's own and calculate its position after all beforeUndoImage events end. This way we achieve three things:

  1. We are guaranteed that editor contents won't change because of other event listeners.
  2. We get rid of events in selection code.
  3. We don't have to make a mess with another data attribute.
Last edited 3 years ago by Szymon Cofalik (previous) (diff)

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

Milestone: CKEditor 4.5.2CKEditor 4.5.3

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

We need to speed up 4.5.3, so we won't be able to include this task in it.

comment:16 Changed 3 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.3CKEditor 4.5.4
Summary: Widget Plug in Bug (typing Korean)Widget plugin issue (typing Korean)

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

Milestone: CKEditor 4.5.4CKEditor 4.5.5

@a.nowodzinski is working on this issue (and all other ZWS/IME related things) and his work in branch:zws looks very promising. But still... we need more time.

comment:18 Changed 3 years ago by Olek Nowodziński

A ticket with the new solution #13816.

comment:19 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

This issue will be fixed by #13816 in the next milestone.

comment:20 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:21 Changed 3 years ago by Marek Lewandowski

#13801 was marked as duplicate.

comment:22 Changed 3 years ago by Marek Lewandowski

Resolution: fixed
Status: assignedclosed

Fixed with git:2d078ff42a by #13816.

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