Closed Bug 1170166 Opened 4 years ago Closed 4 years ago

Add a capture flag to BrowserTestUtils.waitForEvent

Categories

(Testing :: Mochitest, defect)

defect
Not set
Points:
1

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8613524 - Flags: review?(smacleod)
Blocks: 1132518
Comment on attachment 8613524 [details] [diff] [review]
waitForEventCapture

Review of attachment 8613524 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a huge fan of adding the mandatory boolean argument here, but for the most part I'd r+ this. I'm going to pass this review off to Paolo though as he's more opinionated about the convention of BrowserTestUtils and I feel he'd be a better reviewer here.
Attachment #8613524 - Flags: review?(smacleod)
Attachment #8613524 - Flags: review?(paolo.mozmail)
Attachment #8613524 - Flags: feedback+
Comment on attachment 8613524 [details] [diff] [review]
waitForEventCapture

Review of attachment 8613524 [details] [diff] [review]:
-----------------------------------------------------------------

Considering how rarely we ended up using the callback argument, I'd say we can add the optional "capture" argument before it. In fact the "focus" event can only be seen through a capturing listener and we don't support "focusin" yet, so we have a compelling use case.

::: layout/xul/test/browser_bug703210.js
@@ -19,5 @@
>      is(event.originalTarget.localName, "tooltip", "tooltip is showing");
>      return true;
>    });
>    yield BrowserTestUtils.synthesizeMouseAtCenter("#p1", { type: "mousemove" }, browser);
>    yield popupShownPromise;

For the record, I believe we could rewrite at least the first one (maybe the second too, if it doesn't rely on being synchronous) as:

let popupShownPromise = BrowserTestUtils.waitForEvent(document, "popupshown");
yield ...synthesizeMouseAtCenter...;
let event = yield popupShownPromise;
is(event.originalTarget.localName, "tooltip", "tooltip is showing");

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +242,5 @@
>     *        The element that should receive the event.
>     * @param {string} eventName
>     *        Name of the event to listen to.
> +   * @param {bool} capture
> +   *        True to use a capturing listener

@param {bool} capture [optional]

nit: dot at the end
Attachment #8613524 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/94310347a60b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Points: --- → 1
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.