Fix various issues with todo list converters

[jodator]

Suggested merge commit message (convention)

Fix: Use model-to-view position mapping in todo list. Closes ckeditor/ckeditor5#2009. Closed ckeditor/ckeditor5#1980.

The todo list converters always assumed that checkbox ui element is the first child
of a list item in the view. This failed with attribute element converters which wrapped
whole list item contents, including the checkbox, with their attribute element
(e.g. <strong>). This changed the first child of the list item and caused problem
with typing or changing list type in the editing area. This change makes sure that
position at the begining of a list item from model is mapped after the checkbox
ui element in the view.

---

Additional information

• This PR fixes the issues with a todo list described in orignal issue Link is removed from a to-do list after transforming it to a list ckeditor5#2009 and others found by me during the work on this PR which revolved around the issues described above.

---

Notes from draft PR

• This is a draft PR - fixing some issues with todo lists that involve typing and list modifications when the list item contains a text node with attributes.
• after some fiddling I gather that the easiest fix would be to fix the model to view position mapper, unfortunately, it breaks the test for the markers.

So I have some questions about this: todo list + markers - why the previous solution didn't just map the positions? Is it because of the markers? After my change, you cannot create a highlight that covers the whole <li> - it will be set after the checkbox.

Fixing position mapping in helps with:

• typing at the first position when the text has one or more attributes
• the same situation but instead of typing you press enter - then some view element leftovers appear and the arrow movement was broken
• original issue when changing list type and one todo item has a text attribute

The problem for the above wast that the converters assumed that the first child was always a label for the checkbox but with the text attributes (bold, link) the associated view element was wrapping the li contents and the ui element (label). Fixing the position mapping so the view position is always set after the ui element keeps the ui element as the first child and other converters also properly render their view (ie only wraps the text of the li excluding the UI element.

/cc @oskarwrobel - with a question about the implementation - maybe I'm missing something
/cc @scofalik How this should work with the markers? Or this should the markes be rendered differently on a todo item?

[coveralls]

Coverage remained the same at 100.0% when pulling 9a881d2 on t/ckeditor5/2009 into 63deb51 on master.

[oskarwrobel]

So I have some questions about this: todo list + markers - why the previous solution didn't just map the positions? Is it because of the markers? After my change, you cannot create a highlight that covers the whole

- it will be set after the checkbox.

I haven't come up with an idea to use mapper. That's the only reason :P Markers should be rendered after the checkbox element and it's better when attributes don't wrap the checkbox so you change is 👌

I removed "some" code that has become unnecessary after using mapper dce5c7b :D

[oskarwrobel]

You should ensure that nextSibling is not an UIElement. Selection converter will try to break it (that's why one unit test is red).

UIElement is used to represent a selection of collaborative users, so when two users put selection at the beginning of the totoList item it's gonna 💥 💥 💥

[jodator]

Thanks for the clarification :) I'm still not used to the markers :/

[jodator]

Again, thanks for the help @oskarwrobel now the todo list should work even better :d .

@Mgsy could you verify that this works now? The main disruptor here was any text attribute (bold, link, highlight, etc) at the beginning of the list item. It caused the problems from ckeditor/ckeditor5#2009:

• changing list type removed the text wrapped in attribute
• typing at the beginning of a todo list with text attribute

ps.: it fixes also the ckeditor/ckeditor5#1980 :D

[oskarwrobel]

I'm afraid checking only the next element won't be enough. There could be more UIElements at the beginning of the list item. You should walk over them and find a node after the last one.

[jodator]

Check the test - it works. We return a position after our UIElement so others are properly (in order of insertion) inserted to the model. For me it is OK.

[oskarwrobel]

Yep, you are right. It works fine.

[Mgsy]

LGTM.

[oliverguenther]

Since I didn't see an explicit test for it and it is breaking our conversion to Markdown todo-lists, is the following output from the nightly todo list as expected? This means that the label does not always wrap all content of the todo list, resulting in two different parsing behaviors for the todo list output in our case.

<ul class="todo-list">
  <li>
    <label class="todo-list__label">
      <input type="checkbox" disabled="disabled" checked="checked">
      <span  class="todo-list__label__description">4 teaspoons baking powder&nbsp;</span>
    </label>
  </li>
  <li>
    <a href="https://example.com">
      <label class="todo-list__label"><input type="checkbox" disabled="disabled">
        <span class="todo-list__label__description">1/4 teaspoon salt&nbsp;</span>
      </label>
    </a>
  </li>
</ul>

[Reinmar]

Hm... it seems to be a bug. The label should always be the outermost element. Could you report a separate ticket for it?