search

Financial-Times / ftdomdelegate

Create and manage a DOM event delegator
MIT License
320 stars 36 forks source link

Scoped selector #50

Closed orangemug closed 9 years ago

orangemug commented 10 years ago

Added scoped selector support. This uses data attributes so it'll suffer on perf a bit, although looking at this its not too bad http://jsperf.com/queryselectorall-vs-getelementbyid/32

orangemug commented 10 years ago

So what is the general opinion on this PR? Merge or close?

matthew-andrews commented 10 years ago

OK. I am really sorry that it has taken me so long to think this through and I think I am finally clear in my head about how I feel about this.

One of the main design principles behind DOM Delegate is that you should be able to detach it from one bit of DOM and attach it to another and for it to continue working.

So imagine the DOM looked like this

<body>
  <section id="pane-1">
    <button>Click me</button>
  </section>
  <section id="pane-2">
    <button>No, Click me</button>
  </section>
</body>
var pane1 = document.getElementById('pane-1');
var pane2 = document.getElementById('pane-1');

var dd = new Delegate();
dd.on('click', '> button', function() {
  console.log("button clicked");
});

dd.root(pane1);
// Clicking 'Click me' => console.log
// Clicking 'No, click me' => nothing

dd.root(pane2);
// Clicking 'Click me' => nothing
// Clicking 'No, click me' => console.log

To being able to support this I believe we need to keep all state in memory in javascript. (Or also add UIDs to the DOM as you de-root and re-root DOM delegates - which feels like a fair amount of overhead...)

Even though it'd probably be less performant I'd actually prefer an implementation that walked over the root element's current children looking for a match - as that wouldn't require modifying (and therefore holding state in) the DOM. I don't mind doing this work.

Something like...

function matchesDirectChild(selector, element) {
  var children = this.rootElement.children;
  for (var i=0, l=children.length; i<l; i++) {
    if (element === children[i] && matches.call(children[i], selector)) {
      return true;
    }
  }
}

What do you think?

orangemug commented 10 years ago

I'm not sure that works, unless I'm misunderstanding something. I think you're going to have to write a css rule parser to achieve this functionality. Imagine the DOM structure

<div>
   <div>
      <div></div>
   </div>
</div>

How would the following selector get matched in your above example?

> div
orangemug commented 10 years ago

I'm also not sure its exactly storing state in the DOM, it's just making them selectable. It just happens to be unique it's no different to any other class/id on the DOM element is it? Also what are the downsides of having a unique data-attribute on a DOM element?

Removing data-attributes when we change the root (good spot) won't be difficult, let me know what you think.

orangemug commented 10 years ago

It works in firefox/chrome locally but not on travis, I'll work that out tomorrow. Let me know what you think

orangemug commented 10 years ago

I realised I hadn't yet put a use case for this. So if you're using a framework like fruitmachine and you have a module with nested modules of the same type. How would you bind to the correct DOM node. For example

<div class="menu">
  <a href="#" class="menu__title">root</a>
  <div class="menu__children">
    <div class="menu">
      <a href="#" class="menu__title">sub-menu item1</a>
    </div>
    <div class="menu">
      <a href="#" class="menu__title">sub-menu item2</a>
    </div>
  </div>
</div>

Adding a selector of .menu__title to the root would fire on any child nodes as well as the root menu. The current API makes nested menus really difficult.

orangemug commented 10 years ago

Also note this functionality is included in http://dev.w3.org/2006/webapi/selectors-api2/ and looks like there is a shim https://github.com/lazd/scopedQuerySelectorShim. However I don't think Element.matches would work with :scope, so we'd have to change domdelegate to use querySelector in the scoped case.

So I still think this pull request is the correct approach, I also prefer the > .direct syntax, rather than the :scope > .direct syntax for our usage.

@matthew-andrews any thoughts?

lazd commented 10 years ago

Just chiming in here as the author of scopedQuerySelectorShim. I believe scoped delegation is required functionality, but unfortunately something you'll have to implement manually even full support for :scope is present in the browser. The querySelector approach will work, and you can require that your customers include a shim if they want to support it. This will change the browser support for this library, so it is definitely a breaking change.

orangemug commented 10 years ago

This will change the browser support for this library, so it is definitely a breaking change.

@lazd sorry I don't understand, are you saying this pull request will change browser support? If so can you highlight which bit.

lazd commented 10 years ago

@orangemug, I was implying that, without an ID hack (as in scopedQuerySelectorShim), relying on querySelector alone would result in a browser support change.

Looking at your PR, since you do the ID hack, it should work everywhere.

orangemug commented 10 years ago

Thanks @lazd, just waiting to see if @matthew-andrews is happy with this in the lib now (then I'll work out why the tests are failing on travis)

orangemug commented 9 years ago

@matthew-andrews can you decide if you want this in the lib or not, and comment / close.

orangemug commented 9 years ago

Ok I've decided I'm going to try to maintain my own fork of ftdomdelgate, I'll merge this ticket into that fork instead. Closing ticket, I'll update this ticket with details for anyone thats interested.

orangemug commented 9 years ago

Started a forked repo at https://github.com/orangemug/domdelegate for anyone who wants this feature