Bug 1590682 Comment 13 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to Paul Morris [:pmorris] from comment #12)
> > +function sleep(window) {
> > +  return new Promise(resolve => window.setTimeout(resolve));
> > +}
> 
> Nice, that is useful.  Should we export this for use in other test files? 
> This pattern came up a lot in the mochitests I've worked on recently

It is a useful shorthand. I overuse timeouts, several of the ones in this patch are probably unnecessary.

As for exporting it, this probably isn't the best place to have it, and I especially don't want this file imported for any reason other than the existing ones. At some point I've got to do a grand rationalisation of a lot of Mochitest code, but it won't be until after the other Mozmill conversion is done.

If you want to copy these three lines into a head file for a directory of tests, that's probably the way to go for now.

> > +    synthesizeMouseAtCenter(input.getNode(), {}, iframe.window);
> 
> Huh, does the `{}` send a single click to the input?  Looks like it must,
> but you wouldn't know that just from looking at it.

Yeah, it does. I hate the arguments of this function, especially since the second is non-optional, even without the third.

> > +  synthesizeMouseAtCenter(menulist, {}, iframeWindow);
> > +  await BrowserTestUtils.waitForEvent(menupopup, "popupshown");
> 
> A general question: In cases like this, is it guaranteed that the popupshown
> event won't fire before we are awaiting for it to fire?  Or could it fire
> really quickly after the click before we're waiting on it? Or...
> 
> > +  let hiddenPromise = BrowserTestUtils.waitForEvent(menupopup, "popuphidden");
> > +  menupopup.hidePopup();
> > +  await hiddenPromise;
> 
> ...or should we use this approach of setting up the promise first, then
> awaiting it after doing the thing that causes it to resolve?

In the first case, we know the event won't happen until after the listener is added. The `mouseup` and `mousedown` events created by `synthesizeMouseAtCenter` are added to the end of a list of things to do. We're currently somewhere higher up the list, so this code will keep executing (and add the listener) until it runs out of things to do (in this case when it waits for a Promise that didn't immediately resolve) then do the next thing on the list.

Incidentally this is exactly what setTimeout with no (or 0) timeout does: it puts the callback on the end of the list. So we can use it to wait for things to happen, like custom elements or event handlers doing things.

In the second case, calling `hidePopup` fires the `popuphidden` event before returning. I'm pretty sure we don't need to wait for the event, but I've done it now. (I think at some point I was closing this menu pop-up a different way then got frustrated and did it this way.)

> > +  if (!BrowserTestUtils.is_visible(label)) {
> > +    menuitem.click();
> 
> .click() here instead of synthesizeMouseAtCenter ?  Probably fine, just
> curious since elsewhere you're using the latter to send clicks.

I usually consider this to be cheating but it works. This menu item is part of the window's main menu which means it's a native menu item on Mac OS and we can't fire click events at it.

> > +  await new Promise(resolve => iframeWindow.setTimeout(resolve, 500));
> 
> Hm, I thought eslint was outlawing such setTimeouts now?

(I only remembered this was there after submitting the patch. I can't for the life of me work out why it's necessary but *something* is going on here.) I got away with it because this of where the file is. The path doesn't match any of the patterns for XPCShell or Mochitest files. TBH I'd probably just silence the linter warning here anyway. ;-)
(In reply to Paul Morris [:pmorris] from comment #12)
> > ```
> > +function sleep(window) {
> > +  return new Promise(resolve => window.setTimeout(resolve));
> > +}
> > ```
> 
> Nice, that is useful.  Should we export this for use in other test files? 
> This pattern came up a lot in the mochitests I've worked on recently

It is a useful shorthand. I overuse timeouts, several of the ones in this patch are probably unnecessary.

As for exporting it, this probably isn't the best place to have it, and I especially don't want this file imported for any reason other than the existing ones. At some point I've got to do a grand rationalisation of a lot of Mochitest code, but it won't be until after the other Mozmill conversion is done.

If you want to copy these three lines into a head file for a directory of tests, that's probably the way to go for now.

> > ```
> > +    synthesizeMouseAtCenter(input.getNode(), {}, iframe.window);
> > ```
> 
> Huh, does the `{}` send a single click to the input?  Looks like it must,
> but you wouldn't know that just from looking at it.

Yeah, it does. I hate the arguments of this function, especially since the second is non-optional, even without the third.

> > ```
> > +  synthesizeMouseAtCenter(menulist, {}, iframeWindow);
> > +  await BrowserTestUtils.waitForEvent(menupopup, "popupshown");
> > ```
> 
> A general question: In cases like this, is it guaranteed that the popupshown
> event won't fire before we are awaiting for it to fire?  Or could it fire
> really quickly after the click before we're waiting on it? Or...
> 
> > ```
> > +  let hiddenPromise = BrowserTestUtils.waitForEvent(menupopup, "popuphidden");
> > +  menupopup.hidePopup();
> > +  await hiddenPromise;
> > ```
> 
> ...or should we use this approach of setting up the promise first, then
> awaiting it after doing the thing that causes it to resolve?

In the first case, we know the event won't happen until after the listener is added. The `mouseup` and `mousedown` events created by `synthesizeMouseAtCenter` are added to the end of a list of things to do. We're currently somewhere higher up the list, so this code will keep executing (and add the listener) until it runs out of things to do (in this case when it waits for a Promise that didn't immediately resolve) then do the next thing on the list.

Incidentally this is exactly what setTimeout with no (or 0) timeout does: it puts the callback on the end of the list. So we can use it to wait for things to happen, like custom elements or event handlers doing things.

In the second case, calling `hidePopup` fires the `popuphidden` event before returning. I'm pretty sure we don't need to wait for the event, but I've done it now. (I think at some point I was closing this menu pop-up a different way then got frustrated and did it this way.)

> > ```
> > +  if (!BrowserTestUtils.is_visible(label)) {
> > +    menuitem.click();
> > ```
> 
> .click() here instead of synthesizeMouseAtCenter ?  Probably fine, just
> curious since elsewhere you're using the latter to send clicks.

I usually consider this to be cheating but it works. This menu item is part of the window's main menu which means it's a native menu item on Mac OS and we can't fire click events at it.

> > ```
> > +  await new Promise(resolve => iframeWindow.setTimeout(resolve, 500));
> > ```
> 
> Hm, I thought eslint was outlawing such setTimeouts now?

(I only remembered this was there after submitting the patch. I can't for the life of me work out why it's necessary but *something* is going on here.) I got away with it because this of where the file is. The path doesn't match any of the patterns for XPCShell or Mochitest files. TBH I'd probably just silence the linter warning here anyway. ;-)

Back to Bug 1590682 Comment 13