Key support. Implements #8232

[SteveSandersonMS]

This is the runtime part of allowing syntax like <something key="@somekey" /> to guarantee affinity of elements/components with model objects.

Common use case

@foreach (var person in People)
{
    <div key="@person">... some complex DOM structure goes here ...</div>
}

... or:

@foreach (var person in People)
{
    <PersonDisplay key="@person" PublicInfo="@person.PublicInfo" />
}

The use of key changes how the diff algorithm handles changes to the People list:

• Without key, it matches model objects with elements/components in the order they both appear. So if, say, People[0] was removed, then it would reassociate each element/component with the n+1-th item in the list, and finally delete the last element/component. This can cause unnecessary extra re-rendering of child components or even problems such as making the focus appear to move to an unexpected textbox (if the focus was in a textbox for Person[2], that textbox would now be reassociated with what was previously Person[3], which looks as if the focus moved (although really, it's the rest of the world that moved and the focus stayed still)).
• With key, it matches model objects with elements/components based on equality of the supplied key object. So if, say, People[0] was removed, then it would know simply to delete the first element/component, and leave everything else alone.

Similarly, if the entries in People were shuffled, then:

• Without key, it would reassociate the i-th element/component with the new entry People[i], and re-render. This performs a fine-grained update on all descendants (e.g., updating all the text nodes individually).
• With key, it would simply re-order the elements/components to match the new ordering. It would not necessarily re-render descendants (though in practice, in the components case, it does ask them if they want to re-render because it doesn't know whether the People entries themselves have mutated).

Using key remains optional, but it's nearly always a good idea to use it any time you render a list whose data is subject to insertions/deletions/reordering.

The other possible reason to use key is if you want to guarantee non-preservation of a given subtree when some data changes. Example:

    <div key="@myModel.Id">... some complex DOM structure ...</div>

Without key, the renderer will respond to changes by recursing into the descendants and updating each affected descendant individually. With key, if myModel.Id changes, it will throw out the entire subtree and insert a new one - this guarantees that things like focus state are not retained.

Code review notes

While implementing this, I hit what I felt were complexity limits in a couple of areas, forcing me to refactor before making substantial additions. You may find it easier to read the individual commits in order. In particular, I refactored:

• RenderTreeFrame, because the old system of having many ad-hoc constructors corresponding to different scenarios became intractible. There would soon be clashes of constructor signatures, and it was hard to be sure that all the With... methods (that clone-with-mutatation the immutable struct) would correctly preserve all the fields that were not being modified. Basically, this is crying out for C# 8.x records. I changed it to have one constructor per frame type, where that constructor forces you to specify all the fields applicable to that frame type. It's therefore unambiguous where to add any new fields and ensures you can't forget to copy other ones.
• RenderTreeBuilder, mainly to reorganize the AppendDiffEntriesForRange logic. Previously it mixed "deciding what to do" with "doing that thing". Those two concepts are now separated, so it's easier to have multiple distinct code paths that result in the same outcome (e.g., "treat as insert" or "match and recurse into descendants"). The code diff for this refactoring is hard to read, unfortunately, because it looks as it basically everything changed even though mostly it was moving blocks of code around.
  • I also tried breaking up AppendDiffEntriesForRange into multiple methods, because it's huge. However, benchmarking showed about a 10% slowdown on Mono WebAssembly after doing that, which I would think is because it has to execute a lot more instructions to do all the parameter passing. So I have left it as one big method, but tried to clarify the different areas of responsibility.

The logic around detecting keyed inserts/deletions is really simple and doesn't need much of an explanation. Handling moves, however, is much more complicated. My goal was to find a way of distributing the workload between the .NET side and the JS side such that it was no worse than O(N) on either side, and that the .NET side didn't involve any per-render allocations in typical cases. The solution was not generate a list of "moves" (I can't think of a way to do that without either allocating for each key or doing O(N^2) lookups, but if anyone has a solution, I'd be interested!). Instead it generates a description of the final state as a permutation of the normal post-edit state. On the JS side, we can apply the permutation in O(N) time with O(N) allocations (inserting marker nodes into the DOM to track where things will get inserted) - this small number of allocations on the JS side doesn't concern me at all.

[javiercn]

I've taken an initial look through the source code. Everything looks good, although TBH I'm not an expert yet on the diffing code, so I will have to take a deeper look probably tomorrow. I plan to clone the branch and play a bit with it, but overall looks great!

If I understood correctly there are two cases for using key:

• Components: The entire component gets rerendered (this happens today without key, so no issue here?)
• Elements: Instead of moving the world around the first item, things are moved around. Do subtrees get diffed between matching key elements or do we always render the new entire subtree?

Regarding the algorithm. Are we using a specific diffing algorithm from literature or did we just "craft" our own. I imagine you've done this, but we could potentially do some research and see if we can find anything already invented that might perform better in terms of time/space.

I've taken a quick look at the tests and I couldn't find any E2E test. Shouldn't we have some?

[SteveSandersonMS]

If I understood correctly there are two cases for using key:

The common use cases are described as best as I can in the PR description. It's not just about the first item in a list; it's about general insertions/deletions/reorderings and preserving the association between model objects and elements/components.

Do subtrees get diffed between matching key elements or do we always render the new entire subtree?

They do get diffed between matching keys. We only create an entirely new subtree when we have an entirely new key.

Regarding the algorithm. Are we using a specific diffing algorithm from literature or did we just "craft" our own.

At a high level, our algorithm for elements/components/regions is essentially "merge join". This is much simpler than a general-purpose diff, as well as faster, and is possible only because we have the compile step that lets us inject sequence numbers. We only need a single linear pass, which is much cheaper than most general-purpose diffs that don't require sequence numbers. When we detect reordered attributes or keys, our algorithm is essentially "hash join", which is again O(N) time.

I've taken a quick look at the tests and I couldn't find any E2E test. Shouldn't we have some?

Still to do. I'll be adding them tomorrow.

[javiercn]

This is noted in the PR description as "still to do".

Fair enough, didn't see it.

[rynowak]

I looked over the first few commits, and got lost when we got to the diffing algo changes. This is an area where I haven't really gone deep in the past, and I think it would be more valuable for someone besides me to become a second expect on this (👀 in the direction of Team Blazor EU).

I don't want to create noise by adding low value comments :)

[SteveSandersonMS]

OK, hopefully this is now ready to go.

[SteveSandersonMS]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[SteveSandersonMS]

Ping to reviewers on this.

@javiercn If it would be helpful to have a call about this so I can walk you through the algorithm, please let me know!

[SteveSandersonMS]

@javiercn

Should we have some E2E test that involves inputs to set expectations?
For example, when we move an input the focus doesn't get lost.
We could probably check that the cursor/selection stays in place.
Could we have a test for inputs moving elsewhere in the tree? (like the checkboxes issue that was reported the other day).

I'm very glad you suggested this. Adding these E2E tests uncovered two problems:

• We were not preserving focus state when moving elements. I just totally forgot this was needed. I should have known because we do the exact same thing in knockout.js when reordering elements :) Fixed.
• There's an utterly bizarre behavior in Chrome that I can only guess is a browser bug. It doesn't happen in Firefox. I found that when moving nodes, the act of calling node.insertBefore from inside an input event can trigger a change event at that point in the call stack. So this leads to nested event handlers, which is not something we support. In turn it causes nested rendering, which leads to corruption and a renderer exception. Firefox behaves better by not firing the change event until after the input handler has returned.
  • This only affects client-side Blazor, because that's the only case where events are truly synchronous. On server-side Blazor, the asyncness of running the event handler across the network means the handlers aren't nested on the call stack. Plus the server-side sync context would run the event handlers in series anyway.
  • To work around this, and to protect us against other more general cases of nested event handling (e.g., maybe DOM mutation observers could cause this), I've made some changes in client-side Blazor's RendererRegistryEventDispatcher. It now explicitly checks for nested event dispatch, and if that happens, it queues items so they run in series (though they still start synchronously). This makes things consistent both across server/client hosting and across browsers.

There are now E2E test cases for all things you suggested (typing while boxes move around, and checking checkboxes in lists that move as you check them).

If you want to review the last few new commits please go ahead! I'm still planning to merge this once the CI checks complete because this PR has been waiting for so long and I think the final changes (which are mostly E2E tests) won't really need much further review.

[SteveSandersonMS]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[javiercn]

I'm very glad you suggested this. Adding these E2E tests uncovered two problems

Yay! I added value :)

Taking a look at the new commits.

[javiercn]

New changes look good too!

[SteveSandersonMS]

Windows, Mac, and Linux builds all failed for entirely different reasons, none of which appear related to this PR :(

[SteveSandersonMS]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[SteveSandersonMS]

/AzurePipelines run AspNetCore-ci (Test: Templates - Windows Server 2016 x64)

[azure-pipelines]

No pipelines are associated with this pull request.