474486 - Dynamic (realtime) menu access needed for MenuAPI

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090117 Shiretoko/3.1b3pre Ubiquity/0.1.4 ID:20090117020415

As noted by Mikeal, the current implementation requires us to re-create the controller to have an up2date version of the menu available. It would be great if we can observe any menu changes and recreate the menu hierarchy on the fly.

Comment 1

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

Sadly this is one of the problems I ran into the last days again and again. Mikeal, do you have any estimate on this topic?

Comment 2

[Mikeal Rogers [:mikeal] (mrogers@)]

It would be trivial to get the Menu API object to use a getter to access the element instead of the static creation it does now, but we found that this isn't the only issue.

There seems to be some event that has to fire before the menu elements are updated. So in the case you need, adding a bookmark, the menu elements aren't updated with the new element until you click on the menus in the OS. We need to track down something we can call that will cause the same refresh of the menu elements, I'm at a loss as to how we can figure this out because regular window listeners won't cut it.

Comment 3

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

The only workaround I have here is to directly use the nsINavBookmarksService for the add bookmarks test. Other tests which would rely on an updated menu could not be written. Right now I don't have one in mind => P1.

Comment 4

[Mikeal Rogers [:mikeal] (mrogers@)]

I've added a reload() method to the Menu API. This will traverse back down the base nodes to refresh the menu API.

Keep in mind this will only work if the underlying menu elements have been updated.

Fixed in r441.

Comment 5

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

I tried to change my test to use the new method and was half-wise successful. It seems to work unreliable. Sometimes the click is successful but most of the times I get an error: "could not find element Elem instance". See the attached test.

Comment 6

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

Attachment: MozMill test

Comment 7

[Mikeal Rogers [:mikeal] (mrogers@)]

Yeah, this is because the menu elements don't get updated immediately.

If you have multiple windows open i bet switching focus from one window to the next would fix this. I wonder if a focus() call forces update.

whimboo; could you try running controller.window.focus() before controller.menus.reload() ?

Comment 8

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

I did but it doesn't help. It's even not enough to reopen the browser window or the Mozmill window. I have to restart the browser to get it to work.

Further I have seen the following error now directly when accessing the menu for the first time (click on bookmarks>bookmark this page):

fail :: controller.menus is undefined

Comment 9

[Mikeal Rogers [:mikeal] (mrogers@)]

All I can do on the API side is iterate over the menu elements, if those aren't updated there isn't much I can do without knowing how to force them to update.

Don't know why controller.menus undefined, that's incredibly odd, is this a consistent repro?

Comment 10

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

Mikeal, I know why that cannot work. The current implementation is wrong. You always rely on the old nodes in the menu and never catch new or deleted items. In the former case we fail when accessing the menu item and in the latter case we get "controller.menus is undefined" because we are trying to access a node which doesn't exist anymore.

Check those lines:
http://code.google.com/p/mozmill/source/browse/trunk/mozmill/extension/resource/modules/controller.js?spec=svn441&r=441#146

The first statement in this function "var elements = this.nodes;" retrieves the old nodes and operates on them. Instead of doing this we have to fetch the current childNodes of the menubar. Mikeal, I believe that should be an easy fix.

Comment 11

[Mikeal Rogers [:mikeal] (mrogers@)]

That's just the array of top level menu items, which never change.

Later in the code we get the childNodes for all the sub-menus which are the ones that should change.

Comment 12

[Mikeal Rogers [:mikeal] (mrogers@)]

I just had to add this function back to trunk because adam accidentally removed it in another big merge he did.

When I added it back I removed the caching of the top level menu set in cases that was causing any kind of problem.

Comment 13

[Mikeal Rogers [:mikeal] (mrogers@)]

Is this fixed?

The fix that I had checked in was accidentally removed due to a merge conflict. It should be back now, is this still valid?

Comment 14

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

Yes, still valid. The problem we have here is that the menu is rebuild when a user clicks on the Bookmarks menu. Only then newly added bookmarks will be available when run a menu click against them. So how could we trigger the onpopupShowing event?


http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#520

Comment 15

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

Mikeal, we can use the following way to update the menu:

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#210

