All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: ahal, Assigned: balazs)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Details

(Whiteboard: [mozmill-2.0.1][mentor=whimboo][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
See Also: → bug 474486
Whiteboard: [mozmill-2.0?]
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?
Depends on: 474486

Comment 2

8 years ago
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...
Whiteboard: [mozmill-2.0?] → [mozmill-2.0-wanted]
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill-2.0-wanted] → [mozmill-2.0-][mozmill-next?]
(Assignee)

Comment 3

5 years ago
This issue blocking bug 474313
(Assignee)

Updated

5 years ago
Flags: needinfo?(mak77)
Blocks: 474313
(Assignee)

Comment 4

5 years ago
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?
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?
Flags: needinfo?(mak77)
(Reporter)

Comment 6

5 years ago
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.
Sounds good. Balazs, could you try without this if condition? Does it make a difference for you? Do dynamically populated sub menus work then?
(Assignee)

Comment 8

5 years ago
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']");
That are great news. Would you mind to also do a quick spotcheck for the history menu?
(Assignee)

Comment 10

5 years ago
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']");
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.
(Assignee)

Comment 12

5 years ago
Created attachment 819388 [details] [diff] [review]
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.
Attachment #819388 - Flags: review?(hskupin)
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.
Attachment #819388 - Flags: review?(hskupin) → review+
Assignee: nobody → juhaszbal
Status: NEW → ASSIGNED
Whiteboard: [mozmill-2.0-][mozmill-next?] → [mozmill-2.0-][mozmill-2.1?][mentor=whimboo][lang=js]
(Assignee)

Comment 14

5 years ago
Created attachment 821984 [details] [diff] [review]
Remove allowevents checking. Add dynamic menu generation in menu:getItem. v2

Trailing white-spaces removed.
Attachment #819388 - Attachment is obsolete: true
Attachment #821984 - Flags: review?(hskupin)
(Assignee)

Comment 15

5 years ago
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').
That sounds like the right approach. Please combine the test patch with the existent one.
Attachment #821984 - Flags: review?(hskupin)
(Assignee)

Comment 17

5 years ago
Created attachment 822493 [details] [diff] [review]
Remove allowevents checking. Add dynamic menu generation in menu:getItem. Add new Mutt test.

Combined previous patch with new Mutt test patch.
Attachment #821984 - Attachment is obsolete: true
Attachment #822493 - Flags: review?(hskupin)
Blocks: 875451
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!
Attachment #822493 - Flags: review?(hskupin) → review+
Landed on master for Mozmill 2.1:
https://github.com/mozilla/mozmill/compare/4b5218b01c44...e89223e10a8f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0-][mozmill-2.1?][mentor=whimboo][lang=js] → [mozmill-2.0-][mozmill-2.1][mentor=whimboo][lang=js]
Whiteboard: [mozmill-2.0-][mozmill-2.1][mentor=whimboo][lang=js] → [mozmill-2.1][mentor=whimboo][lang=js]
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
Blocks: 931828
Whiteboard: [mozmill-2.1][mentor=whimboo][lang=js] → [mozmill-2.0.1][mentor=whimboo][lang=js]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.