940902 - UITour: Add ability to hide a menu (complements showMenu)

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P2)

Description

[Matthew N. [:MattN]]

Since we have showMenu, it makes sense to have hideMenu, especially after bug 936187 makes the panel menu sticky.

Comment 1

[Matthew N. [:MattN]]

Attachment: Pointer to Github pull request: https://github.com/Unfocused/mozilla-uitour/pull/1

Pointer to Github pull-request

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Add hideMenu (with tests)

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8335188 [details] [diff] [review]
v.1 Add hideMenu (with tests)

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

::: browser/modules/test/browser_UITour.js
@@ +239,5 @@
> +    let bookmarksMenuButton = document.getElementById("bookmarks-menu-button");
> +    ise(bookmarksMenuButton.open, false, "Menu should initially be closed");
> +
> +    gContentAPI.showMenu("bookmarks");
> +    ise(bookmarksMenuButton.open, true, "Menu should be shown after showMenu()");

Is this reliable? Opening/closing a menu/panel can be async typically. Or does using nsIMenuBoxObject.openMenu() force it to be synchronous?

(And if that is the case, maybe we shouldn't be using it.)

Comment 4

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #3)
> Review of attachment 8335188 [details] [diff] [review]:
> 
> ::: browser/modules/test/browser_UITour.js
> @@ +239,5 @@
> > +    let bookmarksMenuButton = document.getElementById("bookmarks-menu-button");
> > +    ise(bookmarksMenuButton.open, false, "Menu should initially be closed");
> > +
> > +    gContentAPI.showMenu("bookmarks");
> > +    ise(bookmarksMenuButton.open, true, "Menu should be shown after showMenu()");
> 
> Is this reliable? Opening/closing a menu/panel can be async typically. Or
> does using nsIMenuBoxObject.openMenu() force it to be synchronous?
> 
> (And if that is the case, maybe we shouldn't be using it.)

nsMenuBoxObject::OpenMenu calls either nsXULPopupManager::ShowMenu or nsXULPopupManager::HidePopup with the third and fourth arguments respectively set to false (for async).

Are you worried about janking the browser? If so, that would also happen when users go through other code paths which call OpenMenu.

It's unclear how to proceed.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8335188 [details] [diff] [review]
v.1 Add hideMenu (with tests)

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

Oy. Ok, lets sort that out in a separate followup bug then. Don't think it needs to be a priority, but it's something we should probably fix at some stage.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8335188 [details] [diff] [review]
v.1 Add hideMenu (with tests)

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

*ahem*

Comment 7

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/f7e4f618f9f7

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7e4f618f9f7

Comment 9

[Mike Conley (:mconley) (:⚙️)]

This was backed out on Holly due to introducing an orange (bug 944485).

Backout was https://hg.mozilla.org/projects/holly/rev/f296b665cdb5