I have tested the following snippet in my add Bookmark to Bookmarks Menu test and it works perfectly:

var utils = controller.window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
            getInterface(Components.interfaces.nsIDOMWindowUtils);
utils.forceUpdateNativeMenuAt("4");
utils.activateNativeMenuItemAt("4|10");

Comment 16

[Mikeal Rogers [:mikeal] (mrogers@)]

Fixed in r522.

Comment 17

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

Mikeal, this was just an example. The code above only updates the 11th element in the bookmarks menu. So having it work for all menus and their sub menus we should better iterate through the menu items. We should back this out for now and try to fix it for 1.3?

Comment 18

[Mikeal Rogers [:mikeal] (mrogers@)]

So, if I use this to update a top level menu it doesn't cascade down to the corresponding submenus?

This sound like OOTB we still can't get this to update newly created submenus unless we know what their position should already be?

Comment 19

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

(In reply to comment #18)
> So, if I use this to update a top level menu it doesn't cascade down to the
> corresponding submenus?

Correct.

> This sound like OOTB we still can't get this to update newly created submenus
> unless we know what their position should already be?

Right. So probably this way is not the best. As we talked on IRC yesterday we would also need to fire the onpopupshown event for each menu/submenu click. Right now we directly click the target menuitem and don't fire clicks against the menu/submenu items. Some of the menus like the bookmarks menu update its content when clicking on them. I believe that would require a complete rewrite of the menu API and how we handle controller.click. Eventually we should make it a member function of the menu API which could take care of those extra steps?

So we can call it like 

var elem = new elementslib.Elem(controller.menus.bookmarksMenu.menu_bookmarkThisPage);
controller.menus.click(controller.window.document, elem);

Comment 20

[Mikeal Rogers [:mikeal] (mrogers@)]

Punting to 1.3

Comment 21

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

Today I got some really helpful information from Marco how to make sure we can also include dynamic content in our menu hierarchy. We should simply fake a popupshowing event:

+function fakeOpenPopup(aPopup) {
+  var popupEvent = document.createEvent("MouseEvent");
+  popupEvent.initMouseEvent("popupshowing", true, true, window, 0,
+                            0, 0, 0, 0, false, false, false, false,
+                            0, null);
+  aPopup.dispatchEvent(popupEvent);  
+}

Example:
https://bug544817.bugzilla.mozilla.org/attachment.cgi?id=454488

We would have to do that when we reach a node with the tagName "menu". Sending the event should call the appropriate onpopupshowing callback directly.

Clint, it doesn't look that hard to implement. Can we also get this in for 1.4.2? It would help us in a lot of places where we are blocked writing tests at the moment.

Comment 22

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

Attachment: Patch v1

Finally this patch allows us to have access to dynamic menu items.

Comment 23

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

Attachment: Patch v1 (corrected)

This time with the diff in the correct direction.

Comment 24

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

(In reply to comment #21)
> Today I got some really helpful information from Marco how to make sure we can
> also include dynamic content in our menu hierarchy. We should simply fake a
> popupshowing event:

Marco, when I run the doCommand or initiate a normal mouse click (controller.click) on elements like "View | Toolbars | Bookmarks Toolbar" the action behind is not executed. Means the bookmarks toolbar is not toggled and the menu item has the same check state. Is something more necessary to make this happen?

Comment 25

[Heather Arthur [:harth]]

commented on http://github.com/whimboo/mozmill/commit/cfabd9ad91f5777fc61910baccd8c48620bec5cd

r+ if you move fakeOpenPopup to events module instead of utils.

Comment 26

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

Changes have been made. And patch has been pushed to master and 1.4.2.

master: http://github.com/mozautomation/mozmill/commit/74a6844ad159a5d53cf9739bf2d4454749fad4e7
1.4.2: http://github.com/mozautomation/mozmill/commit/855b364fdeb9ccea8b2feb004a3180ea37291fb5

Comment 27

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

Not fully solved. Sometimes I get assertions in a modal dialog which blocks the execution of Mozmill tests.

Comment 28

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

Assertions I get when running the add bookmark test:

ASSERT: node must have _DOMElement set
Stack Trace: 
0:PMV__nodeTitleChanged([object ResultNodeClassInfo],Mozilla.org - Home of the Mozilla Project)

ASSERT: node must have _DOMElement set
Stack Trace: 
0:PMV_nodeIconChanged([object ResultNodeClassInfo])

Comment 29

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

Mak, do you have an idea why that can happen? It only occurs when I run a faked click against all the available menu entries before I work with places.

Comment 30

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

The problem is the history menu bar item. Synthesizing a popupshowing event will result in that assertion. It's the only menu node which specifies an oncommand handler. In that case it wants to access event.target.node. But that one seems to be null.

Comment 31

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

Backed-out on 1.4.2 for now:
http://github.com/mozautomation/mozmill/commit/97a10eba999e7243c8f93cad349fd8896f7d7630

Comment 32

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

Marco, have you had a change yet to check this bug?

Comment 33

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

Comment on attachment 455102 [details] [diff] [review]
Patch v1 (corrected)

Sadly this patch doesn't really work for Mozmill. As given in the last comments a couple of assertions are shown. I don't know why that happens. There seem to be too many factors here. Marco, could you please give feedback? That would be appreciated. If you would need an XPI i should be able to create one for your testing.

Comment 34

[Andrew Halberstadt [:ahal]]

Attachment: Testcase for reproducing the asserts

I still don't know what is causing the asserts, but I've managed to narrow it down a bit.

It seems to reproduce all you need to do is fire a 'popupshowing' event on the menupopups and then open a new page or refresh the current one as seen in the attached test case.  No matter what page you open afterward, you'll always see the same four assert messages.

My best guess is that by firing the popupshowing event without actually opening the popups, we are somehow messing up some of the menupopup properties. Then when the window onload event is fired, a check is performed and the assertions are thrown.

I tried immediately sending a 'popuphiding' event after the first event, but that didn't help.  I'll continue to investigate.

Comment 35

[Andrew Halberstadt [:ahal]]

Attachment: Incremental patch which fixes the asserts

This fixes the assert errors for me (note this patch should be applied to master and not the previous patch).

It seems that some of the menus had a 'allowevents' attribute set to false. Sending events to these menus is what was causing the asserts. By adding a check for this, the asserts disappear. 

The down side is that this means we won't be able to retrieve dynamic content from these specific menus (such as the 'History' menu). Regardless, most menus are unaffected and I think this could be checked in to 1.4.2 at least as a temporary solution.

Comment 36

[Heather Arthur [:harth]]

Comment on attachment 461422 [details] [diff] [review]
Incremental patch which fixes the asserts

looks good (:

Comment 37

[Andrew Halberstadt [:ahal]]

Applied this patch to master but not 1.4.2 as I don't have the whole patch.

http://github.com/mozautomation/mozmill/commit/1ad0ca6448ac8c6cf2f36b80130a7950be8eeb71

Comment 38

[cmtalbert]

What else needs to land on 1.4.2?

Comment 39

[Andrew Halberstadt [:ahal]]

The patch I just submitted was based on Henrik's patch which he backed out of 1.4.2 and I don't have that diff.

We've already talked and Henrik said he would check both combined patces into 1.4.2 when he had a chance.

Comment 40

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

Attachment: Combined patch for 1.4.2 branch

Comment 41

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

Comment on attachment 455102 [details] [diff] [review]
Patch v1 (corrected)

Lets remove the feedback from Marco, now that we mostly identified the problem.

Andrew, can you please file a new bug for the remaining issue, you have discovered? Thanks.

Comment 42

[Heather Arthur [:harth]]

Comment on attachment 461689 [details] [diff] [review]
Combined patch for 1.4.2 branch

looks good.

Comment 43

[Andrew Halberstadt [:ahal]]

This can be checked in and closed.

New bug: https://bugzilla.mozilla.org/show_bug.cgi?id=583379

Comment 44

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

Landed on 1.4.2:
http://github.com/whimboo/mozmill/commit/0a6b1d38e3a3fa374d1c1c43631f3442f1e93b9c

Comment 45

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

No assertions are thrown anymore with the checked-in follow-up patch. Looks like we got it! The remaining issue is covered on bug 583379.