ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

When cloning elements, clone events from all child elements #65

Closed benvinegar closed 12 years ago

benvinegar commented 12 years ago

This fixes a "bug" where if you use appendTo, events are only cloned from the src element, and not its child elements (when Bean is present).

Hastily written, un-verified example (using Ender):

var div = $('<div><span/></div>');
div.find('span').bind('click', function() {
  console.log('click');
});
div.appendTo('body');
div.find('span').trigger('click'); // does nothing

I'd love to write a test for this, but the Bonzo tests don't include Bean right now, and I'm not 100% sure how to proceed.

valueof commented 12 years ago

I approve

ded commented 12 years ago

right. ok... so... there's at minimum, an integration page -- and you could add bean to the ender instance there. the page isn't exactly a full proof thing — i built it just as a gutcheck page to make sure things appear to be working.

benvinegar commented 12 years ago

What if I turned integration.html into a proper sink test suite?

ded commented 12 years ago

well, i think what we really need is a good integration suite for Ender's Jeesh.

benvinegar commented 12 years ago

So, integration.html already includes bean, and there's a line where you bind a click event on a top-level element. But you don't fire the event to see if it was successful.

https://github.com/ded/bonzo/blob/master/integration/integration.html#L38

I guess I don't know what purpose it serves to add my bug scenario here if we're not asserting that it works.

Anyways, I'd love to get this patch accepted – please tell me what you need to make that happen.

benvinegar commented 12 years ago

Ping. Let me explain why this is an issue.

Normally, I use event delegation everywhere. But you can't delegate focus events. Which means that, with the current version of Bonzo, you can't apply a focus event handler to an element's child unless it's already on the DOM.

rvagg commented 12 years ago

+1 on a sink-test version of the integration tests @benvinegar, perhaps if you started that here with the basics that are already in integration.html plus tests for this PR we could expand on it and move it into ender-js/jeesh.

benvinegar commented 12 years ago

Hmm, not sure how this became closed (and I definitely did not merge this into ded:master), but I opened up another pull request: #70