Check whether it's optimal that selection updates its own attributes on #changesDone and #change:range

[Reinmar]

Quote from https://github.com/ckeditor/ckeditor5-core/pull/234

I'm not sure on what events the selection should update its attributes itself. This should not conflict with the situation when the feature sets the selection and its attributes (e.g. bold command when executed will add attributes to the selection and that will, if I understand the flow correctly, trigger #changesDone).

[pjasiun]

Now we have two pieces of code in document: the https://github.com/ckeditor/ckeditor5-engine/blob/b13aadd0fc9b42e9fff70f7bb1b847ba14581ecc/src/treemodel/document.js#L86-L93.

First:

this.selection.on( 'change:range', () => {
    this.selection._updateAttributes();
} );

Looks like a code which should be moved to the selection. It makes perfect sense for the selection to update attributes when we change ranges. Keeping this code in the document is... lets say "strange".

Second:

this.on( 'changesDone', () => {
    this.selection._updateAttributes();
} );

Should be removed. If there is collapsed range in the text, without attributes (foo[]bar) and one press a bold button, we want the changesDone event to be fired, because it causes new selection rendering (foo<b>[]</b>bar). But if the _updateAttributes will be called, then attributes will be removed from the selection and not rendered (foo[]bar). I think that user should manually update attributes of the selection as long as he does not change ranges.

[scofalik]

Right now Selection has it's own _getDefaultRange method that returns the "default range" for given document if there are no ranges in Selection. The method is wacky because it uses schema which is ModelDocument property and all it does looks like it's more of ModelDocument responsibility than Selection.

_getDefaultRange should be moved to ModelDocument and Selection should use it when getting ranges if there are no ranges in it.

[scofalik]

I am afraid that event on changesDone is needed. First of all, change:range is fired only when ranges are changed through LiveSelection API. It is not fired when they change due to model document changes. Secondly, selection ranges would not change after attribute operation is applied but selection attributes would have to be refreshed anyway. This is true for both collapsed and non-collapsed selection.

I can't imagine that user has to manually fire some methods on LiveSelection each time they change attribute using Batch API or LiveSelection API. It feels very wrong to not have this automated.

However, I understand that the issue is important and urgent. Unfortunately it's hard to come up with completely different and satisfying solution. With no doubt, LiveSelection has to have it's attributes updated when document changes or it's ranges change. Maybe there's a better way to generalize/abstract it, but for now it's clear that we have change:range and changesDone events for this. So I think that we need to just fix the current situation.

1. All methods of LiveSelection attributes API should be wrapped in enqueueChanges so they actually fire conversion events.
2. We need to ensure that attributes set by LiveSelection attributes API are not overwritten with _updateAttributes. This can be done by implementing _updateAttributes in a way that it only clears those attributes, that were added by it. There might be other ideas, but more complicated to describe and implement.
3. On change:range we have to clear both attributes added by setAttribute and _updateAttributes. After range from selection is changed, selection has to have only "inherited" attributes.

This should guarantee that selection attributes correctness.

We can also discuss efficiency:

• change:attributes should be only fired if any of attributes actually changed,
• _updateAttributes should not be fired if neither attributes nor ranges changed,
• etc.

However, in this issue, I'd focus on correctness and leave efficiency for other issue if we have problems with it. Anyway, it's worth noting, that change:attributes event is fired very frequently at the moment, which is a separate issue.

[scofalik]

Okay I gave it a little more thought and, hopefully, I have some kind of idea what should be changed. So I'll sum it up once again, because why not.

First, rendering and enqueue changes blocks:

1. Changes applied to selection have to be rendered, no matter whether ranges or attributes changed.
2. Rendering is already done on changesDone event. We should/need to avoid multiple rendering.
3. We should/need to avoid nested enqueue changes blocks. They make debugging more complex and might bring some unexpected behaviors. Also we should avoid loops/cycles inside LiveSelection both with events fired by LiveSelection and creating enqueue changes blocks.
4. Selection is changed by features. There are two ways to do so:
   • indirectly, for example a feature changes attributes on a range which interferes with selection range, or removes/inserts some nodes from the beginning of selection range (collaborative editing is one of examples),
   • directly, for example Bold sets/removes attribute bold on selection, or Undo sets restored selection ranges.
5. Indirect changes happen because of use of Batch API. Since doc.batch().xxx() calls already has to be in enqueue changes block, those changes are already in enqueue changes block so they will be rendered eventually.
6. Direct changes happen in features. In most cases those changes will either already be in enqueue changes block or are placed next to it and can be easily included in the block. Most of changes done by features are actually done in commands. When Commands will have beforeExecute and afterExecute support, _doExecute method will be automatically wrapped in enqueue changes block. This means that in near feature most calls of selection range API and selection attributes API will be also already done in enqueue changes block.

This all leads to the conclusion that:

1. We cannot add enqueue changes inside Selection code, beacuse we will create nested enqueue changes blocks.
2. But this is not a problem because most of the time that code is already called in an enqueue changes block. This means that we don't make users write a lot of redundant code.

[scofalik]

About events.

1. change:range and change:attribute events should be fired by LiveSelection. They should be fired both when range/attribute changes due to direct use of API or because of changes done to model document. To provide better flexibility, events fired by API and fired because of model changes should be distinguishable. I propose change:range:api and change:range:documentChange or data object with api flag set to true/false.
2. change:range and change:attribute may be renamed to :ranges and :attributes, I am not sure whether it will be possible and profitable to fire one event or somehow treat them separately. I'd rather have only one event and rename it.
3. I want to greatly reduce "events noise", that is firing not needed events:
   • fire change:ranges only if any range changed
   • fire change:attributes only if any attribute was added/removed/changed
