Flow: Create and configure elements in child-before-parent order

[Legioth]

For instance <option> elements for a <select> need to be added before the value attribute for a select is added

[macjuan]

Added to the backlog

[Artur-]

<marked-element> (https://elements.polymer-project.org/elements/marked-element) supports defining the markdown using a <script type="text/markdown"> child. This does not work if you do

        Element script = new Element("script");
        script.setAttribute("type", "text/markdown");
        script.setTextContent(codeBlock);
        getElement().appendChild(script);

The script tag is read in the ready event and at that point it seems the script tag is not available.

[Legioth]

Components that read their contents in the ready event are "impossible" for us to support, but we can do something about attach event components by attaching things in the right order.

[Artur-]

1. Update properties/attributes from state tree
2. Create child elements and recursively do the same steps as for the parent
3. Attach child elements

Test cases:

• ViewTestLayout.onLocationChange should be removed and select in View tests should still work
• <select> should work inside a template

[Legioth]

My idea for how this could be implemented without implementing separate logic for building the DOM and doing incremental updates would be like this:

• Explicitly collect all Computation instances for each binding. This also helps unify the logic for stopping computations when the state node is unregistered.
• Record the binding used for each state node.
• When building the DOM for a node, it flushes all child bindings before attaching child elements.
• Flushing a binding causes all its computations to be recomputed in an appropriate order, including flushing any child bindings.

[Haprog]

Components that read their contents in the ready event are "impossible" for us to support

Why is this?

I made a Polymer Element I try to use like this in a Hummingbird template:

<element-demo-section>
  <element-demo-item name="basic-demo" title="Basic">
    <div class="description">...</div>
    <code class="execute">...</code>
    <code class="sample-polymer">...</code>
    <code class="sample-angular2">...</code>
  </element-demo-item>
  <!-- more element-demo-items ... -->
</element-demo-section>

The custom elements here try to set properties based on their contents. element-demo-item could use attached or ready event to do this but currently neither seems to work since the contents aren't there yet when the events are run. I also tried using Polymer's observeNodes(callback) instead for reading the contents but that also fails with Hummingbird since (even though it detects when nodes are added to element-demo-item) the div and code elements don't have their contents set yet when they're added and then the observer callback doesn't get notified anymore when the contents is added since the div and code elements still refer to the same elements (they weren't added again, only modified).

If I only had to deal with static content inside I guess I could work around this at least in a couple of ways for example by using my workaround mentioned here https://github.com/vaadin/hummingbird/issues/1190#issuecomment-254602742

But because I want to supply the contents to this component using Hummingbird data binding like this:

<element-demo-section>
  <element-demo-item [attr.name]="demo.name" [title]="demo.displayName" *ngFor="let demo of demos">
    <div class="description" [innerHTML]="demo.description"></div>
    <code class="execute" [innerHTML]="demo.htmlCode"></code>
    <code class="sample-polymer">{{demo.polymerExampleCode}}</code>
    <code class="sample-angular2">{{demo.angularExampleCode}}</code>
  </element-demo-item>
</element-demo-section>

It just doesn't work.
I would really like to see this issue fixed.

Only ways I can think of to workaround this now is to use something like setTimeout or setInterval to check if the contents didn't change in a while and only then try to process it or I think I could generate the HTML for this element-demo-section composition in Java code as an HTML string I would bind to innerHTML of a div.

[Artur-]

Components that read their contents in the ready event are "impossible" for us to support

Why is this?

The "ready" event is fired directly after you run document.createElement so there is no time to attach the children before the "ready" event

Effectively if you read the children in the "ready" event in a web component, you only support static HTML and not JS element creation

[Haprog]

Artur: Thanks for the explanation.

[Haprog]

I just noticed this an wanted to comment on Leif's original note:

For instance <option> elements for a <select> need to be added before the value attribute for a select is added

Actually there is no value _attribute_ for <select> and there shouldn't be any issue using the selected attribute or property for the <option> elements even if they're configured before inserting into the parent <select>. But there is a value _property_ for the <select> for which this can be an issue e.g. if you try to set that using Hummingbird property binding in a template.

[Legioth]

Won't do this before we have sorted out the template stuff since it's fully possible that it will cause changes to how Computation is used.

[mstahv]

Decision makers, please also see the discussion in the closed #3651 to see the importance of this issue. If this wont be solved soon properly, we need a lot of quality documentation to play down the WTFs this causes.

[amahdy]

