Financial-Times / ftdomdelegate

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

Can't listen to window.scroll #32

Closed wheresrhys closed 10 years ago

wheresrhys commented 10 years ago

I have

var del = new Delegate(window);
del.on('scroll', null , function () {}); // fails
del.on('scroll', 'html' , function () {}); // fails
del.on('resize', null , function () {}); // succeeds

I'm not sure what I think should be done about this, but it's not nice behaviour. I'm gonna need to set up an additional delegate instance to cope with it

var del = new Delegate(document);
del.on('scroll', null , function () {}); // will succeed
matthew-andrews commented 10 years ago

Oh wow, great spot.

The problem is although you add the event listener for scroll events on the window, the target node for a scroll event is the document so this line fails to match: https://github.com/ftlabs/ftdomdelegate/blob/master/lib/delegate.js#L403.

For now I can put in a hack above that so that it is:-

Delegate.prototype.matchesRoot = function(selector, element) {

  // HACK:MA:20140425 If the rootElement is the window, no event will
  // ever come through targeting it, so assume that if the `document` is
  // the target then we have a match - fixes #32
  if (rootElement === window) return element === document;
  return this.rootElement === element;
};

But it's a bit nasty :-/. I'll implement this over the weekend with a regression test (unless I can think of something better).

matthew-andrews commented 10 years ago

Also I think as scroll events don't bubble their default useCapture should be true. I should also add a test for overflow: scrolld block elements.

wheresrhys commented 10 years ago

Ah good. Though I was starting to come to the conclusion that setting up a delegate on window is a misuse and should be documented as such; you can just use document unless you're listening for an event on window, in which case you're not using the delegate pattern anyway.

Another one to look at might be orientationchange (although this will probably be well behaved like resize - it's just less trivial to verify)

matthew-andrews commented 10 years ago

Actually we probably should do this

var del = new Delegate(document);
del.on('scroll', function () { console.log(arguments); }); 

But that's broken for other reasons. Argh. Will look into this properly

matthew-andrews commented 10 years ago

Hi @wheresrhys,

It seems to work OK actually with:

var Delegate = domDelegate.Delegate;
var del = new Delegate(document);
del.on('scroll', function () {
  console.log(arguments);
});

See here: http://jsbin.com/xemut/2/

Bad things happen when you set useCapture to be true though...