Ticket #9794 (closed New Feature: fixed)

Opened 2 years ago

Last modified 18 months ago

OnChange event

Reported by: giammin Owned by: pjasiun
Priority: Normal Milestone: CKEditor 4.2
Component: General Version: 4.0
Keywords: Drupal Cc: wim.leers@…, chris.ingham@…, dahjelle.cksource.com@…

Description

Why ckeditor doesn't have a onchange event. It would be very useful to alert the editor of unsaved changes mainly in inline edit mode

Change History

comment:1 Changed 2 years ago by fredck

  • Status changed from new to confirmed
  • Cc wim.leers@… added
  • Milestone set to CKEditor 4.1

We've taked extensively about it on #900, but we're now decided to have it inside the editor as a better promoted feature.

comment:2 Changed 2 years ago by fredck

My current idea, to help on easy to implement and less performance impact, is making this featured based on the undo plugin. In fact, the "undo" button reacts in the exactly same way one would expect this event to happen.

We may either have it as a undo plugin feature or a dedicated plugin, depending on the performance impact of it.

comment:3 Changed 2 years ago by Wim Leers

Sounds sensible :)

comment:4 follow-up: ↓ 5 Changed 2 years ago by giammin

maybe a config parameter: fireonchangeevent=true

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

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

Replying to giammin:

maybe a config parameter: fireonchangeevent=true

That would be fine, at least from a Drupal perspective.

comment:6 Changed 2 years ago by fredck

  • Keywords Drupal added

comment:7 follow-up: ↓ 8 Changed 2 years ago by giammin

I don't get the dupal keyword...

this ticket is not drupal related. I don't even use drupal.

comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 2 years ago by Wim Leers

Replying to giammin:

I don't get the dupal keyword...

this ticket is not drupal related. I don't even use drupal.

This is definitely not Drupal-specific. The reason it is tagged with "Drupal" is simply because this is one of the things that CKEditor doesn't already have and that Drupal needs. Drupal 8 is going to ship with CKEditor 4, and that is why it is tagged: to indicate which tickets are related to getting CKEditor to ship with Drupal.

See http://buytaert.net/from-aloha-to-ckeditor.

comment:9 in reply to: ↑ 8 Changed 2 years ago by giammin

Replying to Wim Leers:

This is definitely not Drupal-specific. The reason it is tagged with "Drupal" is simply because this is one of the things that CKEditor doesn't already have and that Drupal needs. Drupal 8 is going to ship with CKEditor 4, and that is why it is tagged: to indicate which tickets are related to getting CKEditor to ship with Drupal.

See http://buytaert.net/from-aloha-to-ckeditor.

Ok,thanks now i get it!

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

comment:10 Changed 2 years ago by wwalc

Just to not forget: the onchange plugin has already been used for some time.

comment:11 Changed 23 months ago by fredck

  • Milestone changed from CKEditor 4.1 to CKEditor 4.2

comment:12 Changed 23 months ago by inghamc

  • Cc chris.ingham@… added

Looking forward to this...

Last edited 23 months ago by inghamc (previous) (diff)

comment:13 Changed 21 months ago by dahjelle

  • Cc dahjelle.cksource.com@… added

Looking forward to this as well! Since the onchange plugin isn't "officially" supported in v4, this is holding us back from upgrading just yet. Keep up the good work!

comment:14 Changed 21 months ago by jonnott

+1 for this .. what's the eta for v4.2?

comment:15 Changed 20 months ago by Wim Leers

Drupal's current emulation of this feature is the following:

    var editor = CKEDITOR.dom.element.get(element).getEditor();
    if (editor) {
      var changed = function () {
        callback(editor.getData());
      };
      // @todo Make this more elegant once http://dev.ckeditor.com/ticket/9794
      // is fixed.
      editor.on('key', changed);
      editor.on('paste', changed);
      editor.on('afterCommandExec', changed);
    }

Sadly, editor.getData() does not yet contain the changes made by the latest keypress!

E.g.: the value is fooba. You type an 'r' at the end of this, now the visible value is foobar. But editor.getData() will still return fooba!

The temporary solution I found was to change the changed callback to wrap the call to editor.getData() in a window.setTimeout… not very nice, but it works for now:

      var changed = function () {
        window.setTimeout(function () {
          callback(editor.getdata());
        }, 0);
      };

Drupal.org issue: http://drupal.org/node/1995730

Last edited 20 months ago by Wim Leers (previous) (diff)

comment:16 follow-up: ↓ 17 Changed 19 months ago by Reinmar