Oh hey there, my bug was closed so it seems I need to bug this one. Initial impression feels that there will be a lot of complains about this problem. I was told that few people internally ran into this as well.

[mstahv]

I'm trying to build a flow wrapper for this component: https://github.com/miztroh/wysiwyg-e/blob/v2.1.0/README.md

I guess it is this bug that makes it impossible to configure e.g. available heading levels from Flow. The web component reads them when the "wysiwyg-tool-heading" element is attached, but Flow sets them too late? Only way to configure this component would be to use template?

[mstahv]

It might be that my issue with the wysiwyg-e is not about this. Don't know for sure. With Flow a menu is not visible, without it displays fine.

[Legioth]

In general, it's a bug in the web component if it doesn't support incremental updates (i.e. it doesn't react to changes after it has been attached).

Still, it's quite common that web component implementations have such bugs, so this is not a reason for Flow not to do things in an order that would work better.

[samie]

Yes, I ran into this one also. If not supported, we need to document how to make properly Vaadin Flow compatible web components. By default web components are not really designed in a dynamic way and are primarily meant to be used in a rather static HTML document not in a SPA.

[samie]

I very much agree what @amahdy already said in #3651 ("The logic of execution is different from what I wrote in Java, add(..) is the last line of the code so I expect it to happen last."). Would also expect to keep the order of method calls:

    HtmlContainer parent = new HtmlContainer("parent-element");
    HtmlContainer child = new HtmlContainer("child-element");
    child.getElement().setAttribute("attr", "value"); // attr value used in component ready
    parent.add(child); // builds "detached" parent-child tree
    add(parent); // attaches everything once, ready invoked

i.e at least in this case I'm not sure, if it is really needed to be "child-before-parent", but just _maintaining the order_ of element API calls.

[Legioth]

Should element API call order also be preserved in cases like this:

Element e = new Element("div");
e.setAttribute("foo", "bar");
e.callFunction("focus");
parent.appendChild(e);
e.removeAttribute("foo");

The cases of interest here are:
1) Calling focus() before the element is attached (which has no effect)
2) Setting an attribute that is removed within the same round trip and therefore only observable by side effects in focus() or appendChild().

[mstahv]

I think it should.

[samie]

Yes, I think sounds logical and generally understandable. I would expect symmetric behaviour (as much as possible) of elements/components both on client and server side.

[Legioth]

Understandable sure, but is it practical if focus() doesn't work and lots of overhead may be sent to the client unless you're really careful to not set any properties eagerly if might still change during the same round trip?

[mstahv]

We have absolutely no idea how focus() works from the frameworks perspective. I think it is up to the component implementation to fix the functionality in that case. The Java API (and possibly some framework helpers) or the web component should fix the broken web.

[Legioth]

The framework's own focus() implementation could be made to work in whatever way is necessary. I'm referring to any custom JS that directly or indirectly calls something like focus() or document.getElementById("foo") that won't work before the DOM tree has been constructed and attached.

The hypothesis is that this ordering would work in 99% of all cases, and for the remaining cases it would have to be possible to do something to override the ordering.

1. Create element instance
2. Attach all fully configured children
3. Set all properties and attributes
4. Attach the element to its parent (recursively, thus attaching it to the DOM)
5. Call any JS

The benefits of having a hardcoded ordering would be:

