Access parentNode within a nested binding handler

[cervengoc]

I noticed an issue which probably exists with the original foreach as well.

When I have a nested binding handler which would use element.parentNode for whatever reason, it will fail because when fastForEach applies bindings to the item nodes, the nodes are not yet attached to their future parent, so the parentNode will always be a technical div.

I was thinking a bit about possible workarounds, but didn't find anything nice yet.

[cervengoc]

One way would be to extend the context with the future parent node, so it would be accessible for nested binding handlers. But need to think of nested fastForEach bindings too. But generally I don't like this idea too much because AFAIK the context doesn't hold any DOM nodes now, and it smells like violating its purpose.

[cervengoc]

Here is an example use case. Consider a selectable plugin which can be applied to a container like ul, and it will allow users to select child nodes by clicking, SHIFT clicking, CTRL clicking, etc.

<ul class='selectable-list' data-bind='fastForEach: items'>
  <li data-bind='selected: $data == $parent.selectedItem()'>
    ...
  </li>
</ul>

$(function() {
  // initialize our external plugin
  $(".selectable-list").selectable();
});

Now in the selected binding handler I need to call the Selectable plugin instance to select/deselect the item on observable change.

ko.bindingHandlers.selected = {
  init: function(element, valueAccessor) {
    // this will fail because the element is not attached to the `ul` element yet
    var selectable = $(element).parent().data('selectable');
    // ... call selectable.select(element, isSelected) somewhere in a computed
  }
}

@brianmhunt any thoughts? Is it a valid use case, or do you think this is a bad design? If it's bad, how would a correct one look like?

[brianmhunt]

Good question. 😄

[cervengoc]

Important news, the original foreach binding does not have this issue, here I've tested it: https://jsfiddle.net/3dLnzktw/. You can see that console logs the correct parent div.

So it seems like the original foreach binding first appends the nodes and then it applies child bindings.

Besides that I actually need it currently, I also think that this it's a major difference and probably it would be better to handle it as the original foreach binding.

Do you remember any reasons why you chose this order when implementing fastForEach? Anyway I'll have a look how this affects the code.

[brianmhunt]

It's a performance benefit IIRC - operate outside the DOM then insert.

Should be a trivial fix, and suggest we do it.

You ok putting a PR?

[cervengoc]

Yes I can next week sometime. But it would be interesting to test it if it matters and if it does, how much is the difference. In most cases I think the browser will not reflow until bindings have ended no matter when we bind, but maybe I'm completely wrong. I'll try to put together some performance tests.

[brianmhunt]

I never did any empirical testing of the advantage, so I'm curious to see results of a test.

Since we are timing on animationFrame the reflows should batch and improve performance.

We could make it an option to bind to parent before rendering, but I don't feel that should be a public part of the API.

[cervengoc]

@brianmhunt I was thinking quite a bit about this issue, as you can see in your mailbox probably :) (Actually sorry for spamming you with unsorted thoughts).

At the end I've found that in the current stage it's not worth dealing with this, because it would need too much code change, and probably it would have more drawbacks than benefits.

So what do you think about providing a workaround and extending the child contexts with $container? I think if this is well documented, it could even go through the merging into core.

The name can be $containerElement if we want to be very verbose about what it actually is.

[brianmhunt]

I'm wary of exposing $containerElement since it might give people the feeling that they can muck with it, but fastForEach is pretty fragile since it use DOM node offsets to calculate positions ... so I have a concern that people will muck with the container element and end up posting support issues that are hard to track down. Or we could document it really well. :)

Just an idea, once could put the selectable into a component, and then the $component is exposed, so something like this might work:

ko.components.register('selectable-widget', {
    viewModel: function(params) {
        this.items = params.source();
        this.selectedItem = params.selectedTarget;
    },
    template: {element: 'selectable-component'},
    synchronous: true
});

<template id='selectable-component'>
  <ul class='selectable-list' data-bind='fastForEach: items'>
    <li data-bind='selected: $data === $component.selectedItem()'>
      ...
    </li>
  </ul>
</template>

Do you think that solves the problem?

[cervengoc]

The use case was about accessing the parent node, not the context of it. I don't think component could help with that. I'll think about it. Still the best would be to apply child bindings on attached children.