render() should not use a <span> to contain rendered elements

[marad]

The render() function internally uses span as it's container element. Removing it would be a god send, but I don't think there is an easy way to do this so what we can do instead is provide a way for user to give it some custom element as container.

The problem I've stumbled upon goes like this. I want to create dynamically filled dropdown using fomantic. So I did this:

val areaId = KVar("")
div(fomantic.ui.search.selection.dropdown).new {
  input(type = InputType.hidden).setValue(areaId, updateOn = "change")
  i(fomantic.dropdown.icon)
  div(fomantic.menu).new {
    render(state.listAreas()) { areas ->
      areas.forEach { area -> div(fomatnic.item).setAttributeRaw("data-value", area.id).text(area.name)
    }
  }
}

This works, but because of the span it doesn't look right (styling is wrong). So instead I think we could do this:

val areaId = KVar("")
div(fomantic.ui.search.selection.dropdown).new {
  input(type = InputType.hidden).setValue(areaId, updateOn = "change")
  i(fomantic.dropdown.icon)
  render(stat.listAreas(), container = div(fomantic.menu)) { areas ->
    areas.forEach { area -> div(fomatnic.item).setAttributeRaw("data-value", area.id).text(area.name)
  }
}

Tell me what do you think.

[marad]

I just thought that if we went this way it would be pretty easy to make an extension function that would make things even cleaner (IMO anyway 😄)

div(fomantic.menu).render(state.listAreas()) {
  // ... 
}

I'm not sure about the name I think it would be confusing to call it render here. Maybe renderChildren or renderContents would be a better options.

Or go with something like dynamicNew 😃 which is obviously a joke, but with this API it's kind of apparent that new makes static content and this render* makes dynamic content for the block.

Thinking about it I'd even say that I'd move render function into the Element so we have an API like this, and then make the old extension function just do span().renderContents(kvar, block)

[marad]

I've experimented with this a bit and it seem to work fine but I'm not sure what to do with this:

kweb-core/src/main/kotlin/kweb/Element.kt

Lines 426 to 433 in c2073e9

	// this.onCleanup(false) {
	// containerSpan.deleteIfExists()
	// }
	// this.onCleanup(true) {
	// previousElementCreator.getAndSet(null)?.cleanup()
	// kval.removeListener(listenerHandle)
	// }

I think that the first onCleanup is not necessary because render is not managing the container anymore (it should probably just stay in the old render function), but the second one looks important and I have no idea how to achieve that inside the Element.

[sanity]

Requiring a wrapper element like the <span> for render {} was kinda a kludge from the start, to make it easy to replace the rendered content when the KVar updates, although I didn't realize it could impact appearance.

But ElementCreators remember the elements they create (in ElementCreator.elementsCreated) so it should be possible to skip the span wrapper provided the ElementCreator ensures that the right DOM elements are replaced and the replacements go in the appropriate positions under their parent element.

In exchange for doing this server-side housekeeping we shouldn't need any wrapper element at all - would that address your use case?

[marad]

Yes, getting rid of wrapping <span> would do fix things for me. So as I understand - the render function on re-render should remove all elementsCreated and then start a rendering new ones? Does removed elements also get removed from elementsCreated?

[sanity]

I think if we reuse the same ElementCreator then it should be reset to the state it would have been in with no elements before we add any new elements, so yes I think we should remove elements from elementsCreated.

We'd also need to make sure that new elements get inserted at the same positions under the parent element as the replaced elements.

Another approach would be to use a new ElementCreator every time the KVar is re-rendered (deleting the old one and the elements that belong to it), but it's not clear to me at this point which would be cleaner.

[marad]

Are there any other important factors? Like speed for example?

[sanity]

It should be possible to make this improvement without sacrificing speed. Currently we can just call Element.removeChildren() on the wrapper <span>, which lets the browser do the work of figuring out which elements to delete.

The proposed new approach would require the server to pass a list of the element IDs to be deleted to the browser which would then delete them, this should be a single websocket message. I don't anticipate this being inefficient unless there are a very large number elements to be deleted.

Oh, and we also need to ensure that any subsequently added elements are added in the same position as those they replace.

[marad]

Oh, and we also need to ensure that any subsequently added elements are added in the same position as those they replace.

This one can be hard. If we don't have the container element the children added by render could be interleaved with other components. Then re-render would have a really hard time to address that.

[sanity]

Yeah, getting element replacement right in situations where there are sibling elements that are not part of the render {} block is the headache that prompted the <span> solution in the first place.

I'm confident it's doable though if we think about the problem the right way, needs more thought.

[sanity]

Note: version 0.7.19 doesn't yet eliminate the need for a container element, but it does allow you to change the type of element used with render(containerElementTag = "li") { .. }.

[marad]

This is nice!
Would it work with classes like render(containerElementTag = "li.item")?

[sanity]

Not right now I'm afraid, I could add another parameter to render to support configuration of the container element - although this would just be a stop-gap until we can remove the requirement for the container element altogether.

The stop-gap would be something like:

render(containerElementTag = "li", containerConfig = { it.classes("item") }) { ... }

Not very elegant, but hopefully temporary - would this work for you?

[marad]

Well it would work for this exact use-case. Why you don't like the idea of just passing Element as the parameter? User could preconfigure it however they like.

[sanity]

Good point, just released 0.7.20 which supports this:

render(container = { li().classes("item") }, value = v) { ... }

(The reason I'm not allowing a raw Element to be passed in his to ensure that the element is a child of the parent ElementCreator)

[marad]

So would something like this work?

val el = li().classes("item")
render(container = { el }, value = v) { ... }

[sanity]

Yes - that would probably work with the current implementation - so I guess it was inaccurate to say it would ensure use of the parent ElementCreator, but it would encourage it (I view this as a stopgap measure until we can remove the need for a container element, it's far from perfect).

Is there a particular reason to pass in an Element rather than creating it within the container function?

[sanity]

Relevant for appending multiple elements:

• https://johnresig.com/blog/dom-documentfragments/
• https://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-B63ED1A3
• https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ms766442(v=vs.85)

Notes:

• ElementCreator should be able to add elements to a DocumentFragment rather than just a parent element.
• CreateElement instruction should be generalized to support this

[sanity]

Somewhat related: #128, #177

[sanity]

After quite a bit of thought and discussion with @Derek52, I think I have a solution.

While it would be nice if we could do this without needing to add any additional elements to the DOM - I don't think this will be practical because any approach I can think of for keeping track of which elements need to be replaced has some very nasty edge cases.

Rather than wrapping the rendered elements in a noop marker <span>, we add a noop "begin" before the rendered elements, and an "end" after them. Note these spans will be siblings of the newly added elements. The ids of these marker spans are noted within the render function.

We modify ElementCreator so that instead of providing a position, we can provide the id of an element before which any new elements will be added using insertBefore(). We can use this to insert elements before the end - which will ensure they're added in the correct order.

We should also predefine a JavaScript function that can be given two element ids and which will delete any sibling elements between them. This can be used by render to clear previously added elements before re-rendering. It will be important to ensure that ElementCreator.cleanup() is called for the deleted elements.

[sanity]

Render was redesigned a while ago to remove the <span>.