1. Multiple assignments to the same property or attribute can be coalesced to avoid wasting network bytes and client-side CPU on things that will be overwritten "immediately".
2. Running JS only after things have been set up. This is analogous to the way V7 first updates all shared state, then fires state change events for everything and only after that running RPC.
3. Setting up as much as possible before attaching the component (triggering e.g. Polymer's ready callback).

[samie]

Yes, sounds logical on "component level" APIs. On the other hand, on "DOM level" it would make sense to keep it as close as possible to original DOM logic. Then you can at least predict when some method call might fail because of detached state and change the order in your own code.

[Legioth]

The problem is that component and element "levels" are intermingled because the application developer might call component methods in an arbitrary order, which would in turn cause element methods to happen according to the same order. The ordering would thus only be effective for things that happen within one component-level method call.

[Legioth]

There are basically two different classes of this problem.

• The simple case is when there's only some specific property or attribute that absolutely needs to be set before attaching. An example of this is the official <google-map> web component where apiKey must be set from the beginning, and you cannot even change it afterwards without causing problems.
• The difficult case is when the entire web component is unresponsive to changes after attaching. For these cases, changing the ordering of operations would still only help with initialization, whereas incremental updates originating from user interaction would still not work.

It might be possible to do a workaround for the simple case that wouldn't require any heavy refactoring. The idea would be to add a low-level feature for assigning a "post create" (or "pre attach") JS snippet to a newly created Element instance. This would thus be independent of the regular logic for setting children, properties and attributes - those would still happen after attaching in the same way as now.

[amahdy]

The second use case is not our responsibility. If the webcompoenent is designed not to respond to changes why would we want it to respond?

It feels that you want to solve problems for a forecasted bad code, like bad WC design and setFocus when component is not attached yet. While in my opinion this is not our responsibility. Or at least the most important to have is: execute code in the correct order. As a developer I expect the logic I write to go as intended and not be part of a special case as mentioned in the workaround.

[amahdy]

Quick discussion with @samie , here is what I expect:

let x = document.createElement("div");
x.setAttribute("foo", "bar");
document.body.appendChild(x);

Result is:

<body>
 <!-- .. -->
 <div foo=​"bar">​</div>​
</body>

Straightforward. This is what any developer would expect. appendChild should go to the client and append whatever I have created before that line. All other use cases / error preventions are just side cases not the norm.

Also if a developer write a wrong code and after testing it finds that it works, s/he will be puzzled on why it works?! I don't normally expect frameworks to correct my semantic errors without explicitly telling me so.

[Legioth]

The final result is typically the same regardless of reordering or coalescing things. The interesting question is which intermediate states there will be.

One rather extreme example is a sequence like this:

parent.appendChild(Element.createText(oneMegabyteOfText));
parent.removeAllChildren();

Should this lead to sending a 1mb message to the browser, or should the framework be "smart" and leave out things that won't be included in the end result?

[amahdy]

My use case shows that it is not the same. I like to describe it as a constructor for the webcomponent. Attributes during initialize time are constructors that accept parameters. Initializing a component without any possibility to have attributes is equal to no possibility to have a non-empty constructor.

If I understand your examples correctly, you are mainly trying to lower the number of round trips to the client side? While this sound nice to have but again it’s a bad code of the developer so this shouldn’t be the priority. The priority is to execute the code as intended.

How is the situation now? Do you render the component on the server side and evaluate? Doesn’t seem so because for some reason the client side initialize an empty component first regardless of all Boolean operations your perform on the server side.

[willtemperley]

I've been working on a Flow wrapper for the LeafletJS mapping library where I've been running into this issue.

Essentially Flow doesn't support overloaded constructors for web components. Take the example of a web component wrapping a JS object with two optional constructor arguments [1]. With the current design I would have two observe two properties and delay construction until I get all the arguments. The problem is I don't know how many arguments the server-side code is going to send, so the problem is intractable.

The workaround / hack I've implemented instead is declaring a "constructorargs" string property on all components, which is actually a JSON serialisation of the constructor arguments. As I know the number of arguments, I can easily call the correct factory method to construct an object. It's messy but it works.

Unfortunately I think that unless Flow starts to support the same semantics as Polymer and web components in general, there will be a plethora of Vaadin-specific components created (as I'm doing) and a significant proportion of third-party web components will be unusable in Vaadin.

[1] http://leafletjs.com/reference-1.3.0.html#popup

[Legioth]

The "ready" event is fired directly after you run document.createElement so there is no time to attach the children before the "ready" event

This is how it used to work in Polymer 1, but no longer true as of Polymer 2. This change is based on changes in the custom element specification that restricts what can be done in a custom element constructor. It seems like nowadays, ready is triggered the first time connectedCallback is run (but still always before conenctedCallback triggers Polymer's own attached callback).

This means that whatever we do that covers attached will also cover ready.

[Legioth]

I realized that we can actually treat creation and initial binding for an element as a special case and in that way fix anything related to ready, attached and connectedCallback with a quite easy fix, which is in https://github.com/vaadin/flow/pull/4146.

This means that for a newly created element, the ordering will always be:

1. Add $server and @EventHandler stubs
2. Add DOM listeners that do RPC to the server
3. Attach children, virtual children and any server-provided shadow root
4. Set inline styles and class names
5. Set attributes and properties
6. Attach the element to its parent

Incremental updates to an existing element will still happen in an arbitrary order.

[Legioth]

I'll consider this issue closed since the main issue about components that read initial settings in ready, attached or connectedCallback has been fixed.

I've created https://github.com/vaadin/flow/issues/4157 for also applying incremental updates for an existing element in the same order.

Please create a separate issue if there's something else that has been discussed here that you still feel would be relevant to do something about.