Closed Bug 474486 Opened 12 years ago Closed 10 years ago

Dynamic (realtime) menu access needed for MenuAPI

Categories

(Testing Graveyard :: Mozmill, defect, P1)

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [mozmill-1.4.2+])

Attachments

(5 files, 1 obsolete file)

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.
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?
Severity: normal → major
Summary: Make the MenuAPI realtime aware → Dynamic (realtime) menu access needed for MenuAPI
Blocks: 474313
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.
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.
Priority: -- → P1
Whiteboard: [mozmill-1.2]
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file MozMill test
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() ?
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
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?
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.
Assignee: nobody → mrogers
Status: REOPENED → ASSIGNED
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.
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.
Depends on: 491227
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?
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
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");
Fixed in r522.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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);
Punting to 1.3
Whiteboard: [mozmill-1.2] → [mozmill-1.3]
Blocks: 504653
No longer blocks: 504653
Whiteboard: [mozmill-1.3]
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.
Assignee: mikeal.rogers → nobody
Status: REOPENED → NEW
Whiteboard: [mozmill-1.4.2?]
Attached patch Patch v1 (obsolete) — Splinter Review
Finally this patch allows us to have access to dynamic menu items.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #455101 - Flags: review?(harthur)
This time with the diff in the correct direction.
Attachment #455101 - Attachment is obsolete: true
Attachment #455102 - Flags: review?(harthur)
Attachment #455101 - Flags: review?(harthur)
(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?
commented on http://github.com/whimboo/mozmill/commit/cfabd9ad91f5777fc61910baccd8c48620bec5cd

r+ if you move fakeOpenPopup to events module instead of utils.
Attachment #455102 - Flags: review?(harthur) → review+
No longer depends on: 491227
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Not fully solved. Sometimes I get assertions in a modal dialog which blocks the execution of Mozmill tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [mozmill-1.4.2?] → [mozmill-1.4.2+]
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])
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.
Status: REOPENED → NEW
Status: NEW → ASSIGNED
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.
Marco, have you had a change yet to check this bug?
Whiteboard: [mozmill-1.4.2+] → [mozmill-1.4.2+][mozmill-2.0?]
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.
Attachment #455102 - Flags: feedback?(mak77)
Blocks: 544980
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.
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.
Attachment #461422 - Flags: review?(harthur)
Comment on attachment 461422 [details] [diff] [review]
Incremental patch which fixes the asserts

looks good (:
Attachment #461422 - Flags: review?(harthur) → review+
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
What else needs to land on 1.4.2?
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 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.
Attachment #455102 - Flags: feedback?(mak77)
Comment on attachment 461689 [details] [diff] [review]
Combined patch for 1.4.2 branch

looks good.
Attachment #461689 - Flags: review? → review+
This can be checked in and closed.

New bug: https://bugzilla.mozilla.org/show_bug.cgi?id=583379
Landed on 1.4.2:
http://github.com/whimboo/mozmill/commit/0a6b1d38e3a3fa374d1c1c43631f3442f1e93b9c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Blocks: 575250
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.
Blocks: 583379
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-1.4.2+][mozmill-2.0?] → [mozmill-1.4.2+]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.