yabab-dev / ng2-ckeditor

Angular2 CKEditor component
MIT License
358 stars 96 forks source link

High frequency of value changes #198

Closed nightullr closed 6 years ago

nightullr commented 6 years ago

It looks like the CKEditor change event kicks off more often than you might expect it to. I'm using this in my own reusable module for SharePoint forms, and the change event executes after I edit any input elsewhere on the form. It also appears to execute when you click in the editor box, click a toolbar button, etc. Since the updateValue function updates the model value each time (unless you have a debounce set), angular's valueChanges observable also presents a new value that often if you subscribe to it, even if the value didn't actually change - sometimes 2-3 times for one click. I'm using Angular 5 and CKEditor 4.8.0.

Can we update the control only if the new value is different from the last value? I can certainly submit a PR for this!

updateValue = function(value) {
    if (this.value != value) {
        . . .
    }
}
kzimny commented 6 years ago

With debounce you can add a delay when updating ngModel. Did you try to set the dobounce="500"?

<ckeditor
 [(ngModel)]="Content"
 [config]="ckeConfig"
 #ckeditor
 (change)="onChange($event)"
 debounce="500">
</ckeditor>

...

    onChange($event: any): void {
        console.log("onChange");
    }
nightullr commented 6 years ago

@kzimny I did. That does help cut it back if you are actively typing in the editor itself, but it still runs on all other inputs, plus twice as much as it seemingly needs to for actual editor changes. Even with a debounce of any length, a 1-character editor change hits the valueChanges observable twice.

kzimny commented 6 years ago

I'm not sure if the described scenario has anything to do with the ng2-ckeditor component, because

and the change event executes after I edit any input elsewhere on the form.

Please keep in mind that the most of complex problems are not necessarily related to ng2-ckeditor component. Maybe you could prepare a working example with your scenario, plunker?

nightullr commented 6 years ago

That's true - CKEditor itself is the thing firing the change event and the component is just listening for it, so the issue with it happening on my other inputs isn't directly related to ng2-ckeditor. I would say I was thinking more along the lines of, wouldn't we want to avoid unnecessary changes to the control anyway? Not taking into account my extra issue, cke intentionally fires the change event when you click/select in the text area, click any toolbar buttons, focus/blur, etc. It doesn't consider what the previous value was, though I don't think that's a problem since that's not cke's intent at their level. But angular's change listener scope is the model value, since that is what observable valueChanges is for in my opinion. Since this component is currently updating the control value regardless of whether or not the value actually changed, my "onChange" functions execute more than they need to be. I have to support IE, so I'm always watching for that... lol.

I will put together a plunkr to show the change events and their values. 👍

kzimny commented 6 years ago

Dear nightullr, not all you wrote is true. For instance:

, cke intentionally fires the change event when you click/select in the text area, click any toolbar buttons, focus/blur, etc.

and

Since this component is currently updating the control value regardless of whether or not the value actually changed

is not true. I've prepared a plunker demonstrated what happen onChange() event. Please click the "Search" button in the toolbar and you will see that the event doesn't fire. Why? Because the code doesn't change. The onChange() event fires after the code changed: image

I look forward to test your plunker.

nightullr commented 6 years ago

I figured out why mine does that - it is the plugin I have called "onChange" that fires the "change" event when interacting with the editor, not just on value changes. I believe I originally installed it because it also fires the event for source changes, which cke standard does not do. Neither the plugin nor CKEditor check for actual value changes though. My previous angularjs directive did, so I was able to listen for either model changes or editor changes, depending on what I needed to accomplish.

From CKEditor's docs:

Due to performance reasons, it is not verified if the content really changed. The editor instead watches several editing actions that usually result in changes. This event may thus in some cases be fired when no changes happen or may even get fired twice.

With the plugin removed, the "change" even is still fired twice every couple of times, though no longer on some button clicks or from the source. But anyone using a plugin that expands the change event for any reason should have this issue as well.

kzimny commented 6 years ago

What do you expect from ng2-ckeditor? Do you want ng2-ckeditor do some work more than ckeditor do? Don't forget that ng2-ckeditor is just an angular wrapper that allow interact with ckeditor. FYI: in the next release 1.2.0 you will get additional event from ckeditor. Maybe the additional event(s) would help to solve your problem. Look at question 197 asked a few days ago. I don't think that ng2-ckeditor implementation should contain fixes for ckeditor issues. Can you look at your implementation and try to find a way to solve your specific problem?

nightullr commented 6 years ago

Well, what I was thinking was this: https://github.com/nightullr/ng2-ckeditor/commit/23018486d3564d7778561d20c3b11dcf82e6d342 (Github's diff is displaying it weird, but I only wrapped that section in the new if statement.)

Anyone can make their own plugin for ckeditor, or use one that exists like onChange, that fires that change event for all editor changes instead of just content changes... it's not exactly something that ckeditor is doing wrong, I don't think. But because ng2-ckeditor is the thing responsible for setting the angular control value, and it is already retrieving the new value, the single 1-line addition of if (this.value != value) { . . . } would ensure that the control is updated only on an actual change, no matter what plugin or extra fire comes from cke's change event, intentionally or otherwise. I consider this an in-scope change actually because it's an angular wrapper for ckeditor... If ckeditor doesn't check for a changed value, and this component doesn't either, than the behavior of the valueChanges observable is left up to ckeditor and any of the dozens of plugins you can get or create yourself, which don't know or care about your angular app.

I could (and will have to for now) track the old editor values internally and compare them to the new value as the first action in my change function, but I will need to do that for every editor in every list form. This feels like the wrong place to check for that...

kzimny commented 6 years ago

To be honest, I would not make the change just because you use some plugins with the ckeditor. In your proposal line 156 and 167 there is more traffic than necessary. Maybe there are other requirements which do not prefer to update the value in real time. Would you prepare a working example demonstrated your scenario? @chymz what do you think?

nightullr commented 6 years ago

I'm not sure what you mean by more traffic than necessary. If the new value is the same as the last value, it won't attempt to update the control at all. If it did change, it would continue to check the debounce and update the control just like the component does currently. Not everyone will use that plugin of course.... I just consider that check to be a logical place and an improvement that prevents changes that aren't changes, regardless of the source of the change fire. I haven't had time to make a plunk yet but I will aim to do that this weekend. If you guys don't agree, no problem, I can use my fork as well.

Thanks!

Edit: To specify, I added lines 156 and 169 but the rest (157-168) is unmodified from the existing function, except for being tabbed.

kzimny commented 6 years ago

Unfortunately I've misinterpreted the code because of unified code view... Let the @chymz decide... Does something speak again the request?

yabab-dev commented 6 years ago

I'm okay with this new behavior, just hope that is not a breaking change for someone :P

yabab-dev commented 6 years ago

Change published in 1.2.0