Ckeditor5: Delete element attributes once it has any content

[Reinmar]

See https://github.com/ckeditor/ckeditor5-enter/issues/40:

The style should be removed from a block once something is inserted in it. Who's job should it be? Can we actually understand which attrs applied to a block are inline attrs? I remeber we discussed this but I don't know how we implemented it.

And:

We talked briefly with @scofalik and it seems that the listener which clears the element attributes (they can be recognised by the selection: prefix) will need to be placed in the Document.

It should be fairly simple – it needs to listen to insert and move operations and just clear attributes of an element to which insertion happens. It doesn't even need to check if it's empty or not – if, by any coincidence the attributes weren't cleared before they can be cleared now.

[Reinmar]

@scofalik: Which batch should I used for that? It's not just selection's attributes here, but element's.

[Reinmar]

We discussed this and checked how applying attributes to empty selection work in GDocs and in CKEditor 5.

GDocs:

• applying bold in an empty paragraph creates an undo step with this action:

  

• applying bold in a text creates empty undo steps and causes some bugs:

  

CKEditor 5:

• applying bold in an empty paragraph creates an undo step (restoring selection after redoing enter key doesn't work, but this is a separate issue and will be handled soon):

  

• applying bold in a text doesn't cause unnecessary undo steps:

  

So, our undo implementation and selection+attributes mechanisms are really good.

[Reinmar]

@scofalik: Which batch should I used for that? It's not just selection's attributes here, but element's.

This will be simple – we should use the batch in which the changes (inserting the content) were applied. We shouldn't touch transparent batches though to not break collaborative changes (they should've had this change already).

BTW, attributes are already copied to the element if they are set on the selection. This is done by LiveSelection itself and it uses its own batch for that. It's that batch what creates an undo step in the first tested scenario.

[scofalik]

As I've already been suggesting - any "autofixing" callbacks on change or changesDone should ignore all transparent batches.

[Reinmar]

I realised that the fact that LiveSelection.setAttribute() changes the model means that there can be just one LiveSelection per document. No one should ever create other instances of it because that might lead to strange behaviours.

If we're fine with that, I'd implement the additional "remove attrs from element on change" inside LiveSelection as well and document it as "for internal use only".

[scofalik]

I don't want to be that guy, but I wanted to call it DocumentSelection. :P

[Reinmar]

Well, at this point this is a good point. AFAICS, we have just one LiveSelection instance now.

I'd even remove createFromSelection() (and other static methods) from it (since it's not used anywhere) and consider throwing from clone() (EDIT: there's no clone() fortunately).

OTOH, this is not the cleanest design... If we'd move the entire code to Document we could still make it reusable. It's not that far.

OTTH:

[scofalik]

I'd leave it as it is. No point in introducing mess to the project at this moment.

[Reinmar]

The point is that someone may mistakenly clone document.selection and there will be blood. Besides, if we claim that there's just one live selection per document, then why these methods? Let's better secure the code.

[Reinmar]

PS. There's no clone() method, so we're talking purely about createFromSelection(). This is good news.

[Reinmar]

OK, I made my mind:

1. Rename to DocumentSelection and document as internal.
2. Throw from createFromSelection(). Since this is a static method and you need to explicitly import LiveSelection it's absolutely ok if we throw.
3. Implement the whole behaviour in documentselection.js to keep things in place. We could even consider moving https://github.com/ckeditor/ckeditor5-engine/blob/6217ea4a93b8dbc51c1b102f585cbed50d321aee/src/model/document.js#L115-L128 to that file (but I'd skip this for now).

Better solution for the future – documentselection could expose injectDocumentSelection( document ) or a similar function as a default so the purpose of this module is clearer.

[Reinmar]

On thing that I'd like to understand is why _getStoredAttributes() has that isCollapsed && childCount check:

    * _getStoredAttributes() {
        const selectionParent = this.getFirstPosition().parent;

        if ( this.isCollapsed && selectionParent.childCount === 0 ) {
            for ( const key of selectionParent.getAttributeKeys() ) {
                if ( key.indexOf( storePrefix ) === 0 ) {
                    const realKey = key.substr( storePrefix.length );

                    yield [ realKey, selectionParent.getAttribute( key ) ];
                }
            }
        }
    }

If selection attributes are meant to only exist on empty elements, does this check make any sense?

[Reinmar]

There's something wrong either with maaany tests (more likely) or with the change event's docs. Once I started checking batch.type it turned out that frequently, batch isn't passed to the change event. I guess that's due to tests not wrapping deltas in batches. Am I right?

E.g. all the delta/transform/_utils/utils.js helpers don't do that. Can I do such a quick fix there:

+ some comment that Batch() actually needs a document, but these are only tests?

[Reinmar]

I can see that so far the last change's param was used only in undo and one manual test ;< So there's many tests which I'll need to fix.

[Reinmar]

Another problem with change – its doc says that it always contains data.range which is not true – at least in case of rename it doesn't. Also, root and marker operations don't contain that property.

Second thing – there's data and changeInfo mentioned – both are the same thing.

[Reinmar]

OK, I fixed part of the tests, but I find it suspicious that many of them fire the change event manually and explicitly pass null instead of the batch. What does it mean? Why was that null added there?

[pjasiun]

We know that, for now, the change event is a mess. In fact, it should be fired per-delta, not per-operation. It's a part of the big Refactoring: https://github.com/ckeditor/ckeditor5-engine/issues/679#issuecomment-276360352.

[scofalik]

It's because change event got expanded (it happened twice already, first we added batch, then deltaType (or was it delta? I don't remember)).

Instead of creating dummy batch in all tests, I've added null. (TBH I don't know if it was me but it sounds like me).

[Reinmar]

OK, thx. I'll fix those tests which I can fix quickly so the code is hit enough.

[Reinmar]

I can't fix some tests because they only mock operations (e.g. in liverange and liveposition tests) but the model is empty, so the check fails on trying to query changes.range.start.parent.