It turns out that there is an input event which is fired when contenteditable element is changed.

https://developer.mozilla.org/en-US/docs/Web/Reference/Events/input?redirectlocale=en-US&redirectslug=DOM%2FMozilla_event_reference%2Finput

Have anyone tested it? It seems to be implemented in every browser we support (except Opera, but it switches to Blink). I wonder how broken it is :D

Edit: It works pretty well. It is triggered by D&D and typing.

Last edited 19 months ago by Reinmar (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 19 months ago by Wim Leers

Very interesting! Did you test it in every supported browser already; does it work "pretty well" in all of them? :)

comment:18 Changed 19 months ago by Reinmar

Just a quick check on FF and Chrome and it works fine - it is fired on every key and drag&drop (but programatic DOM changes aren't triggering it).

In fact, we are planning (but it's hard to tell now if it is going to work) to implement onchange event using undo manager. So this event may not be required for onchange specifically, but to improve undo manager which e.g. has problems with D&D (#7422).

Last edited 19 months ago by Reinmar (previous) (diff)

comment:19 Changed 19 months ago by fredck

Our current idea is firing the "dirty" event, instead of "change". It would be named after the checkDirty method.

This means that we would not have the event fired constantly, for every single change in the editor, but instead just once, to indicated that the editor has changes. This in fact seems to be the most common requirement for it, so the hosting app UI can react to the dirty state.

This would be a much lighter event in this way, in fact.

Would this be enough? Opinions are welcome.

comment:20 Changed 19 months ago by Reinmar

  1. First thing that comes to my mind is that developers may have troubles with finding this event and understanding it right. The "change" event is well known from DOM, when the "dirty" flag is rarely used. So even googling may not help, because "ckeditor onchange" may not return anything related to this event before it will be popularized on StackOverflow, our forum etc. And this can take a lot time, because for very long there was no official solution, so old answers will be misleading.

So - one thing is consistency, but second - intuitiveness. I'd say that better to have latter one in this case, because we're making it mostly for other developers.

  1. When this event will be fired? Like we've been talking - together with undo?
  1. If "dirty" was fired there won't be another "dirty" event unless I call editor.resetDirty(). Do I understand correctly?
  1. Regardless of whether 3) is true - consider such case:
    1. I listen on "dirty".
    2. When it is fired I getData() and try to save them via XHR (and call resetDirty() if have to).
    3. Now... when will be next "dirty" fired? If there's no forced gap between them, then we can have a situation when next one is fired nearly immediately what can cause issues with concurrency.

Of course, developers should be able to implement data saving correctly, but let's face it - most of them won't know that such situation can happen and great majority of others won't know how to solve this.

Thus, I think that there should be a configurable timeout between subsequent "dirty" events. It can be easily implemented using https://github.com/cksource/ckeditor-dev/blob/t/9764/core/tools.js#L1004 It's not a complete solution for concurrency, but at least something making this situation less probable.

comment:21 Changed 19 months ago by Wim Leers

Good points by @Reinmar.

For Drupal's purposes, an isDirty event would be sufficient. But as @Reinmar says, it may be counterintuitive. And in fact, his 4th point brings us very close to an onChange event, with the caveat that it is not fired every time, but only X times per second.

I think that's the best compromise: let there be a changeInterval setting or something like that, by which the user can say "only fire the onChange event at most every 250 ms". CKEditor can then even default to a very conservative value, e.g. 2000 ms.

Note: this does not mean the first event should also be delayed by 2000 ms: the first one should fire immediately, but after that it should only fire every 2000 ms.

comment:22 Changed 19 months ago by Reinmar

PS. My main concern is that this event should be useful for developers without deep knowledge about editor, problems caused by asynchronous execution etc. I know that there's a place for 3rd party plugins to solve all these issues, but we shouldn't simplify our event too much, because the whole addition will be useless in 90% cases.

comment:23 Changed 19 months ago by j.swiderski

"dirty" event. but instead just once, to indicated that the editor has changes

How will this work? Will event inform that something was changed and we can do with it whatever we want?
What about undo manager - what will happen when user reverts changes to moment before event was fired? Perhaps this should be flag plus event?

comment:24 Changed 19 months ago by jonnott

From my developer-perspective, what I need is way of knowing if/when *anything* has changed in the editor's output since the editor was initialised. Currently I'm only able to check for key events, i.e. via:

CKEDITOR.replace('editordiv').on('key',function(){ this.updateElement(); });

I'm using FormSavior (https://github.com/ianand2/Form-Savior), and this is the only way I've been able to make it detect that something's changed.. Hence why a 'change' event would be perfect.

comment:25 Changed 19 months ago by Reinmar

Replying to Wim Leers:

I think that's the best compromise: let there be a changeInterval setting or something like that, by which the user can say "only fire the onChange event at most every 250 ms". CKEditor can then even default to a very conservative value, e.g. 2000 ms.

Note: this does not mean the first event should also be delayed by 2000 ms: the first one should fire immediately, but after that it should only fire every 2000 ms.

This is how eventsBuffer works - first "event" triggers output immediately, but a next one not earlier than given timeout after the previous one.

On the other hand - there's also no point in sending data to server after first key was pressed. Usually it's better to buffer them. So if we want to make something useful for saving data, then we should start from a nice algorithm for saving data which buffers changes in reasonable way and hide part of it under the CKEditor's API.

If we'll start designing this from "we should have an event" perspective, then we can end up with something that can't be easily used.

comment:26 Changed 19 months ago by wwalc

So correct me if I'm wrong, the correct flow to write an "autosave" feature would be to:

  • listen for the onChange event
  • when onChange is triggered, call resetDirty() immediately after getting the data from the editor

This way it would be impossible to lose the last few characters typed by the user in case of leaving the page by the user straight after the last onChange was called. The isDirty flag will let us know properly that such user has still unsave changes.

That looks fine to me. The only issue that I see here, eventually, is that dirty flag / resetDirty() is global, so there might be a problem if another 3rd party plugin is resetting the dirty flag, because it is using it for "something".

comment:27 Changed 19 months ago by fredck

I think you guys got it already. The idea about "dirty" is not bringing a weird name for the "change" event. It is in fact a different kind of event.

It is supposed to be fired whenever the "dirty" status of the editor changes to "true". In other words, calling checkDirty after that will give you "true".

Then, i would not get fired any more, unless a new change happens following a resetDirty call.

I understand that there is also a potential requirement for the "change" event, that's why I'm calling for opinions.

Btw, when it comes to timeouts... this is out of the scope of the editor. Firing an event, which is not used by the editor code, doesn't bring performance issues to the editor. It's the holding app the responsible to deal with concurrency or whatever other issue related to event frequency, based on it own requirements and parameters. Well, at least for the first implementation, so we can KISS.

comment:28 Changed 19 months ago by wwalc

The KISS approach with dirty looks reasonable to me. Actually the more I read about this, the more I see that having timeouts directly in CKEditor makes very little sense. After all if someone writes an autosave or something, he should simply do it smartly. We cannot predict all nonoptimal usages of any event and try to protect against it.

Still, reducing the onchage event to a dirty event and a global flag seems to have one drawback: suppose one wants to have

  • a word counter that counts the number of words in the editing area (people seem to use such things: http://ckeditor.com/addon/wordcount)
  • and have an autosave feature / code that checks if there are any unsaved changes

both use cases seems suitable to listen to the onchange event and do something when it is triggered, each having of course an internal interval to not make time/resources consuming actions too often.

comment:29 Changed 19 months ago by giammin

I think that the onchange plugin works quite good.

I use this code:

config.minimumChangeMilliseconds = 500;	

CKEDITORCUSTOM.setEditorSavedState = function (editor, isUnsaved) {
	editor.unsaved = isUnsaved;
	var element = document.getElementById(editor.name);
	if (isUnsaved) {
		if (element.className.indexOf(' ckeditorUnsaved') === -1) {
			element.className = element.className + ' ckeditorUnsaved';
		}
	} else {
		element.className = element.className.replace(' ckeditorUnsaved', '');
		editor.resetDirty();
	}
};
	
CKEDITOR.on('instanceReady', function (ev) {
	ev.editor.on('change', function () {
		if (!ev.editor.unsaved && ev.editor.checkDirty()) {
			CKEDITORCUSTOM.setEditorSavedState(ev.editor, true);			
		}
	});
});

the only problem is that the event is fired when a modal(link, image,etc) is opened and closed without any modification.

Last edited 19 months ago by giammin (previous) (diff)

comment:30 Changed 19 months ago by Reinmar

"KISS" is good if its result still solve some problem. I'm not saying that "dirty" event does not make sense - I don't know this. That's why I said that we should have use cases first (live if possible) and we'll verify if "dirty" at least partially help in solving them. Because there's no point in introducing useless API.

Already mentioned use cases:

  • saving data,
  • word counter,
  • ?

Does "dirty" really help in implementing them? If not - in what it helps?

The only issue that I see here, eventually, is that dirty flag / resetDirty() is global, so there might be a problem if another 3rd party plugin is resetting the dirty flag, because it is using it for "something".

I agree. This is in fact... a very important drawback. We'll make checkDirt() useless. I even remember issues reported for conflicting resetDirty() calls.

comment:31 Changed 19 months ago by Reinmar

If we'll remove the requirement to call resetDirty(), then it doesn't make sense any more to call this event "dirty". So I'd change its name to "change" and voila! We have an event which is fired after every single change in editor.

Used together with eventsBuffer in its listener it can be a good base for autosave data and word counter.

comment:32 Changed 19 months ago by fredck

The use case for "dirty" is Drupal, were the event is used to simply enable the save button, nothing else. It's similar to something like once('change').

I'm still unsure about how simpler having "dirty" can be when compared to "change". If it'll be the same effort, then ofc "change" makes more sense. We gonna probably figure this out when starting the implementation.

comment:33 Changed 19 months ago by m.guzek

  • Owner set to m.guzek
  • Status changed from confirmed to assigned

ok, I am going to start working on it. The summary of requirements before starting coding will appear here asap. Any further propositions / conclusions / suggestions very welcome.

comment:34 Changed 19 months ago by m.guzek

After some investigations, we decide to have "change event" not the "dirty event" and put requirements in the following way:

  • change event functionality is the part of undo plugin,
  • change event is triggered AFTER the change,
  • attaching handlers to event is done by "on('change')" method of editor object,
  • each change in code MUST trigger change event (1 char change as well),
  • executing the command (Bold, Italic, etc. ) MUST trigger change event,
  • undo, redo MUST trigger change event

Comments still welcome.

comment:35 Changed 19 months ago by jonnott

The above specification sounds just right to me.

comment:36 Changed 19 months ago by m.guzek

Working draft (kinda proof of concept) available for preview https://github.com/cksource/ckeditor-dev/commit/6265ce7af73c27a571d50f37b9bb9128fc52a2c1 .

comment:37 Changed 18 months ago by pjasiun

  • Owner changed from m.guzek to pjasiun

comment:38 Changed 18 months ago by pjasiun

  • Status changed from assigned to review

There is light implementation of change event. Event does not verify is content really change so in some cases it could be fired even if nothing change (only in edge cases, in exaple if you press backspace in empty editor) or it could be emitted multiple time on one change. But I think that if somebody need to be sure that content really changed he should check if content changed manually.

comment:39 Changed 18 months ago by fredck

  • Status changed from review to review_passed

I've made two commits to fix up some things. If you're ok with them, please go ahead masterising.

I think we have a good enough solution, definitely satisfying the purpose of this feature.

We may bring enhancements to it in the future, based on feedback.

comment:40 Changed 18 months ago by Reinmar

In my opinion this ticket misses some impportant tests:

  • what happens during initialization phase and after destroy,
  • if redo causes change,
  • whether change isn't caused by arrows, etc,
  • what happens on setData,
  • whether undoM#type still works - it had no tests before, but when refactoring code it should be done with at least some basic tests.

Also - we shouldn't destroy editor after every test case, because this is heavy.

Other minor issues in code:

  • curly braces were used to wrap single lines in if/else block and shouldn't be,
  • this is incorrect:
* @param {number} keystroke The key code,
* @param {boolean} isCharacter If true it is character ('a', '1', '&', ...), otherwise it is remove key (delete or backspace),

It should be {Number} and {Boolean} and there should be dots at the end of sentences. Also true should be wrapped in `` as well as other values.

comment:41 Changed 18 months ago by pjasiun

Branch is majorised and in:

About dev: I've pushed this code && style changes.

About test: I agree that there should be more tests. I will spend a while today and add some more tests.

comment:42 follow-up: ↓ 43 Changed 18 months ago by pjasiun

setData does not emit change event because setData does not save a snapshot. I think it is setData bug and this behavior should be changed in setData (in my opinion setData should create snapshot).

comment:43 in reply to: ↑ 42 Changed 18 months ago by fredck

Replying to pjasiun:

setData does not emit change event because setData does not save a snapshot. I think it is setData bug and this behavior should be changed in setData (in my opinion setData should create snapshot).

Confirmed: http://dev.ckeditor.com/ticket/5217

comment:44 Changed 18 months ago by pjasiun

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

I've added tests for:

  • what happens during initialization phase and after destroy,
  • if redo causes change,
  • whether change isn't caused by arrows, etc,

Majorised and closed:

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