4. LiveSelection will have to listen to engine.Document. Probably to change event, looking for AttributeOperation changes and checking whether ranges in AttributeOperation affect selection. Though it will be tricky, it will provide us more meaningful events.
5. There are two ways to react to indirect range changes:
   • same as with AttributeOperation
   • implement change event in LiveRange that will be fired whenever LiveRange is changed (indirectly or directly and indirectly).
6. Whenever change affects selection ranges, we mark the selection as dirty and then, on changesDone fire _updateAttributes.
7. To prevent any cycles and loops, _updateAttributes should not fire any events. This means that it should not use selection attributes API, which will fire change:attribute.
8. We also need to fire _updateAttributes whenever selection ranges are changed directly through API.
9. The last question is whether it's better to have _updateAttributes as event callback to change:range with high priority or just fire it in selection range API methods.

As I wrote 2 posts above, _updateAttributes have to re-set only those attributes that were previously set by it and haven't been changed. (In other words, attributes set by selection range API have to be preserved).

[Reinmar]

What can I say... it will take me one day to read this :D.

[scofalik]

All you have to do is +1 me, bro.

[Reinmar]

To prevent any cycles and loops, _updateAttributes should not fire any events. This means that it should not use selection attributes API, which will fire change:attribute.

So how would you know when selection attribute has changed? There will be no robust way to listen to that.

LiveSelection will have to listen to engine.Document. Probably to change event, looking for AttributeOperation changes and checking whether ranges in AttributeOperation affect selection. Though it will be tricky, it will provide us more meaningful events.

Can't engine.Document have all the bindings? So it would listen to its events and call proper methods on the selection.

As for the rest, the plan sounds ok. I can't think of a better solution.

[scofalik]

So how would you know when selection attribute has changed? There will be no robust way to listen to that.

I might forgot about it, but you would have to do it when listening on change event. When processing AttributeOperation and noticing that it is affecting current selection you'd have to generate such an event.

However. If there is no _updateAttributes() on change:attributes (or Selection change event), we can, in fact, fire this event in change:attributes.

Can't engine.Document have all the bindings? So it would listen to its events and call proper methods on the selection.

It's not that important to me, so if it looks okay and works okay, it can be done this way.

[Reinmar]

I might forgot about it, but you would have to do it when listening on change event. When processing AttributeOperation and noticing that it is affecting current selection you'd have to generate such an event.

That's what we'd like to avoid :D.

However. If there is no _updateAttributes() on change:attributes (or Selection change event), we can, in fact, fire this event in change:attributes.

I don't understand this, but I read it as "we can fire change:ranges everytime values returned by sel.getAttributes() would change.

[scofalik]

No, no, I just got confused and for a while thought that we might want to fire _updateAttributes() on change:attributes (because we did fire _updateAttributes() on change:ranges). It, should be other way around, of course.

[scofalik]

I am working on this issue. Core ideas are already implemented. What's left to do, are tests and refactor. Here's a quick summary of what will change and how:

1. All document.enqueueChanges were removed. Any changes on selection will have to be done in enqueue changes block.
2. Events fired by Selection and LiveSelection are change:range and change:attribute. Name didn't change to avoid making changes in many repos. Those events are fired with one parameter - object. Both events have directChange flag in that parameter set to true or false, depending whether the change was caused by using API (true) or changes to document (false). change:attribute gets an additional parameter attributeKeys that is an array with keys of changed/set/removed attributes.
3. Events are fired only when something really changed. This is true both for ranges and attributes.
4. Less events firing events (previously change:range always fired change:attribute AFAIR). Now the only situation when selection event fires another selection event is when setting a range. This will fire change:range (direct) and this will fire _updateAttributes, which MAY fire change:attributes (indirect) if attributes changed.
5. Event listeners were moved to LiveSelection (from Document). I am not sure which place is better for them, though.

What I don't like but will probably be left like this right now (and may need followup issues):

1. LiveSelection listens to document#event:change and looks for attribute operation. Right now, LiveSelection is stupid, and always fire _updateAttributes when attribute operation is done on model. We could make it better by checking whether range from operation and selection ranges intersect.
2. On change:range (direct) change:attribute may be called twice (first one when attributes are cleared and second when they are set by _updateAttributes).
3. Selection#setRanges always fire event, even if same ranges are set. It's like this because it caused problems in LiveSelection that inherits some stuff from Selection.

What's left to do:

1. Check code to see if there are places where selection is not changed in enqueue changes block. This should probably also include tests, as we might have hidden errors by not doing something when selection is changed. Even though test passes (but it might fail in real editor).
2. Some refactoring.
3. Tests.
4. I introduced attributes priority to ensure that _updateAttributes only deals with attributes set by it. This may be overengineering though, so I will re-consider this idea after everything else is done (so I'll have tests).
5. Docs.

I am pushing t/259 so you will be able to check the changes. Would be great to finish it on Friday but I am afraid I will need one day more.

[maxbarnas]

BTW Is there a simple way to differentiate between selection that was changed because user moved cursor and selection that changed because a text was inserted?

[scofalik]

Yes (there will be as a result of fixing this issue):

Events fired by Selection and LiveSelection are change:range and change:attribute. Name didn't change to avoid making changes in many repos. Those events are fired with one parameter - object. Both events have directChange flag in that parameter set to true or false, depending whether the change was caused by using API (true) or changes to document (false). change:attribute gets an additional parameter attributeKeys that is an array with keys of changed/set/removed attributes.

The event will have parameter with directChange flag.

[scofalik]

After writing manual test I discovered a bug (which may not be directly connected with the issue). When selection is collapsed and attribute is set on it, it is rendered, for example like this:
foo<strong>&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;^</strong>bar.

Now, after a letter is typed, all ZWSes are removed aaaandd.. selection is moved to the end of paragraph...