597330 - Make Menu API from controller.js available for Mozmill tests and add support for context menus

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The Menu API inside of controller.js is only available for the controller itself. Tests should also be able to benefit from this class. It's possible to use it for each menu and context menu.

Would be great to have it for 1.5.1. IMO we should move it to utils.js.

Comment 1

[cmtalbert]

We just have to add the menu API to the export list on the controller.js function.  That's all we need here?

Anyone for an easy patch?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right. Nothing more to do here with a big benefit for us!

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Well, "all we need" should include the documentation update on MDC too since it'll now be a public API.  Sounds like it's totally worth it though!

Comment 4

[Aaron Train [:aaronmt]]

Is this all that's required, this fantastically enormous change?

var EXPORTED_SYMBOLS = ["MozMillController", "waitForEval", "MozMillAsyncTest",
-                        "globalEventRegistry", "sleep"];
+                        "globalEventRegistry", "sleep", "Menu"];

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, but eventually it would better live under utils.js and needs at least one simple test? Clint or Heather?

Comment 6

[Heather Arthur [:harth]]

(In reply to comment #5)
> Yes, but eventually it would better live under utils.js and needs at least one
> simple test? Clint or Heather?

Yeah, given that you have to provide the window anyways to use it:

  new Menu(aWindow, popup.childNodes)

I think it's probably best to put it on the mozmill object or change the API so that it's on the controller and doesn't need the window argument.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> I think it's probably best to put it on the mozmill object or change the API so
> that it's on the controller and doesn't need the window argument.

So what about something like this:

http://github.com/whimboo/mozmill-panorama/blob/master/shared-modules/testUtilsAPI.js#L152

Comment 8

[Heather Arthur [:harth]]

How about:

var menu = new controller.Menu(popup);
var item = menu["File"]["Save As"];

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The biggest problem we have seen in the past months is the fact that when using context menu popups, the menus sometimes don't close after a click. It would be a huge improvements when the menu class could take care of that particular issue. The example class I have created for Panorama shows that.

I could come up with a proposal but I would need feedback what you want for Mozmill 1.5.1. Should it simply offer the Menu API or do we want to implement a complete handling of menus? The complete patch is probably too invasive and will break existing tests, and should probably moved out to Mozmill 2.0.

Comment 10

[Heather Arthur [:harth]]

(In reply to comment #9)
> I could come up with a proposal but I would need feedback what you want for
> Mozmill 1.5.1. Should it simply offer the Menu API or do we want to implement a
> complete handling of menus?

I'd love to see it, you probably know best what we want. I just think it should be off of the controller object instead of passing it (or the window) as a parameter.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> complete handling of menus? The complete patch is probably too invasive and
> will break existing tests, and should probably moved out to Mozmill 2.0.

My last comment was totally wrong. We can't break any of the existing tests because we haven't exposed the Menu API so far. Heather, I will try to get something together by tomorrow.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This patch implements a new way in using the different menus in Gecko based applications. It's not only able to interact with the main menu but also with context menus. Member functions make it easier to trigger certain actions for menu items. At the end it's much easier to use and more reliable compared to the old class, which has been marked as deprecated now.

I have implemented the Menu class as an independent class because it's not related to the controller. Both classes only make use of some of the others functions. I would love it when you wouldn't have to specify the controller in the constructor of Menu, but creating a new one inside the constructor based on the document we fold in isn't probably the best idea. 

Finally it's not a simple function anymore. So no idea yet, how it could be part of MozMillController.

Comment 13

[Heather Arthur [:harth]]

(In reply to comment #12)
> I have implemented the Menu class as an independent class because it's not
> related to the controller. Both classes only make use of some of the others
> functions.

I disagree, you always pass the controller in to Menu, so each Menu instance is linked to a particular controller, why have a level of indirection with getMenu() when you'll never use Menu without a controller? I also don't think it fits in with the other exports of the controller module, it's not actually independent of the controller like sleep and waitFor.

> Finally it's not a simple function anymore. So no idea yet, how it could be
> part of MozMillController.

var menu = new controller.Menu(doc, '#my-popup');

Comment 14

[Heather Arthur [:harth]]

(In reply to comment #13) 
> I disagree, you always pass the controller in to Menu, so each Menu instance is
> linked to a particular controller, why have a level of indirection with
> getMenu() when you'll never use Menu without a controller? I also don't think
> it fits in with the other exports of the controller module, it's not actually
> independent of the controller like sleep and waitFor.

I take this back, if you put it on the controller's prototype then there's no way to access the controller object from within the Menu class ('this' will be the Menu object), so it does have to be passed in the controller.

Comment 15

[Heather Arthur [:harth]]

Comment on attachment 482943 [details] [diff] [review]
Patch v1


> var EXPORTED_SYMBOLS = ["MozMillController", "waitForEval", "MozMillAsyncTest",
>-                        "globalEventRegistry", "sleep"];
>+                        "globalEventRegistry", "sleep", "Menu"];

We talked about this, there's no reason to export Menu.

>+var Menu = function(aController, aDocument, aMenuSelector) {

You could put the menuSelector before the document and make the doc an optional argument that defaults to the controller's window's document.

>+    node = node.wrappedJSObject ? node.wrappedJSObject : node;

more succinctly:

node = node.wrappedJSObject || node;


>+  open : function(aContextParentElement) {

Not a big deal, but aContextElement or contextElement would probably do the trick here. The arcane Mozilla "a" syntax isn't used anywhere else in controller.js so I'd be inclined not to use it for any of this either.

>+    // We have to open the context menu
>+    var menu = this._menu.getNode();
>+    if (menu.localName == "menupopup" &&
>+        aContextParentElement && aContextParentElement.getNode()) {
>+      this._controller.rightClick(aContextParentElement);
>+      this._controller.waitFor(function() {
>+        return menu.state == "open";
>+      }, 5000, 100, "Context menu has been opened.");
>+    }
>+
>+    // Run through the entire menu and populate with dynamic entries
>+    this._buildMenu(this._controller, this._menu.getNode());
>+

didn't you save this._menu.getNode() in a var called menu earlier?

>+    Array.forEach(items, function(item) {

items.forEach(function(item) {} , this);

does that not work?


This seems like a good API for right now, better than what we have, r+ with the nit-picks.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #15)
> Not a big deal, but aContextElement or contextElement would probably do the
> trick here. The arcane Mozilla "a" syntax isn't used anywhere else in
> controller.js so I'd be inclined not to use it for any of this either.

We started to use it in waitForPageLoad. Clint wanted to have it that way. So I would proceed and we should update our functions to use that style.

> >+    // Run through the entire menu and populate with dynamic entries
> >+    this._buildMenu(this._controller, this._menu.getNode());
> >+
> 
> didn't you save this._menu.getNode() in a var called menu earlier?

yeah, good catch.

> >+    Array.forEach(items, function(item) {
> 
> items.forEach(function(item) {} , this);
> 
> does that not work?

No it doesn't because items is a node list and not an array. I already ran into that problem while working on the nodeCollector for our tests.

Comment 17

[Heather Arthur [:harth]]

(In reply to comment #16)
> We started to use it in waitForPageLoad. Clint wanted to have it that way. So I
> would proceed and we should update our functions to use that style.
> 

update from Clint is that he doesn't care. I'd still prefer to go with the style of the current
file which is no "a".

> No it doesn't because items is a node list and not an array. I already ran into
> that problem while working on the nodeCollector for our tests.

Ah, I see, cool.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #15)
> >+var Menu = function(aController, aDocument, aMenuSelector) {
> 
> You could put the menuSelector before the document and make the doc an optional
> argument that defaults to the controller's window's document.

That makes a lot of sense. I would say in 99% of all cases we will have controller.window.document here. Thanks for that hint. It makes it much cleaner.

(In reply to comment #17)
> update from Clint is that he doesn't care. I'd still prefer to go with the
> style of the current
> file which is no "a".

Ok, updated.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As Heather asked me on IRC yesterday about handling menuitems which can only be accessed via its label, here the solution.

This example searches for a bookmark with the name "mozmill" under the bookmarks menu:

controller.mainMenu.click('#bookmarksMenuPopup menuitem[label="mozmill"]');

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Final version of the patch with review comments addressed. Taking over r+.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on the hotfix-1.5.1 branch:

http://github.com/mozautomation/mozmill/commit/c561ee2527ca694fa0f6ec8f080855b377f90760

Comment 22

[Aaron Train [:aaronmt]]

Verified fixed with 1.5.1rc1

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master as:
http://github.com/mozautomation/mozmill/commit/ba8fdcadd471a34e10b0fdbbfcba4457d64e3886

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Did docs on the menu API land anywhere so people know how to use this?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Heather and Clint, I have a hard time to figure out where the docs should be placed on MDN. Reason is that we don't have a page for controller.js but handle the MozMillController object on its own page. Where should I put the docs for the Menu API?

Comment 26

[u279076]

Has this been documented? It's needed for bug 604327.