kweb-core icon indicating copy to clipboard operation
kweb-core copied to clipboard

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

Open marad opened this issue 5 years ago • 19 comments

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 avatar May 30 '20 06:05 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 avatar May 30 '20 06:05 marad

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

https://github.com/kwebio/kweb-core/blob/c2073e9e5aeebe232772b802edf1a38a7509caa6/src/main/kotlin/kweb/Element.kt#L426-L433

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.

marad avatar May 30 '20 06:05 marad

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?

sanity avatar May 31 '20 18:05 sanity

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?

marad avatar May 31 '20 20:05 marad

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.

sanity avatar Jun 01 '20 00:06 sanity

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

marad avatar Jun 01 '20 06:06 marad

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.

sanity avatar Jun 01 '20 20:06 sanity

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.

marad avatar Jun 02 '20 09:06 marad

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 avatar Jun 02 '20 21:06 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") { .. }.

sanity avatar Jun 26 '20 14:06 sanity

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

marad avatar Jun 26 '20 17:06 marad

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?

sanity avatar Jun 26 '20 17:06 sanity

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.

marad avatar Jun 28 '20 11:06 marad

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)

sanity avatar Jun 28 '20 21:06 sanity

So would something like this work?

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

marad avatar Jun 29 '20 21:06 marad

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 avatar Jun 30 '20 21:06 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 avatar Aug 20 '20 15:08 sanity

Somewhat related: #128, #177

sanity avatar May 18 '21 17:05 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 avatar May 31 '21 14:05 sanity

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

sanity avatar Oct 09 '22 15:10 sanity