Switched order of attribute initialization and DOM addition.

[jspuij]

• Switched order of attribute initialization and DOM addition.

Fixes #6218.

Part of https://github.com/dotnet/aspnetcore/wiki/Blazor-July-2020-Sprint
Tagging @SteveSandersonMS for review.

[SteveSandersonMS]

I think it might not be enough to do this only here. Note that on line 251, it recurses into the entire rest of the DOM building process. So the change as-is here would invert the ordering of building the entire DOM, which might have large numbers of unintended consequences. It might work fine, or it might cause big problems depending on whether (for example) custom elements care about being attached to the document at the time they are inserted into their parent.

To avoid this problem, I recommend calling insertLogicalChild in two places instead of just one:

• Immediately before the call to this.insertFrameRange on line 251. This will apply to elements with descendants.
• Immediately after the for loop completes on line 254. This will apply to elements with no descendants.

Of course, we only want to make one of these two calls on any given pass, so you'll also need a flag to track whether it was called on line 251 so you can then skip it on line 254.

[SteveSandersonMS]

Also it's worth clarifying: when a custom element is attached in regular HTML documents, are its descendants all populated by that time, or is it just its attributes?

If you expect to see all the descendants already in the document, the logic you have already would be correct. Whereas if you expect to see all the ancestors already in the document, then the logic you have here is not correct and should be updated as in my comment above.

[jspuij]

You're right, I missed the recursive part. I'd say the user will expect ancestors to be ready.

[jspuij]

On second thought: Every DOM insert is a potential cause of reflow. Leaf to trunk insertion might be benificial and way cheaper than the current order.

[SteveSandersonMS]

I don't think it will reflow during the synchronous event loop, but leaf-to-trunk might be beneficial if it looks more like normal HTML from the point of view of the custom element.

It depends on when the browser calls connectedCallback. Does it do it [A] when the element is inserted into its parent, or [B] when its ancestor is attached to the real live document?

If it's [A], I think we need trunk-to-leaf order so that we don't trigger these notifications before inserting into the real document.

If it's [B], then I think leaf-to-trunk is better because then the custom element gets to see all its ancestors and descendants at once during connectedCallback.

[SteveSandersonMS]

Good question :) It's because there are different insertion orders to account for. Hopefully this will clarify: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Web.JS/src/Rendering/BrowserRenderer.ts#L257-L261

[jspuij]

Good question :) It's because there are different insertion orders to account for. Hopefully this will clarify: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Web.JS/src/Rendering/BrowserRenderer.ts#L257-L261

I got that ;-)

But the question is: Do we want to support HTMLSelectElement.value from Blazor? It's a string that represents the value that is going to be sent on form submission. normally a "preselected" value (or multiple) would be indicated by a selected attribute on the option. The fact that you can get / set it from javascript does not mean that it needs to be supported at DOM insertion. It's not official behaviour anyway.

This would be my way to handle selecting an option value (possibly delayed):

<form>
<select>
    @foreach (var item in items)
    {
        <option value="@item.Item1" selected="@item.Item2">@item.Item1</option>
    }
</select></form>

@code
{
    List<(string, bool)> items = new List<(string, bool)>()
    {
        ("Test", false),
        ("Test2", false),
    };

    protected override async Task OnInitializedAsync()
    {
        await Task.Delay(3000);
        items.Add(("Test3", true));
        await this.InvokeAsync(() => this.StateHasChanged());
    }
}

[SteveSandersonMS]

We need to support all existing syntaxes, including binding:

<select @bind="myValue">
    <option>First</option>
    <option>Second</option>
    <option>Third</option>
</select>

[SteveSandersonMS]

What I suspect we'd need to do, in order to switch to the leaf-to-trunk order, is to go through all of the logic involved in building the DOM (unfortunately!) to find exactly which parts of it might rely on the previous trunk-to-leaf ordering. It might be that running the E2E tests highlights most of those cases, but it would be worth attempting some manual inspection too.

For any such case, we would need to come up with a way of flowing in whatever information is needed to make it work. For example, when applying values to a <select>, we'd need the <option> elements we're inserting to be able to trace back up the DOM to find their closest <select>, even if they haven't yet been inserted into the document. It might be possible to do this by changing something like LogicalElement to have a concept of "what my parent node is or will be once I'm inserted into the physical DOM" so that the logic for tracing up the hierarchy can still work even when the chain of parents isn't yet physically established.

Overall I recognize this is pretty complex stuff. If you think it's impractical to attempt I completely understand. However if you are keen to have a go I'm happy to try to help you along!

[jspuij]

I went at it while preserving the original order. Reversing the order is for another time.

[SteveSandersonMS]

Thanks @jspuij, this looks like a viable thing we can do!

Would you also be able to add a suitable change to the E2E tests showing that this now takes effect? As an example, we already have an E2E test example here (with the actual test logic here) that makes use of custom elements. If you made a similar test that relies on attributes being available at the right time, we'd have the coverage we need. Thanks so much for contributing!

[jspuij]

So @SteveSandersonMS I went with the current order for now. This was the least invasive method. It adds to the DOM directly after setting attributes, but before recursing into children and before the setting of the select value.

