fix(dom): change EventDelegator bubble to search topElement instead of body

[ajmchambers]

• I added new tests for the issue I fixed/built
• I ran npm test for the package I'm modifying
• I used npm run commit instead of git commit

Searching the body will not find the element inside a DocumentFragment or shadowRoot, so the function ends up exiting early and the event is ignored. Added a test for a click inside a DocumentFragment, behaviour is similar to shadowRoot but has better browser support.

ISSUES CLOSED: #412

[Widdershin]

This looks good to me :)

It's nice to see small changes backed by quality tests.

[Widdershin]

The test is timing out in a bunch of browsers. I wonder why?

[ajmchambers]

Hmm yea, I'll see if I can figure out what's going on.

[staltz]

Thanks @ajmchambers for this PR. It makes sense. I'll add some comments inline.

[staltz]

We use just element.click() in other tests, so I think you can remove this helper function.

[staltz]

Maybe add more time here, like setTimeout(() => ...., 80).

[ajmchambers]

I've been playing around with Chrome 39 (one of the browsers that failed the test) to see if I can figure out whats going on.

It would appear (in Chrome 39 at least) that events aren't bubbling in detached dom.

var parent = document.createElement('div')
var child = parent.appendChild(document.createElement('div'));
parent.addEventListener('e', function() {
  console.log('event bubbled to parent');
});
child.addEventListener('e', function() {
  console.log('child dispatched an event');
});
child.dispatchEvent(new CustomEvent('e', { bubbles: true }));

Logs both to the console in modern chrome and safari etc, but in chrome 39 it logs the initial child event but not the parent one.

[mightyiam]

Congrats on debugging win. Is it out of curiosity or is there a reason to support Chrome 39 specifically?

[ajmchambers]

Travis failed because the test I wrote failed in the following browsers:

• iPhone Safari 8.0
• iPhone Safari 9.0
• Chrome 30.0
• Chrome 39.0
• Safari 8.0
• Safari 9.1

So in Chrome 39 (at least) I think my test failed because the event didn't actually bubble - which is the point of my test in the first place. Not sure about the other browsers yet, may be the same issue or something else.

I'm pretty new to this in general, but is there a best practice for tests that you expect to fail in certain situations and not others? (or is that just a bad test)

I suppose you could do this:

function eventsBubbleInDetachedDOM() {
  // checks and returns boolean
}

it('should catch bubbling events in a DocumentFragment (and shadowRoot)', function (done) {
  if(eventsBubbleInDetachedDOM()) {
    // run the test
    done();
  } else {
    done();
  }
});

or this:

function eventsBubbleInDetachedDOM() {
  // checks and returns boolean
}

if(eventsBubbleInDetachedDOM()) {
  it('should catch bubbling events in a DocumentFragment (and shadowRoot)', function (done) {
    // run the test
    done();
  });
}

But it doesn't seem right having branching logic for tests.

Any ideas?

(Busy at the moment so won't be able to have a proper look at this again until later on tonight - but when I do get a chance I will make the requested changes and see what happens).

[staltz]

You can do something like: https://github.com/cyclejs/cyclejs/blob/master/dom/test/browser/render.js#L213

But I'm curious what exactly don't those old browsers support.

[ajmchambers]

Hopefully this fixes it: added a check to see if events will capture or bubble in DocumentFragment and added a test for useCapture. Will check back later.

Btw should I also be using npm run commit when updating a pull request?

[ajmchambers]

Ok, now I'm a little confused: dom tests passed ok, but history tests are failing somehow... could my change in the dom EventDelegator have affected history somehow?

Will have a look into it later on. If it was could maybe change my check to something like:

if ( 
  !(
    document.body.contains(rawEvent.currentTarget as Node) ||     
    this.topElement.contains(rawEvent.currentTarget as Node)
  )
) {
  return;
} 

[staltz]

Thanks for the PR!