A triggerBy option that issues element re-render

[jodator]

Provide a description of the task

Implement the base API for the slot conversion API. This should provide a minimal, top-level API for changes defined there and researched in: #7729.

Goals:

• top-level API as described, anything below should support it.
• Use memoization technique to reduce re-rendering big chunks of the same views.
• The renderer is out of scope of this task.

[jodator]

To have this properly handled from top to bottom I'll start with:

• clearing up the code so the test will pass
• write test cases for the feature - to define proper bahavior
• update/refactor the code

[jodator]

I think that I have a solid base of test for the feature - as for now I don't think that any corner cases should be handled.

And while writing it down I see that I lack tests for not re-converting element for changes below slot, ie do not re-convert for changes in a <paragraph> inside a slot.

The other thing is that current API lacks some sort of the element binding (the case for outer/inner view elements maps to one model element - like all elements wrapped in <figure>). For now I'm thinking about extending mapper.bind to allow such mapping and, for now, will do exactly the same as what we do with double binding now.

[jodator]

I tried to make this work with table conversion. The base scenario works well and the code could be simplified a bit and more fluent in other parts. However, re-rendering on multiple changes (mixed with triggerBy) fails currently.

The current table refreshing logic is triggered for any headingRows attributes change, so in the new API it could be refactored to:

conversion.for( 'editingDowncast' ).elementToElement( {
    model: 'table',
    view: downcastInsertTable( { asWidget: true } ),
    triggerBy: [
        'attribute:headingRows:table'                    // <-- this should handle (1)
    ]
} );

And (1) is the whole:

ckeditor5/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js

Line 25 in 1cab865

function tableHeadingRowsRefreshPostFixer( model ) {

(the whole file is about 50 lines long). Complete set of changes is here: 2b9bd51.

This works nice in tests but also showed me that I didn't add more complex test cases. The problem in table scenarios is when:

1. refresh must be triggered ie by attribute change
2. and structure has changed also (added/removed child) and is not defined in triggerBy (might not be relevant).

Why? I didn't know differ internals - there's a huuuuge if-else tree (I'd refactor this to be purely funcitonal maybe some day) that handles overlapping changes.

For instance, if there's an insert change that overlaps an existing remove change, the remove change is deleted from changes set (but not going deep into what actually happens there). Now, for proposed refresh change we need to:

1. have a similar way of deleting changes that will be handled by the converter anyway
2. while allowing parts of the tree below the changed element (slots content).

[jodator]

For starters, I'll try to write tests for similar cases in the engine and see if those shed some light on the issue or solutions. We might invalidate this case for now and revert table changes.

The other thing is to implement triggerBy callbacks together with a complete set of events and a way to manipulate them.

Alternatively, (to not make the solution too complex to use from outside) information about "slot" should be available somewhere (it is in the mapper for instance) so I'll check that first. The goal is to not bloat the API from the inside and to figure things out internally.

[jodator]

For starters, I'll try to write tests for similar cases in the engine and see if those shed some light on the issue or solutions. We might invalidate this case for now and revert table changes.

Oh, I have a similar failing test for this already.

[jodator]

I was trying to handle complex cases of refresh events (9fa12db), a scenario that was kind of hard to figure out was:

$root
    complex        <- refresh, ie by attribute change
        slot    
        slot    <- remove

with configuration like:

conversion.for( 'downcast' ).elementToElement( {
    model: 'complex',
    view: () => { ... },
    triggerBy: [
        'attribute:foo:complex',
        'insert:slot',
        'remove:slot'
    ]
} );

What should happen is that remove operation is not handled by the dispatcher, or in other words a set of changes from a differ should only include refresh change for the <complex> only.

Handling this on a Differ level turned out to be pointless as Differ can't be aware of when given child change should be hindered by the refresh change and when not (for instance in the above example change below <slot> (for instance <paragraph> inserted in <slot>) should not be hidden by refresh of the complex element and should be converted.

However, the downcast dispatcher has this information as it handles triggerBy options.

My next steps will be to skip conversion of matched events either by:

1. marking some changes as outdated in the differ (so the differ would return only refresh)
2. skipping differ results for matched events

As for now the 1) seems a bit to much attached to differ while 2) looks like a proper place to handle above.

[jodator]

e3b8c4e: I've manage to dig out some issue that still need to be resolved, most likely covering the above post.

• consuming in the general insert converter on "refresh" was too greedy
• proper events handling to check what needs to converter is needed in refresh handling
  • do not convert items outside refreshed element (figure it out on events)
  • convert new items outside refreshed element (as above)

[jodator]

db8c17a: The full reconversion with slots is kinda working. The case to handle is a hybrid approach in which main element is handled by reconversion while adding/removing slots is done separately. If time allow I'll investigate this, however I'm worried that it would require API for a layered view items mapping (image -> figure>img case).

[jodator]

So for so good. I've "fixed" the API for now, added some docs and removed debug code. Still to handle:

