Features: example feature Bold

[scofalik]

This issue discusses how an example feature Bold can be described by feature-parts: commands, converters, schema, UI. See also #129.

Feature example: Bold

Bold feature lets a user mark selected text as important (or, simply saying, mark it bold). This is one of the most common features in text editors. If selected text is already marked bold, it should un-bold instead.

Bold initialization and requirements

This feature is very simple and should not have any requirements. Initialization should only create and register proper Commands and converters. The feature will facilitate attribute with name bold only possible value true.

Bold command

Bold should register a single command under name bold. The command should be on when Selection has bold attribute, off otherwise. If command is off, executing it should add bold attribute to selected elements in Tree Model (or, if selection is collapsed, it should add this attribute to the editor's Selection instance). If command was on, it should remove bold attribute from selected elements or Selection.

Bold converters

Tree Model to Tree View converter should check whether processed tree nodes have bold attribute. If that's true and processed node is a character, it should be wrapped in strong element. Note, that the element itself should not have additional style set.

On the other hand, Tree View to Tree Model converter should recognize not only strong elements but also b elements and span elements with style="font-weight: bold;". Those elements, and characters inside such elements should be added to Tree Model with bold attribute.

Bold schema

• For Tree Model, add rule that bold attribute can be set to inline and block element (not for widgets, though).
• For Tree View, bold attribute should have default priority.

  Bold UI

Bold feature should create a button which should execute bold command.

[scofalik]

This is how AttributeCommand could be implemented. Using this you can create commands like bold, italic, etc.

I don't know if it is a good idea to have it here, but this is nowhere near final code and I put it here for discussion - whether this conceptions are good and well understood. You can make your point of view and imagine how much will it take to implement certain command. execute looks like it should be simpler, but there are a few cases we have to handle.

https://gist.github.com/scofalik/cde9c5e09f08d5c55a06

[fredck]

@scofalik : better to move that to a gist, so one can comment about specific lines with ease.

[scofalik]

I moved it to gist.

[fredck]

I see a hardcoded "bold" here: https://gist.github.com/scofalik/cde9c5e09f08d5c55a06#file-attributecommand-js-L23

[Reinmar]

We've been discussing one thing now – how the state of the attribute command depends on the selection. Do you think @fredck that it's a topic for Editor Recommendations?

[fredck]

Exactly... that was one of the things in my mind as well and ER fits well for it.

[scofalik]

Whenever me and @pjasiun were talking about this, we always assumed that the collapsed selection has same attributes as the Tree Model node that is right before it. If there are no nodes before selection, selection has same attributes as Tree Model node that is it's parent.

AFAIR we didn't make any statement about non-collapsed selection, but after talking w @Reinmar it seems that the best idea is to base selection attributes on node before / parent node of the beginning of the selection (the visible beginning, not the focus position).

This way would let us easily implement few things.

[scofalik]

BTW. we had an idea, that features like bold, if applied on collapsed selection, will just change selection's attributes. Any typing then will be based on selection's attributes. This way we won't have to create bogus elements to put selection in.

[Reinmar]

I created an issue for the state thing: https://github.com/ckeditor/editor-recommendations/issues/32

[scofalik]

I have some doubts on how AttributeCommand should work, posted here https://github.com/ckeditor/editor-recommendations/issues/32#issuecomment-183259732

[scofalik]

Connected with: https://github.com/ckeditor/editor-recommendations/issues/32#issuecomment-183350092

After F2F discussion with @fredck we decided that, for now, we will abandon saving attributes to parent node and reading them from parent node. It appeared that there are even more problematic and confusing situations if we go with saving attributes to parent.

There is also a concern, that parent might have other attributes which should not be copied to selection attributes. We would have to somehow mark which attributes are for selection.

We came up with alternative solutions, but they also have their drawbacks:

• attributes saved in parent for selection's purposes will be prefixed by selection:, so we will know how to handle them,
• a special node that won't be rendered but will keep stored attributes - the problem is that it would have to be kept in Tree Model and will mess up with algorithms that check whether a node has any children, etc.
• do not save selection attributes, but instead look for closest character on the left, even if it is in different node, to take it's attributes
• try to use LivePositions to mark places where selection should be restored (alternative to virtual-node but may be challenging to implement in collaborative editing).

If we ever try to implement saving attributes styles, we have to keep in mind:

• why we save styles only in empty paragraph? Maybe we should save them also at the end of paragraph (so, I type, click bold, change caret position, go back, maybe I want bold back)?
• what should happen if I have a one letter in paragraph with styles and then remove it -- and how to implement this in our architecture?
• remember, that if we are in empty paragraph and use a command, the result should be saved (this won't work if we simply go with 3rd solution proposed above).

[Reinmar]

Followup for my comment in https://github.com/ckeditor/editor-recommendations/issues/32#issuecomment-184122749:

https://dev.ckeditor.com/ticket/12634 is a very popular issue – it has a lot of DUPs (much more than we had noticed, because it can be reported in many various ways). Let's not repeat the mistakes of browser vendors here. Users expect that styles will be maintained in empty paragraphs and popular editors handle this as expected (I checked MS Word, GDocs and TinyMCE which workarounds the browser behavior).

So there are three use cases which IMO we must handle correctly:

• returning to an empty paragraph by arrow keys,
• returning to an empty paragraph by mouse click,
• returning to an empty paragraph by pressing backspace.

In all of them the styles that the text would have there initially, should be maintained. It's editor's implementator decision how to do this. For CKEditor 5 we'll continue the discussion in ckeditor/ckeditor5-design#128.

We discussed this topic with @pjasiun a long time ago and I think that the idea still makes sense – the attributes that the text typed in an empty block would have should be stored temporarily in its parent (in the block element).

The only issues that such a solution may have are:

1. the styles must be copied to the block on enter key and removed on first node (letter?) insertion,
2. how to differentiate between block and inline styles when they are all kept in the block,
3. how to do the above when dealing with OT (where will the conflicts be resolved – will the OT implementation know the nature of the styles?).

As for 1, up to a point it seems to be simple – the implementation of enter key feature will handle copying styles. But what with content insertion? If it was only about typing, then it would be easy, because it would boil down to a single feature. But many features will insert some content, so perhaps some automation needs to be implemented – maybe a post fixer?

As for 2, I'm for defining which attribute is block and which is inline in the schema. This information will not be only needed for the selection (it needs to know which styles to inherit from the block when placed there), but it may quickly turn out that such an information will be needed in other features. Hence, it's best to keep things public – in this case in the schema. (Note: an attribute may only be one of inline style and block style – one attribute should not have a dual nature).

As for 3, AFAIR it's the client who resolves conflicts, so it has the whole knowledge about the schema.

[scofalik]

1. We would not have to remove the attributes if they are used only when inserting content into empty paragraph. If the paragraph is empty, we simply use it's attributes (this is achieved through selection, selection might take attributes from parent node). Then, if the paragraph ever get's empty, it's attributes will be overwritten by the attributes of last removed character. We would have some garbage data in Tree Model but it will not cause any problems so we will not have to think about any cleaning algorithms right now.
2. This idea brings some confusion. Inline attributes will be setable on block elements, while block attributes will not be setable on characters. BTW. if any feature will ever use this information (or similar), will it use the same way as Selection? I am not saying that the idea is bad, I just don't like to say that an attribute is inline and then apply it to block element. As far as alternative solutions go, I like the idea of prefixing some attributes. Especially if we decide that the whole attribute-copying-and-saving-to-parent is selection's responsibility, and selection-only thing. We might have selection_store:bold attribute or something.
3. This will work as any other attributes, no conflicts will appear. BTW. Schema has not much to do with OT. The only thing we will do is post-fixing after OT (rare scenarios). But then, both server and client application will have same code, so server will be able to do so (OT will happen on server).

[scofalik]

I've pushed some code to look at on branch t/features in ckeditor5-core https://github.com/ckeditor/ckeditor5-core/tree/t/features

I will add docs and finish tests if we agree that this is a good direction to go.

[Reinmar]

I am not saying that the idea is bad, I just don't like saying that attribute is inline and then apply it to block element.

For me it's just a way to store these attributes. They represent inline styles, but are kept in a block. Kinda like in CSS where you can apply font-weight to a block or an inline element. Also note that the converter will need to create an element for such a style, even if it's applied on a block. E.g. when you have a "P" element with a "bold=true" attribute, the bold feature converter will need to make sure that such a structure is created: <p><strong></strong></p> (see why). If we decide to prefix their names, that will bring confusion for the converters – we'll need there some additional layer deciphering that.

[scofalik]

Well, prefixing doesn't make any sense if we had to do something with such attributes as you described. So we just drop the idea.

[Reinmar]

One more thing – I should explain why do we need <p><strong></strong></p>. Why the typing feature cannot insert the characters with the right attributes (and then it would be fixed in the DOM)?

The answer is – it could, but:

1. This is more work for the engine, because the browser will insert the character directly to the <p> element and the engine needs to fix it after the character makes in the loop-back.
2. The loop-back has a great chance to be executed before the character was even rendered, however, not in all cases. In case of composition, the rendering must be postponed, so the user would see an unbolded text until the composition is committed.

[pjasiun]

I definitely agree that we should put <p><strong></strong></p> there. Wrapping the first letter into <strong> would break the composition. I think that the selection observer will remove such <strong> if the selection move, but this is something I prefer not to think about as long as we do not work on the selection.

[scofalik]

I updated files on branch t/features https://github.com/ckeditor/ckeditor5-core/tree/t/features

Look at commits from Feb 18.

[fredck]

@Reinmar

For me it's just a way to store these attributes. They represent inline styles, but are kept in a block. Kinda like in CSS where you can apply font-weight to a block or an inline element. Also note that the converter will need to create an element for such a style, even if it's applied on a block. E.g. when you have a "P" element with a "bold=true" attribute, the bold feature converter will need to make sure that such a structure is created:

(see why). If we decide to prefix their names, that will bring confusion for the converters – we'll need there some additional layer deciphering that.

@scofalik

Well, prefixing doesn't make any sense if we had to do something with such attributes as you described. So we just drop the idea.

I think the original prefix idea was just about having a way to clearly specify what attributes must be moved to the selection when the block is empty. After all, converters need to convert attributes set to the selection only, for example when simply applying "bold" on a collapsed selection.

For example: p (selection:bold=true) -> moved to selection without prefix if p is empty -> selection (bold:true) -> converter works on the selection to produce proper elements -> <p><strong>|</strong></p>.

[Reinmar]

I think the original prefix idea was just about having a way to clearly specify what attributes must be moved to the selection when the block is empty.

But for this we can use the schema. I think it is a more future safe solution and give us more flexibility.

[fredck]

:/ but then what, you need to say in the schema that "paragraph" accepts the "bold" attribute in it, even if you don't want this to happen, just to support this technical requirement? And again, we're talking about a way to store selection attributes, not nodes attributes.

[Reinmar]

I presume that if you allow bold inside paragraph this bold needs to be preserved by the selection, so it should be copied to the block. But that block allowed that style in the first place, so there's no problem.

If the style isn't allowed in some block, then the selection should not preserve it there anyway so nothing needs to be copied.

So there can be two types of attributes applied to a block – those defined as allowed on it and inside it. I agree that's not perfectly clear, but neither is adding some "selection:" attributes and the whole concept with copying attrs to the block :D.

And again, we're talking about a way to store selection attributes, not nodes attributes.

That's right and that makes me unsure what's the best solution. I think that we can change it in the future if one of the ways will work better or not. On the one hand we have those worried which you expressed. On the other we have converters which need to be executed for the attributes with "selection:" prefix, so some additional code somewhere else. Therefore I would say that it'll be hard to decide now what's the best option because we don't have working features and selection yet.

I'm quite OK with both solutions but I would implement myself unprefixed attrs.

[fredck]

Btw, theoretical conflict issue... if the "paragraph" feature would also support the "bold" attribute (for the paragraph node itself), it would be impossible to understand whether the bold attribute in p (bold=true) is the node attribute or the selection attribute.

[Reinmar]

Btw, theoretical conflict issue... if the "paragraph" feature would also support the "bold" attribute

Fortunately that's forbidden (we make the rules :D).

But we've just found a case which rather votes for prefixing attributes. Let's say you have an empty inline widget and you press CTRL+B inside it. We rather think that the logic retaining the style in empty elements should apply also to an empty inline widget. This means that the attribute would need to be applied on the <widget> element. However, the converter wouldn't now know if it's supposed to render that bold inside or outside the widget (you can apply the bold outside). Buuuuut... should the attribute ever be applied outside the widget? In some cases it needs to be (an image e.g.), but... [[[TO BE DISCUSSED]]].

[Reinmar]

It's amazing how many things we need to consider to make it right. Important points from the discussion we've just had:

1. A single attribute makes sense inside and outside an element. Like pressing CTRL+B when having an inline widget selected and having selection inside that nested widget's inline editable.
2. However, usually applying the attribute in both places at the same time will lead to odd situations, therefore:
   • features should either define in the schema where precisely the attribute is allowed (it means that we define rules for the text inside some widget and on the widget separately),
   • the tree view writer can disallow certain nesting situations (but we don't have to code it),
   • the feature may handle a specific attribute on itself differently than inside (e.g. adding bold to <image> could add a border to the image and bold its caption); those are the situations where having a single attribute in both places makes some sense.
3. We want to retain an inline styling inside empty block (or all – to be discussed later) widgets. This means that we'll need to copy selection attributes to those blocks. This means that we need to prefix them (otherwise there will be a conflict with 1&2).
4. Attribute elements must always be present inside empty blocks. They cannot be created once the selection is moved there, because that will break the x-position of the selection (moving up to an empty paragraph would reset the position, because selection would need to be moved once the atteribute element is created. In general – we should touch selection as rarely as possible.
5. The above means two things:
   • Prefixed attributes will need converters (as an addition to normal converters for these features). Fortunately it quite makes sense that they will be separated because normal converter wraps some content with an element, while a selection attribute converter creates an empty element inside that block
   • Selection attribute elements will need to filled with bogus <br>s if we want the selection to go inside them automatically. So instead of adding bogus <br> directly to the block, it needs to be put inside the innermost inline element inside that block. We checked that this works with up/down arrow keys on Chrome and FF (TODO check Edge). In other cases (left/right arrows, clicks, touch) we can freely post-fix the selection.

[Reinmar]

Similarly to #124 – done, working, will be documented soon.