583379 - Can't get dynamic access to menus with an 'allowevents' attribute set to false

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrew Halberstadt [:ahal]]

We got around the dynamic menu access problem for most menus by sending a fake 'popupshowing' event to the menupopup elements (see Bug 474486).  Unfortunately some menus, such as the 'History' menu, have an 'allowevents' attribute set to false which causes cryptic asserts.

We'll either need a way to send events to these menus without causing asserts, or find a completely different solution to the dynamic menu problem that doesn't involve sending fake events.

Comment 1

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

Marco, Dietrich, or Mano, has one of you an idea why that particular faked event does not work for menupopups with allowevents set to false? What alse can we do to let the menu populate with dynamic entries?

Comment 2

[cmtalbert]

I'd take a look at a patch that implements this, but I'd not hold 2.0 for it.  We've gotten by this long...

Comment 3

[Balazs Juhasz (:balazs)]

This issue blocking bug 474313

Comment 4

[Balazs Juhasz (:balazs)]

In bug 474313 we want to verify if a Bookmark was really created in Bookmarks menu by clicking on it.

The dynamic menu is generated by calling:
  controller.mainMenu.open("#bookmarksMenu");

which would call FakeOpenPopup method:
  https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L224


This method is based on
  http://hg.mozilla.org/mozilla-central/rev/d70ef56fdca6

Unfortunately, popup.allowevents always return with "false" value, so the fake clicking never called - the dynamic menu is not generated.

Marco: do you have any idea what could be the problem?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

off-hand, looks like mozmill is checking allowevents on a menupopup, why does that check exist?
If you read https://developer.mozilla.org/en-US/docs/XUL/Attribute/allowevents you see that in menus the behavior is special, it is usually false but may be set to true on a descendant, and in such a case the event is consumed by the descendant (if I read that correctly). So it seems like a miscomprehension about what the attribute is doing?

Comment 6

[Andrew Halberstadt [:ahal]]

It was added from https://bugzilla.mozilla.org/show_bug.cgi?id=474486#c35

Don't ask me to remember what either of these bugs are about though, I only have a vague recollection of them :). I'd say don't be afraid of changing things around.

Comment 7

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

Sounds good. Balazs, could you try without this if condition? Does it make a difference for you? Do dynamically populated sub menus work then?

Comment 8

[Balazs Juhasz (:balazs)]

I tried removing checking of popup.allowevents

The dynamic menu populated now and the new bookmark is working using:
  controller.mainMenu.open("#bookmarksMenu");
  controller.mainMenu.click("[label='Mozilla']");

Comment 9

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

That are great news. Would you mind to also do a quick spotcheck for the history menu?

Comment 10

[Balazs Juhasz (:balazs)]

Calling of controller.mainMenu.open("#history-menu"); does not give any error.

Moreover, from my experience it is not matter #bookmarksMenu, #history-menu is given as argument, because the entire menu is re-populated by _buildMenu.

For example, the new bookmark is clickable also if #history-menu is called instead of #bookmarksMenu:
  controller.mainMenu.open("#history-menu");
  controller.mainMenu.click("[label='Mozilla']");

Comment 11

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

Sorry, but my question wasn't asked well enough. Can you please check if you select a dynamic menuitem from the history menu, i.e. a formerly closed tab. Also for your example above, the open call should not be necessary.

Comment 12

[Balazs Juhasz (:balazs)]

Attachment: Remove allowevents checking. Add dynamic menu generation in menu:getItem.

Yes, selecting a dynamic menuitem from history menu also works.

I created a patch for mozmill2/controller.js which:
  - remove checking of allowevents attribute
  - calls menu:_buildmenu automatically when menu:getItem called to get dynamic menu entries

With this modification it is possible to directly click on a dynamic menu entry.

Comment 13

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

Comment on attachment 819388 [details] [diff] [review]
Remove allowevents checking. Add dynamic menu generation in menu:getItem.

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

That's great Balazs! I like that we can remove this if condition. Beside the nit I have mentioned inline can you please add a Mutt test, which exercises this path for Firefox?

r=me already for the patch.

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +147,5 @@
>     * @throws Error If menu element has not been found
>     */
>    getItem: function Menu_getItem(itemSelector) {
> +    // Run through the entire menu and populate with dynamic entries
> +    this._buildMenu(this._menu.getNode());   

nit: please remove the trailing white-spaces.

Comment 14

[Balazs Juhasz (:balazs)]

Attachment: Remove allowevents checking. Add dynamic menu generation in menu:getItem. v2

Trailing white-spaces removed.

Comment 15

[Balazs Juhasz (:balazs)]

For Mutt test I will work on. I think about a test which opens a local page, and then checks if the history-menu contains the new menu entry via mainMenu.getItem('label').

Comment 16

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

That sounds like the right approach. Please combine the test patch with the existent one.

Comment 17

[Balazs Juhasz (:balazs)]

Attachment: Remove allowevents checking. Add dynamic menu generation in menu:getItem. Add new Mutt test.

Combined previous patch with new Mutt test patch.

Comment 18

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

Comment on attachment 822493 [details] [diff] [review]
Remove allowevents checking. Add dynamic menu generation in menu:getItem. Add new Mutt test.

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

Absolutely nothing I have to comment on. That looks great and I will get it landed in a bit. Thanks Balazs!

Comment 19

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

Landed on master for Mozmill 2.1:
https://github.com/mozilla/mozmill/compare/4b5218b01c44...e89223e10a8f

Comment 20

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

Given that this is actually a bug fix I should have it ported back to our hotfix-2.0 branch. I did that now and the fix will be part of the upcoming Mozmill 2.0.1.

https://github.com/mozilla/mozmill/commit/60be401b877f321e47d0950a6b88a413ad4dfb9c