https://bug1236979.bmoattachments.org/attachment.cgi?id=8714112 only has tests for bubbling. This bug adds tests for capturing as well.

Comment on attachment 8716437 [details] [diff] [review] Part 1 - Whitespace tweak for existing test. ># HG changeset patch ># User Mike Taylor <miket@mozilla.com> > >Bug 1246246. Part 1 - Whitespace tweak for existing test. r=whitespace r=[explanation-instead-of-a-person] is somewhat frowned upon. (I remember seeing senior folks recommend against it in some newsgroup thread, though I don't recall exactly where. Part of the reason is that it breaks the ability to do analysis on commit messages to extract reviewer names, for example. And part of the reason is that "r=" implies "this was reviewed by", so r=[reason] is kind of lying.) For not-worthy-of-review changes, I think the best practice is just to add a suffix like "(no review, whitespace-only)" at the end of your commit message. (At least, that's what I do for this sort of thing. Not sure how representative I am.) (Historical note: I think people started doing "r=whitespace", "r=bustage", etc. at one point because we had a hook that *required* "r=something". But we don't have that hook anymore.)

(Also: thanks for cleaning/improving this test! :D )

(In reply to Daniel Holbert [:dholbert] from comment #4) > Comment on attachment 8716437 [details] [diff] [review] > Part 1 - Whitespace tweak for existing test. > > ># HG changeset patch > ># User Mike Taylor <miket@mozilla.com> > > > >Bug 1246246. Part 1 - Whitespace tweak for existing test. r=whitespace > > r=[explanation-instead-of-a-person] is somewhat frowned upon. Ah, thanks for letting me know! I grepped in git logs to see how people handled these kinds of changes (not wanting to waste smaug's time with just a whitespace commit). > For not-worthy-of-review changes, I think the best practice is just to add a > suffix like "(no review, whitespace-only)" at the end of your commit > message. (At least, that's what I do for this sort of thing. Not sure how > representative I am.) I'll change it to that, sgtm.

Comment on attachment 8716442 [details] [diff] [review] Part 3 - Add test for firing prefixed legacy event types (in capture phase). r=? >+function mpTestChildrenWithDiffListeners(eventInfo) { I don't understand the naming here. Event is dispatched to target, and propagate up to its ancestors (both in capture and bubble phases). Where does "children" come to the name? Use 'Capture' or 'Capturing' somewhere in the name. >+ return new Promise( >+ function(resolve, reject) { >+ var grandparent = createChildDiv(); >+ var parent = createChildDiv(grandparent); >+ var target = createChildDiv(parent); >+ var didEventFireOnTarget = false; >+ var didEventFireOnParent = false; >+ var didEventFireOnGrandparent = false; >+ var eventSentToGrandparent; >+ >+ grandparent.addEventListener(eventInfo.modern_name, >+ createHandlerWithTypeCheck(eventInfo.modern_name, function(e) { >+ eventSentToGrandparent = e; >+ didEventFireOnGrandparent = true; >+ resolve(); >+ }), true); I don't understand this. Why are we resolving the promise when grandparent gets the event during capture phase? I would expect the listener added to target to call resolve. >+ target.addEventListener(eventInfo.modern_name, >+ createHandlerWithTypeCheck(eventInfo.modern_name, function(e) { >+ is(e.eventPhase, Event.AT_TARGET, >+ "event should be at target phase"); >+ is(e, eventSentToGrandparent, >+ "Same event object should capture, " + >+ "despite difference in type"); >+ ok(didEventFireOnParent, >+ "Event should have fired on parent"); >+ // Clean up. >+ grandparent.parentNode.removeChild(grandparent); >+ })); Why target doesn't use capture phase for addEventListener call? Those fixed or explained, r+

(In reply to Olli Pettay [:smaug] (high review load) from comment #9) > Comment on attachment 8716442 [details] [diff] [review] > Part 3 - Add test for firing prefixed legacy event types (in capture phase). > r=? > > > >+function mpTestChildrenWithDiffListeners(eventInfo) { > I don't understand the naming here. Event is dispatched to target, and > propagate up to its ancestors (both in capture and bubble phases). > Where does "children" come to the name? > Use 'Capture' or 'Capturing' somewhere in the name. No great reason for the name, just a bad name (and I think I changed it at the last minute, NyQuil is a helluva drug). It probably makes sense to change the mpTestAncestorsWithDiffListeners test name as well and use Bubbles or Bubbling in the same way. > >+ return new Promise( > >+ function(resolve, reject) { > >+ var grandparent = createChildDiv(); > >+ var parent = createChildDiv(grandparent); > >+ var target = createChildDiv(parent); > >+ var didEventFireOnTarget = false; > >+ var didEventFireOnParent = false; > >+ var didEventFireOnGrandparent = false; > >+ var eventSentToGrandparent; > >+ > >+ grandparent.addEventListener(eventInfo.modern_name, > >+ createHandlerWithTypeCheck(eventInfo.modern_name, function(e) { > >+ eventSentToGrandparent = e; > >+ didEventFireOnGrandparent = true; > >+ resolve(); > >+ }), true); > I don't understand this. Why are we resolving the promise when grandparent > gets the event during > capture phase? > I would expect the listener added to target to call resolve. Oh, you're right. Will fix. > >+ target.addEventListener(eventInfo.modern_name, > >+ createHandlerWithTypeCheck(eventInfo.modern_name, function(e) { > >+ is(e.eventPhase, Event.AT_TARGET, > >+ "event should be at target phase"); > >+ is(e, eventSentToGrandparent, > >+ "Same event object should capture, " + > >+ "despite difference in type"); > >+ ok(didEventFireOnParent, > >+ "Event should have fired on parent"); > >+ // Clean up. > >+ grandparent.parentNode.removeChild(grandparent); > >+ })); > Why target doesn't use capture phase for addEventListener call? Oversight, will fix. Thanks!

I'm gonna go with mpTestDiffListenersEventCapturing and mpTestDiffListenersEventBubbling for the test re-names.

Carrying forward r+ (just updating commit message to reflect test name change)

Carrying forward r+, I've fixed the issues with the test name, resolving the promise in the target's listener, and making sure target is listening for capture phase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e4f00f575a https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec53494c7c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/42e2296a5b47

https://hg.mozilla.org/mozilla-central/rev/f9e4f00f575a https://hg.mozilla.org/mozilla-central/rev/6ec53494c7c5 https://hg.mozilla.org/mozilla-central/rev/42e2296a5b47