Use triggerBy conversion for table

[jodator]

📝 Provide a description of the improvement

Follow up of the #8099 & #7956 .

During the work on #7956 I had troubles with using differ.refreshItem() or triggerBy converter option to simplify table re-converions on big structural changes.

In order to implement this I think that we need three-way-binding (or layered binding) in conversion. More info can be found here: #7956 (comment).

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[jodator]

Problem statement

What should be reconverted by the main downcast converter?

Model                                  -> View

<table headingRows=0>                  -> <figure>
                                              <!- other stuff like UI ->
                                              <table>
                                                  <thead>
    <tableRow>                         ->             <tr>
        <tableCell>                    ->                <td>
            <paragraph>Foo</paragraph> ->                    <p>Foo</p>
        </tableCell>                                     </td>
    </tableRow>                                       </tr>
                                                  </thead>
                                                  <tbody>
    <tableRow>                         ->             <tr>
        <tableCell>                    ->                <td>
            <paragraph>Bar</paragraph> ->                    <p>Bar</p>
        </tableCell>                                     </td>
    </tableRow>                                       </tr>
                                                  </tbody>
                                              </table>
</table>                                  </figure>

The table model -> view mapping is troublesome because:

1. Model table maps to a complex figure > table > thead & tbody view structure.
2. Table can have one or two "slots" for inserting children (tbody & thead). Those depend on the table's attribute (headingRows).
3. A table row can be moved between one of the "slots" during editing.
4. If a table row is moved between "slots" its child cells might need to be renamed ("refreshed" <th>/<td>).

Options: What should be converted by the triggerBy reconversion?

Below are options where a "slot" can be defined in tables. To help visualize for what will be the main table converter responsible each case will be presented as "JSX-like" template. Some loops were omitted to not blurry them.

Notation <p slotFor="paragraph"> renders the <p> & binds <p> as a slot for <paragraph> so all children of <paragraph> are converted inside <p>.

1. Down to table sections thead and tbody.

<figure>
    <table>
        <thead></thead>
        <tbody></tbody>
    </table>
</figure>

2. Down to each table row tr

<figure>
    <table>
        <thead>
            <tr></tr>
        </thead>
        <tbody>
            <tr></tr>
        </tbody>
    </table>
</figure>

3. Down to table cell td

<figure>
    <table>
        <thead>
            <tr>
                <th></th>
                <th></th>
            </tr>
        </thead>
        <tbody>
            <tr>
                <th></th>
                <td></td>
            </tr>
        </tbody>
    </table>
</figure>

Pros and cons

Table sections

• Good, because reconverting the table would require only the main table wireframe to be handled.
• Good, because every existing <tr> will be memoized and will not be converted again.
• Bad, because it would require providing API for "splitting" model into two "slots". (Problem 1 & 2)
• Bad, because <td> will have to be "refreshed" by a post-fixer. (Problem 4)
• Bad, because requires bigger effort to implment in reconversion API. (Practically a blocker)

Table row

• Good, because reconverting the table would require a smaller set of changes to be reconverted (only table rows).
• Good, because it is easy to handle mappings in reconversion.
• Good, because every existing <td> will be memoized and could be potentially not be converted again.
• Good, because <tr> can be memoized by the view writer thus re-used.
• Good, because its performance is on par with currrent reconversion. (See: Use triggerBy conversion for table #8138 (comment))
• Bad, because <td> will have to be "refreshed" either by post-fixer or by main table converter. (Problem 4)

Table cell

• Good, because each <tableCell> will have corresponding view cell regenerated if needed (Problem 4)
• Good, because children of existing <td> will be memoized and will not be converted again.
• Good, because it is relatively easy to handle mappings in reconversion (might be slightly harden than rows).
• Bad, because table converter will have to converter all the way down to <tableCell> (is doing it ATM).
• Bad, because reconversion API is not suited for deep reconversion (only direct children are taken by triggerBy configuration).
• Bad, because it is tricky to handle attribute changes.
• Bad, because its performance is worse (See: Use triggerBy conversion for table #8138 (comment))

[jodator]

Option 1 is basically too hard to pursue IMO. I'm torn between 2 & 3.

Option 2: Can have a probably simpler <table> converter and atomic tableRow and tableCell converters. (in the long term).

Option 3: Handles td/th problem and could lead to a single table converter in the future (assuming that we handle DOM rendering problems.

[jodator]

The main part will be handled in #1589 as we need to enhance API for it as well.

[jodator]

I've started to digging up in here. I've noticed one problem with the implementation from the #8099 PR.

The consumed items handled by reconvert (it would also do insert/attribute conversion) are not passed to other handlers. This is because we clear conversion API after every this._handleXXX() - like this.convertInsert().

I tried to filter out changes if item was inside already reconverted but this was a no-go. We can't predict which items will be reconverted and which should fire event.

[jodator]

Possible blocker: Partial support for <table> reconversion is tricky because it only reacts to table attributes and not child rows.

In table downcast there's a custom remove row downcast converter. It does cause problems with reconversion as it removes the wrong row. There's no need to remove it as table reconversion would create a new structure.

Maybe its implementation is not needed as its tasks (ensuring any empty <tbody> or <thead> are removed) might be done in view post-fixing.

[jodator]

At this moment it looks that implementing "Differ#getChanges() could return items for remove entries" should do the trick.

The problem here is that the default remove downcast converter maps view element from model position. If the view element was already removed by reconversion converter then handling it once again by the default converter would remove existing view.

[jodator]

Since the above problems with reconversion trigger only on heading rows is problematic I've tested the following setup:

conversion.for( 'editingDowncast' ).elementToElement( {
	model: 'table',
	view: downcastInsertTable( { asWidget: true } ),
	triggerBy: {
		attributes: [ 'headingRows' ],
		children: [ 'tableRow' ]
	}
} );

So reconverting always on added / removed row. Findings:

1. The code got simpler (see how much code I've removed in f6032d5).
2. Unfortunatelly this conversion does not handle table cell renaming (if a row is moved to a different section).
3. Performance at the first sight it looks a bit better.

Ad 3. I've tested it using 43 rows x 15 columns table (sorry for odd numbers):

	conversion	render
f6032d5
master

[jodator]

And the case for reconverting a whole table on any child change (<tableRow> or <tableCell>) using configuration:

conversion.for( 'editingDowncast' ).elementToElement( {
	model: 'table',
	view: downcastInsertTable( { asWidget: true } ),
	triggerBy: {
		attributes: [ 'headingRows' ],
		children: [ 'tableRow', 'tableCell' ]
	}
} );

The code got a bit tangled again, but almost all atomic converters were removed, but:

1. The Dispatcher internals needs to be rewritten - there's a problem with converting attributes on table cells. Right now they are converted twice (no proper attribute filtering). It might be hard to define which events should be triggered after the reconversion. Additionally, I have to support table cell properties converters as well.
2. The performance dropped a bit here as we render every table cell - in the above case those were momoized.

Performance (compare with the above comment)

	conversion	render
d3efbf0

[jodator]

After F2F talk with @Reinmar we concluded that going with option "Reconvert table with table rows" option is the way to go for now.

It is OK to rename table cell's in the view - check writer.rename for this.

[niegowski]

DUP #10502