• table conversion - tests are failing after triggerBy was use in table converter (might be blocked by Conversion from simple model to complex view (slot conversion) #1589)
• engine code coverage dropped - inspect whether test are needed or there is dead code
• marker refreshing
• internal API - code is a mess still in prototyped code

[jodator]

After some battling with table tests, it turned out that I've implemented the POC on the downcast helpers level. Which is not ideal. Table tests showed that removing old view and view momoization should be handled on the dispatcher level. The current form creates duplicated views when refreshing paragraphs in table cell.

[jodator]

Most of the issues with table conversion were related to the refreshing table cell. I've refactored that part to refresh <paragraph> in those cells only. This brought some changes to conversion (mostly due to special handling of the single paragraph in editing and data views) but the whole re-converting algorithm works 👌 here:

• you can refresh the table only on specific events.
• you can refresh the paragraph in a refreshed table cell (I hope we had a test for that :D ).
• you can have attribute converters on a refreshed element.

Moving the logic from downcast helpers to the dispatcher was easier than I anticipated.

[jodator]

The #1589 is a blocker for a group of tests in the engine. So without it, you could only write a whole-refresh item converter that would include creating slots.

Tables have custom row insertion logic that does not rely on position mapping.

[jodator]

ATM the engine tests are OK with one exception for failing table tests.

In tables I have 3 failing tests and 5 ignored.

Failing tests requires some investigation of the downcast conversion ,- minor issue.

However ignored tests are worse as those cause browser to freeze as differ produces infinite loop condition in getChanges(). There's unshift used in the loop processing an array and in those cases, it unshifts, the same change indifinietly.

All of those changes might be tied together. I'd suspect some glitch in table conversion (maybe refresh() is used too often) or the differ still needs and improvement in order to properly handle this edge case.

I'll try to investigate what set of changes produces the infinite loop.

[jodator]

Re-converting whole thing + slot conversion

The table feature showed me the thoughtshortcomings of what I though was needed.

Cases:

1. An element without children (w, w/o external attributeToAttribute() conversion)

Always replaced by a new view structure. Relatively simple.

<apple type="macintosh"></apple>

// converted to:

<div class="apple"><span class="apple-type">macintosh</span></div>

Current use case: none.

1. Flat element with children (w, w/o external attributeToAttribute() conversion).

<tree hasBugs="true">                           // Element start, children are here
    <paragraph>A</paragraph>
    <paragraph alignment="right">B</paragraph>
</tree>                                         // Element end

// converted to:

<div class="tree tree_with-bugs">               // Element start, content goes here
    <p>A</p>
    <p style="text-align: right">A</p>
</div>                                          // Element start

Current use case: <paragraph> inside tableCell requires specific rendering (either <p> or <span> in the editing view).

1. Element with children, complex view (no slots)

<tower levels="2" type="pretty">                  // Element start, children are here
    <paragraph>A</paragraph>
    <paragraph alignment="right">B</paragraph>
</tower>                                         // Element end

// converted to:

<div class="tower tower-pretty">               // Element start
    <div class="tower-level-1">
        <div class="tower-level-2">            // Content goes here
            <p>A</p>
            <p style="text-align: right">A</p>
        </div>
    </div>
</div>                                          // Element start

Current use case: The <image> to some extent.

1. Element with virtual "slots"

Here slots not wrapped in a particular wrapper element or other elements might be rendered alongside them.

<train type="steam">                        // Element start, slots are here
    <car>
        <paragraph>A</paragraph>
    </car>
    <car>
        <paragraph alignment="right">B</paragraph>
    </car>
</train>           table cell                         // Element end

// converted to:

<div class="train train-steam">             // Element start
    <div class="tender"><span class="coal"></span></div>
    <div class="car">
        <p>A</p>
    </div>
    <div class="car">
        <p style="text-align: right">A</p>
    </div>
</div>                                      // Element end

Demo with <tile> and <body> sections. The <table> to some extent. Also <table> and <mediaEmbed> if you wish to add <caption> to them as an external converter.

The problem here is that each slot must be fully controlled by the main converter. You can not add an attributeToAttribue() converter to the slot in this case (at least easily).

The tables turned to be a tricky - slots at the <tableCell> level are extremely hard as other, granular table features need to convert its attributes. Thus <slot> in terms of tables can be:

• on <tableRow> level - feasible in 2-4 days. table converter insert each row (engine tests are already covering this case)
• on table -> thead,tbody level - I'm not sure if feasible in a short period of time. Specific use-case in which one model element has two+ view elements. Probably would require model-to-view position mapping. Interesting idea but requires low-level approach and I don't see an easy API for that. The slot conversion of table+tableRow as in the previous point is good as we will not have to translate some positions.
• on <tableCell> level - almost working in the POC, some troubles were caused by the differ (infinite loops) and lack of rendering attributes from other features (table properties). However, with some additional work, it is doable.

[jodator]

Case 1: The less problematic case IMO - self contained element without children - tested, works.

Case 2: I implemented in #8099 as differ.refreshItem( paragraph ) in table cell refresh post-fixer (previously you would refresh the whole table cell - I don't see that necessary).

Case 3: Requires #1589 - an API to tell the mapper where elments start/ends in the view tree. Partially handled by double-bindings in some places but it is buggy.

Case 4: I'd postpone this case for own ticket. Havin #1589 should easy implementing this. Additionally attributes conversion for rendered <slot> should be also possible (table properties scenario).

[jodator]

To conclude this ticekt with a minimal use case I'd go with enabling the most obvious solutions for reconversion - mostly as a demo/feature guides. Other things should be fixed by the follow-ups to unblock refreshing a table with minimal view changes and allowing to implement <caption> insertion to <mediaEmbed> like describe in #2780.

[jodator]

So the remaining thing is to fix memoization used together with triggerBy (and future element reconverting API - a.k.a. new Differ.refreshItem(). 

It turned out that I've implemented view memoization only when the "slots" are defined and missed the simplest case where a block element is refreshed and its children should be retained.

The differ.rereshItem() changes were reverted as it turned out that the whole reconversion can be (should be?) handled by the downcast dispatcher (starting from this PR comment).

Changes to the table-cell-refresh-post-fixer turned out te be OK - despite reverting to the old differ logic. Now only <p> is re-inserted rather than whole <td>.

ATM I'm going to fix the memoization logic to make those tests green ( 7e359d8 ).

[jodator]

So the memoization works correctly in the downcast dispatcher and the logic should be simpler as the last thing to be removed was slot binding from the mapper.

The code changes are now restricted to DowncastDispatcher and DowncastHelpers.