Reversing the order does not bring any advantages if the render is synchronous and the reflow is after the batch is completely rendered.

Setting the value later is justified, because it's not a real DOM attribute (value does not exist as attribute on select) but a javascript property. It's also not applicable in the case of custom HTML elements.

There is an E2E test and it runs in the TestServer project.

[SteveSandersonMS]

I’m not sure how this will work when dynamically adding an “option”. Won’t it try to apply the “value” before inserting into the document, and thus be unable to update its ancestor “select” element?

Hopefully the E2E tests will catch this but if not could you briefly explain how it works?

[jspuij]

The order now is:

1. Create element
2. Set attributes
3. Insert element into DOM
4. Recursively add child frames (potentially calling insertElement recursively)
5. call setSelectElementValue

So rendering order is trunk-to-leaf, as it was originally. The value for the select is applied after it's children are rendered and it's inserted in the DOM. Nothing changes there. Sending the option later, still works.

The only thing that changes is that 2 and 3 are switched. That does not matter for the select, as it does not have a real value attribute anyway, so it's stored between 2 and 5 in an intermediate custom _blazor attribute.

[jspuij]

I’m not sure how this will work when dynamically adding an “option”. Won’t it try to apply the “value” before inserting into the document, and thus be unable to update its ancestor “select” element?

It should not because child frames are still inserted / processed after the parent element is added to the DOM. only attributes are set before the element is in the DOM.

[jspuij]

The CanBind Select test already fails on master BTW, so I cannot use it to prove that there is no regression.

[pranavkm]

@SteveSandersonMS would you mind looking at this when you get a chance? You probably have the most context.

[jspuij]

@SteveSandersonMS You were right, there is a regression because of the order that was switched. Preparing a fix now.

[jspuij]

Picture proof: The select boxes said B and A upon start before the fix. Now they say B -> B, as they should:

[SteveSandersonMS]

This is great, @jspuij! Thanks so much for handling the <option> case. Select boxes are always such a tricky thing. Your solution of processing the <option> after element insertion (rather than only on setting the value attribute) is a really good one and is simpler than what I had been thinking of.

There's just one other case where I think this change would regress, and unfortunately we don't have an E2E test case for it. My hope is that it will be fairly easy to fix, especially if you don't mind also adding an E2E test case for it. The scenario is if the developer changes an option's value dynamically while also setting that to be the new selection in the same batch. For example:

<select @bind="@selectValue">
    <option value="a">Value A</option>
    <option value="b">Value B</option>
    <option value="@dynamicOptionValue">Dynamic value</option>
</select>

... and then in some event handler:

selectValue = "Some new value";
dynamicOptionValue = "Some new value";

If I'm following correctly, this would have worked before, because the order would be:

1. Attempt to set the value of the <select> to "Some new value". Since there's no matching option, stash it in selectElem[selectValuePropname]
2. Attempt to set the value of the <option> to "Some new value". As part of this, scan up the DOM hierarchy and find the <select>, see the selectElem[selectValuePropname] matches up, so set the selection. Success!

But the new behavior would be:

1. Attempt to set the value of the <select> to "Some new value". Since there's no matching option, stash it in selectElem[selectValuePropname]
2. Attempt to set the value of the <option> to "Some new value". That's it - do nothing else. The select box won't be updated correctly.

So what I suggest would be an easy fix would be to take the logic that was previously here and is being moved here, and factor it out into a reusable function. Then you can call it from both places and we'll have it working in all scenarios :) Of course, both calls should have a comment to explain why they are each separately needed.

Do you think it might be possible to update the E2E test case and test code to show this situation working too? It should be possible just to add one new <option> whose value comes from a variable, and automate changing that variable and setting the select value to the same new value at the same time.

If you think I'm misunderstanding what would happen, please let me know!

[jspuij]

If you think I'm misunderstanding what would happen, please let me know!

Ah, I think I went with the incorrect assumption that the diff would be done on element level, but of course the diff is on frame level, which would result in only the attributes being in the diff, correct? Then your scenario is entirely valid and this would be a regression.
Do you mind me fixing this and writing an E2E test tomorrow? We're pretty much in the same time zone and I should really go and catch some sleep.
This has been a fairly complex issue, heavily underestimated by me, so thank you for your patience and guidance so far! I've learned a lot.

[SteveSandersonMS]

Do you mind me fixing this and writing an E2E test tomorrow? We're pretty much in the same time zone and I should really go and catch some sleep.

Of course! I'm done for the day now too. It might be another team member who helps you with this tomorrow, but be assured we'll be happy to take this change. (Assuming it doesn't blow up in complexity further 😄 )

[jspuij]

@SteveSandersonMS I incorporated the latest changes and an addition to the E2E BindSelectTest. It seems to do everything correctly now. I have also rebased and squashed for easy overview and merge.

[jspuij]

Ahhh, you're killing me with all these rebases in this branch because of the ever increasing commits on master ;-)

[SteveSandersonMS]

Awesome, thanks so much for this!

[jspuij]

Awesome, thanks so much for this!

Well thank you for your time and guidance!