Ckeditor5: Headings are not assigned to all of selected paragraphs

[maxbarnas]

Steps to reproduce:

1. Go to manual test http://localhost:1030/build/modules/amd/tests/headings/manual/headings.
2. Start selection at {}Heading 2 all the way down to Paragraph{}.
3. Select Heading 2 from drop-down.

Result:

Selecting again Heading 2 from drop down will correctly set heading to paragraphs.

[Reinmar]

Confirmed... But I wonder what's the expected result? We've got 3 different blocks in a selection and convert them to an h2, but the first is h2, so we decided to remove the style instead. Then, from all blocks?

[Reinmar]

Interestingly, CKEditor 4 does the same as CKEditor 5 currently. And TBH, I can't think of a better solution.

[maxbarnas]

What is the typical reason to select text spanned across multiple lines and chose heading from the list? Is it more natural to "mix" styles or is it more natural to set style to what user has selected? For me first action seems to be less unexpected.

[Reinmar]

Setting style would be more expectable. So do you agree with the following rules?

• if at least one of the blocks inside selection isn't the chosen one, then apply that block (heading) to all of them,
• if all blocks in the selection are of the same type as the chosen, then convert them to paragraphs.

[scofalik]

Guys, I think this is a broader subject, which applies to multiple features. The question is: "how do we find all blocks that should be renamed".

I belive, that when user selected multiple blocks, all of them should be changed to what user desired - be it a header or list item. I had to write a helper function that will check the selection and return all blocks that were "touched" by it. We should discuss that algorithm, really, and decide whether we want the same behavior in all "block-rename" features (like headings, list items, block quotes, etc.).

Right now this is algorithm:

1. Check if element after position is a block (schema is checked):
   
   
   
   • if so, add it to "selected blocks", and move position after the element,
   
   
   • if not, move position before it's current parent and go to 1.
   
   
2. Do it as long as position is before selection range end (so block after selection range end is not added).

So, if you start selection in block element, span it over 2 block elements and end in another block element, all four will be added to "selected blocks" and then renamed. I do not seek for topmost element or anything like this.

Example:
<h1>F[oo</h1><p>Lorem</p><p>Ipsum</p>]<h2>Bar</h2>

1. Position before letter, set it before <h1>.
2. <h1> is a block, add it, move position before <p>.
3. <p> is a block, add it, move before second <p>.
4. <p> is a block, add it, move before <h2>.
5. Position is equal to range end (if range ended in <h2> it would be also added).

So that changed to list items would be:

<ul>
  <li>Foo</li>
  <li>Lorem</li>
  <li>Ipsum</li>
</ul>
<h2>Bar</h2>

And for me this is what user wanted to do. After all he/she selected all those blocks.

[fredck]

All this seems ok and feasible when you think exclusively on how to APPLY the heading. Being this a user action, we can, of course, spend more time on proper algorithms that can enhance the user experience. The problem is not here, though.

The real issue is about the command STATE CHECK, which happens on every selection change event fired. Now scale this to all commands that need state checking and it is easy to understand that this operation must be extremely fast.

Commands have this "ON/OFF" way of handling state. If I'm inside bolded text, the bold command is "ON". Same things for headers... if I'm inside a "Heading 1", it's "command" will be "ON".

The above "ON/OFF" state checking calls for a toggle behavior. So, if it is "ON", on executing it should go to "OFF", and vice versa.

For the benefit of performance, we've decided that, for non-collapsed selections, we'll check the start boundary of the selection only. Therefore, if the start is inside "Heading 1", the state of this command is "ON". Now, considering the expected toggling behavior, if I execute the command again, it must go "OFF", removing the heading from the whole selection.

All the above means, if we want to make any enhancement the way we apply commands, we need to reflect this in the way we check their state. Performace wise, this may not always work, so we may take solutions which, although not optimal, make more sense for the system as a whole.

[scofalik]

Oh, @fredck, of course! Checking state should be done only at selection starting position. But that's just the matter of what elements are renamed to. After this is evaluated, then algorithm I described can do its work.

Honestly, it's even easier for Headings command, because here you have a dropdown, so you explicitly choose what you want to convert to. So, let's say that a selection starts in heading and spans over several paragraphs. If you choose paragraph, you should convert all of them to paragraphs. If you choose heading2 you should convert heading1 and all paragraphs to heading2, etc. Same is for lists but there - as you said - command value decides what are we renaming elements to.

[fredck]

In headings, you are also supposed to have the ON/OFF state. When you change the selection, the dropdown value changes to the block type of the selection boundary. When opening the dropdown then, that option is marked as active (ON), so if I click on it, it should go (OFF).

Note that you should be able to implement headings as buttons as well, where the UX doesn't change much from inline commands, like bold.

[fredck]

Btw, note that I'm not in any way discarding the possibility of checking all selected blocks on selection change. Maybe making some optimizations so all interested features can share the same check run (like we do in v4), we may verify that such check is not as expensive as we think, especially because we work now over our very own data model.

[scofalik]

In headings, you are also supposed to have the ON/OFF state.

I agree, still we have to define what "turning to OFF" means. Naturally it should be "going back to default state" which is, I assume, paragraph. So, it's just renaming to paragraph then?

[fredck]

Yes, we can assume "paragraph" as the default state of blocks... actually, weren't we supposed to have this "default" as part of the scheme?

[maxbarnas]

To summarize:

1. Problem is deeper than just headings being applied incorrectly.
2. Algorithm for checking state of selected nodes might need improvement. Proposed solution might need performance check.

[scofalik]

Problem is deeper than just headings being applied incorrectly.

Yes.

Algorithm for checking state of selected nodes might need improvement. Proposed solution might need performance check.

I am not sure whether we understood each other correctly. I think (and AFAICS so does @fredck) that current algorithm for checking state is okay and there is no need to change it. We could go with other one but I don't see reason right now. If we go with more complicated one we might need some improvements in engine.

Just to clarify - the algorithm that I described is only for finding "selected blocks" at the moment when user takes an action (i.e. presses "bullet list button").

[maxbarnas]

I meant the algorithm for finding if selection spans across different block types.

Editors like MS Word or LibreOffice Writer will show selection with different styles as "undefined":

I understand this is not the case with current algorithm. We are just checking block type on the boundaries of selection. This might be confusing to the users accustomed to the usual behaviour in other editors (I was). I would consider current solution a simplification that needs improvement.

[oleq]

I believe that during the review of https://github.com/ckeditor/ckeditor5-heading/issues/53 we decided the v4 behavior is correct, which is the first execution of the command for such selection should remove the heading.