1037433 - add platform support for menubar icons

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Florian Quèze [:florian]]

To implement the desired UI in bug 1037430 (see attachment 8453883 [details]), we need platform support for adding icons in the menubar.

We are planning to ship screensharing in Firefox 33. Markus, is there any chance we could get the platform bits added in the 33 cycle (or in the 34 cycle and uplifted to aurora)? If it's not possible, we will probably show on Mac the global indicator designed for the other platforms (bug 1037408), but it would look ugly on Mac under the menubar.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Do the other platforms (Windows and Linux) already have platform-specific support for this?  Do they need it?

Comment 2

[Florian Quèze [:florian]]

(In reply to Steven Michaud from comment #1)
> Do the other platforms (Windows and Linux) already have platform-specific
> support for this?  Do they need it?

We don't need this on the other platforms. For the other platform we are putting a floating window at the top of the screen; but that's not possible on Mac because of the menubar that's already at the top of the screen.

Comment 3

[André Reinald]

Should the icon behave like a menu or like a button ? In the former case that's something I have done before, and I'd likely find my way back. In the latter case, only the notification center does it since 10.7, and I'm uncertain if it's documented anywhere (but then Steven's talent at reverse engineering may help).

Comment 4

[Markus Stange [:mstange]]

Attachment: proof of concept

Assuming the answer to André's question is "a simple menu is sufficient", something like this should do the job. This patch hijacks the new tab button to put an icon into the system status bar (that's what the right side of the menu bar is called officially) for demonstration purposes. Clicking the icon will open a menu. The icon is determined by the list-style-image of the <menu> element that's passed to the standaloneNativeMenu.init method.

I have a few more questions:
 - Will there always be an open browser window as long as the icon is displayed? If not, the XUL menu probably needs to move to hiddenWindow.xul.
 - Are we expecting other uses of this API that may be active at the same time as screen sharing? This patch only allows a single icon, but we should probably do the right thing and allow multiple ones. That means we need a different API than in this patch.
 - Are we expecting the icon to change dynamically while it is in the status bar? I think at the moment we don't support dynamic updates to icons in native menus.

Also, HiDPI icons will look LoDPI due to bug 899334.

Comment 5

[Florian Quèze [:florian]]

(In reply to Markus Stange [:mstange] from comment #4)
> Created attachment 8455302 [details] [diff] [review]
> proof of concept

Thank you so much! This seems very promising.

> Assuming the answer to André's question is "a simple menu is sufficient",

I think we were initially hoping that the icon could act as a button when there's only one page using devices, and like a menu (showing the list of pages using devices) when there's more than one, but changing the UX to always behave like a menu shouldn't be a problem (I'll check with Philipp).

Can these icons have a tooltip?

Looking at the icons currently in my menubar, it seems they don't have tooltips, and the typically practice is to include a descriptive item as the first (disabled) menuitem. I guess we could do this.

> Clicking the icon will open a menu.

Is this going to fire a popupshowing event letting us update the menu when it's about to be shown, or should the menu be kept up to date all the time?

> I have a few more questions:
>  - Will there always be an open browser window as long as the icon is
> displayed? If not, the XUL menu probably needs to move to hiddenWindow.xul.

Yes, but we may still want to use hiddenWindow.xul, because we don't want to duplicate the icons if several pages in different browser windows use devices at the same time.

>  - Are we expecting other uses of this API that may be active at the same
> time as screen sharing? This patch only allows a single icon, but we should
> probably do the right thing and allow multiple ones. That means we need a
> different API than in this patch.

As shown on attachment 8453883 [details], we would like to have one icon for microphone sharing, one icon for camera sharing, and one icon for screensharing.

>  - Are we expecting the icon to change dynamically while it is in the status
> bar? I think at the moment we don't support dynamic updates to icons in
> native menus.

I think we will likely want to change the icon if the microphone is muted, but that's not part of the initial design, so as far as we are concerned, it would be fine to handle this case as a follow-up.

> Also, HiDPI icons will look LoDPI due to bug 899334.

Ok.

Comment 6

[Markus Stange [:mstange]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > Assuming the answer to André's question is "a simple menu is sufficient",
> 
> I think we were initially hoping that the icon could act as a button when
> there's only one page using devices, and like a menu (showing the list of
> pages using devices) when there's more than one, but changing the UX to
> always behave like a menu shouldn't be a problem (I'll check with Philipp).

Good.

> Can these icons have a tooltip?

Surprisingly, yes they can! -[NSStatusItem setToolTip:] is available and works as expected.
However, since having a tooltip doesn't seem to be the platform convention, maybe we shouldn't use one either.

> Looking at the icons currently in my menubar, it seems they don't have
> tooltips, and the typically practice is to include a descriptive item as the
> first (disabled) menuitem. I guess we could do this.

Sounds good to me.

> > Clicking the icon will open a menu.
> 
> Is this going to fire a popupshowing event letting us update the menu when
> it's about to be shown

Yes.

> > I have a few more questions:
> >  - Will there always be an open browser window as long as the icon is
> > displayed? If not, the XUL menu probably needs to move to hiddenWindow.xul.
> 
> Yes, but we may still want to use hiddenWindow.xul, because we don't want to
> duplicate the icons if several pages in different browser windows use
> devices at the same time.

That's true.

> >  - Are we expecting other uses of this API that may be active at the same
> > time as screen sharing? This patch only allows a single icon, but we should
> > probably do the right thing and allow multiple ones. That means we need a
> > different API than in this patch.
> 
> As shown on attachment 8453883 [details], we would like to have one icon for
> microphone sharing, one icon for camera sharing, and one icon for
> screensharing.

Right, somehow I missed that. OK, I'll think about a better API... maybe just

interface nsISystemStatusBar : nsISupports
{
  void addItem(in nsIDOMElement aDOMElement);
  void removeItem(in nsIDOMElement aDOMElement);
};

> >  - Are we expecting the icon to change dynamically while it is in the status
> > bar? I think at the moment we don't support dynamic updates to icons in
> > native menus.
> 
> I think we will likely want to change the icon if the microphone is muted,
> but that's not part of the initial design, so as far as we are concerned, it
> would be fine to handle this case as a follow-up.

OK great.

Comment 7

[Markus Stange [:mstange]]

Attachment: proof of concept v2

This uses nsISystemStatusBar as suggested in the previous comment. Please let me know if this works for you.

Comment 8

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #6)
> (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > Can these icons have a tooltip?
> 
> Surprisingly, yes they can! -[NSStatusItem setToolTip:] is available and
> works as expected.
> However, since having a tooltip doesn't seem to be the platform convention,
> maybe we shouldn't use one either.

FWIW, dropbox seems to use the tooltip (although that isn't necessarily a good enough justification to say we should do the same, if it's against platform convention)

Comment 9

[Florian Quèze [:florian]]

(In reply to Markus Stange [:mstange] from comment #7)

> This uses nsISystemStatusBar as suggested in the previous comment. Please
> let me know if this works for you.

I haven't tried the patch, only looked at the code, but I think it does what we need.

Random thought: You may want the nsISystemStatusBar API to expose either a way to have a list of all the current status bar items, or a way to remove all items even if the calling code doesn't have references to these DOM nodes anymore. But anyway, we don't need this for our use case, so that's really up to you.

Comment 10

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)

> I think we were initially hoping that the icon could act as a button when
> there's only one page using devices, and like a menu (showing the list of
> pages using devices) when there's more than one, but changing the UX to
> always behave like a menu shouldn't be a problem (I'll check with Philipp).

Philipp sent me a new UI proposal with only menus, and with a grayed out descriptive first item.

Markus, what are the odds of the patch here landing by the end of the week? I'm asking this to decide if I try to implement something for bug 1037430 before the Firefox 33 string freeze (Monday).

Comment 11

[Markus Stange [:mstange]]

I'm preparing my patches for review now, so if roc and Steven are available to do the reviews during the next 24 hours, I should be able to land my patches tomorrow.

Comment 12

[Markus Stange [:mstange]]

Attachment: part 1: Add the nsISystemStatusBar interface.

Comment 13

[Markus Stange [:mstange]]

Attachment: part 2: Add nsSystemStatusBarCocoa implementation of nsISystemStatusBar

Comment 14

[Markus Stange [:mstange]]

Attachment: part 3: Trigger icon loads for the root menu item of a standalone native menu.

For submenus this is called from nsMenuX::LoadSubMenu, but it currently isn't called for root nsMenuX instances.

Comment 15

[Markus Stange [:mstange]]

Attachment: part 4: Propagate menu icon updates in native Mac menus upwards.

Comment 16

[Markus Stange [:mstange]]

Attachment: part 5: Update the image of an NSStatusItem when the image of its corresponding NSMenuItem changes.

Comment 17

[Markus Stange [:mstange]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> >  - Are we expecting the icon to change dynamically while it is in the status
> > bar? I think at the moment we don't support dynamic updates to icons in
> > native menus.
> 
> I think we will likely want to change the icon if the microphone is muted,
> but that's not part of the initial design, so as far as we are concerned, it
> would be fine to handle this case as a follow-up.

It turns out this is already implemented for the "image" attribute, and nearly impossible to implement for the "list-style-image" CSS property. So you can either just use the image attribute, or, if you use CSS, trigger an icon update by doing
        statusBarMenuElement.setAttribute("image", "");
        statusBarMenuElement.removeAttribute("image");

Comment 18

[Florian Quèze [:florian]]

Great! Using an image attribute would be fine, unless we want to use different png files for normal and for hidpi screens, where CSS media queries are helpful.

Comment 19

[Markus Stange [:mstange]]

Heh, well, the problem with that is that you can only query for the devicePixelRatio of the window that contains your XUL menu, and that may be on a different screen than the menubar. Actually, if your menu is in the hidden window, this might even accidentally work as intended: The hidden window is never shown, so it's probably assigned to the primary screen (the one with the menu bar) and inherits that screen's devicePixelRatio. But I'm speculating here, you'd need to test it.

Comment 20

[Markus Stange [:mstange]]

Attachment: part 2: Add nsSystemStatusBarCocoa implementation of nsISystemStatusBar

needed to add #include "nsIDOMElement.h" to nsSystemStatusBarCocoa.mm

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Florian, Markus:  Do we have a test page to see these patches in action?

Comment 22

[Florian Quèze [:florian]]

(In reply to Steven Michaud from comment #21)
> Florian, Markus:  Do we have a test page to see these patches in action?

I haven't written the patch for bug 1037430 yet, so a test page using screen sharing wouldn't help test the patches here.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #22)

OK, thanks.

Please do ping me here, though (even after this bug's patches have landed), when it becomes possible to test this new UI.  And please give me a test page then.  I'd like to try it in a bunch of different configurations, to see that it works properly.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

These patches look fine to me, Markus.  Though I'd like to test them when that becomes possible.

I do have one nit, though (for which see below).

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8457874 [details] [diff] [review]
part 4: Propagate menu icon updates in native Mac menus upwards.

+  void              IconUpdated() MOZ_OVERRIDE { mParent->IconUpdated(); }

Shouldn't you null-check mParent?

Comment 26

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #25)
> Comment on attachment 8457874 [details] [diff] [review]
> part 4: Propagate menu icon updates in native Mac menus upwards.
> 
> +  void              IconUpdated() MOZ_OVERRIDE { mParent->IconUpdated(); }
> 
> Shouldn't you null-check mParent?

Well, mParent is is guaranteed to be non-null as soon as nsMenuX::Create has been called, and every instantiation of nsMenuX is immediately followed by a Create call, so I think I don't need to null-check. This probably means that nsMenuX::Create should go away and instead by replaced by a proper constructor. I'll file a follow-up bug about that.

(In reply to Steven Michaud from comment #21)
> Florian, Markus:  Do we have a test page to see these patches in action?

Sorry, I really should know by now that I need to supply proper testing instructions when I ask you for reviews. My bad.

You can test these patches using the browser console (Tools -> Web Developer -> Browser Console). You'll need to flip devtools.chrome.enabled to true in about:config so you'll get a text box at the bottom of the console window. From this text box, execute these commands, for example:

var systemStatusBar = Cc["@mozilla.org/widget/macsystemstatusbar;1"].getService(Ci.nsISystemStatusBar)
var menu1 = document.querySelector("#bookmarksToolbarFolderMenu")
var menu2 = document.querySelector("#bookmarksToolbarFolderMenu ~ menu")
systemStatusBar.addItem(menu1)
systemStatusBar.addItem(menu2)

Here, menu1 and menu2 are taken from the existing Bookmarks menu in the menu bar. (menu1 and menu2 are the first two items that have icons and a submenu.)

Comment 27

[Steven Michaud [:smichaud] (Retired)]

> Well, mParent is is guaranteed to be non-null as soon as
> nsMenuX::Create has been called, and every instantiation of nsMenuX
> is immediately followed by a Create call, so I think I don't need to
> null-check. This probably means that nsMenuX::Create should go away
> and instead by replaced by a proper constructor. I'll file a
> follow-up bug about that.

Sounds reasonable.

And thanks for your instructions for testing.  Turns out I won't be
able to get to them before tomorrow.  Feel free to land these patches
before I've done so.  If I find any problems testing I'll report them
here, and we can fix them later.

Comment 28

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7723b9fc8d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa9065c5da1
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a1e00bc476
https://hg.mozilla.org/integration/mozilla-inbound/rev/970d8f4b1d14
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e306c8d5be

Comment 29

[Florian Quèze [:florian]]

Something that caused me some anxiety until I figured out how to work around it:

If I do:
  let menu = document.createElement("menu");
  menu.setAttribute("image", ...

  let menupopup = document.createElement("menupopup");
  menupopup.addEventListener("popupshowing", showinghandler);
  menu.appendChild(menupopup);

  this._statusBar.addItem(menu);
Then all the updates to the menupopup I do in showinghandler are ignored. It changes the DOM, but not the menu in the menubar.

If instead I do:
  let menu = document.createElement("menu");
  menu.setAttribute("image", ...

  this._statusBar.addItem(menu);

  let menupopup = document.createElement("menupopup");
  menupopup.addEventListener("popupshowing", showinghandler);
  menu.appendChild(menupopup);
Then the changes I do in showinghandler have a visible effect.


Also, while trying to figure out a workaround for this issue, I tried to force the menu to update by calling removeItem and then addItem from the popupshowing event handler. That never updated the menu, but that crashed Firefox half the time (I sent crash reports, but they were from my local build, so no symbols: bp-dc1ad05c-8239-4fcd-9538-b126d2140718 bp-b5219eb7-846d-4497-9346-97ff32140718).

Apart from this, the status icons seem to work as expected :-).

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Florian, please try your crash STR in an m-c nightly without these patches.  If you crash only with these patches, we may need to back them out until the crashes can be fixed.

And if you crash even without this bug's patches, do open a bug :-)  Be sure to include detailed STR.

Comment 31

[Florian Quèze [:florian]]

More surprises while implementing the patch for bug 1037430:
- the "command" event doesn't bubble from the menuitem to the menupopup; it's fired only on the menuitems
- the "command" event is fired after the "popuphiding" and "popuphidden" events, so if the menu is cleaned up in these hiding events, when "command" is received, the menuitem doesn't have a parent node anymore.

Both were easy to workaround, and I now have a working patch attached to bug 1037430.

Thanks again for the quick turnaround here! :-)

Comment 32

[Florian Quèze [:florian]]

(In reply to Steven Michaud from comment #30)
> Florian, please try your crash STR in an m-c nightly without these patches.

The crash is when doing something crazy with the API added in these patches, so trying a build without the patches will just result in a JS error.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c7723b9fc8d5
https://hg.mozilla.org/mozilla-central/rev/bfa9065c5da1
https://hg.mozilla.org/mozilla-central/rev/81a1e00bc476
https://hg.mozilla.org/mozilla-central/rev/970d8f4b1d14
https://hg.mozilla.org/mozilla-central/rev/a0e306c8d5be

Comment 34

[Markus Stange [:mstange]]

I'll look into the bugs you found next week, starting with the crash. Most of these issues are probably bugs that have already existed for menus in the menubar before.

Comment 35

[Markus Stange [:mstange]]

I wasn't able to reproduce the crash but I filed bugs for the other three issues: bug 1043421, bug 1043430, bug 1043435.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #27)

I finally got around to doing this test (as part of reviewing the patch for bug 1043421).  I found that I needed to use the following (otherwise I got an error):

var systemStatusBar = Cc["@mozilla.org/widget/macsystemstatusbar;1"].getService(Ci.nsISystemStatusBar)
var menu1 = document.querySelector("#bookmarksToolbarFolderMenu")
systemStatusBar.addItem(menu1)

I also discovered what I think is a bug:  The added menu item doesn't disappear when Firefox loses focus.  It only disappears when you quit Firefox.

I tested with today's m-c nightly on OS X 10.8.5.

Comment 37

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #36)
> (Following up comment #27)
> 
> I finally got around to doing this test (as part of reviewing the patch for
> bug 1043421).  I found that I needed to use the following (otherwise I got
> an error):
> 
> var systemStatusBar =
> Cc["@mozilla.org/widget/macsystemstatusbar;1"].getService(Ci.
> nsISystemStatusBar)
> var menu1 = document.querySelector("#bookmarksToolbarFolderMenu")
> systemStatusBar.addItem(menu1)

Oh. Not sure what my code was doing, I think it may have assumed that there's a bookmarks folder in a place where there isn't any now. But it's good that you were able to work around it.

> I also discovered what I think is a bug:  The added menu item doesn't
> disappear when Firefox loses focus.  It only disappears when you quit
> Firefox.

This is by design. I can't find a comment that clearly calls out this requirement, but the idea is that screen sharing continues while Firefox is not focused, and the user should know that it's happening.

Comment 38

[Florian Quèze [:florian]]

(In reply to Markus Stange [:mstange] from comment #37)

> > I also discovered what I think is a bug:  The added menu item doesn't
> > disappear when Firefox loses focus.  It only disappears when you quit
> > Firefox.
> 
> This is by design.

Yes, this is by design. Actually, the very reason why we want to have something in the menubar is to remind the user that something is being shared, even if the Firefox window isn't currently visible.

While you are looking at this, do you have an estimate of how difficult it would be to make the platform changes required to let us implement the mockup in bug 1058775?
More specifically, we would need:
- a wider icon
- the ability to open a XUL panel attached to that icon when it is clicked, instead of a native menu.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

> This is by design. I can't find a comment that clearly calls out
> this requirement, but the idea is that screen sharing continues
> while Firefox is not focused, and the user should know that it's
> happening.

Oh, OK.  Thanks for letting me know.

Comment 40

[Markus Stange [:mstange]]

(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> While you are looking at this, do you have an estimate of how difficult it
> would be to make the platform changes required to let us implement the
> mockup in bug 1058775?
> More specifically, we would need:
> - a wider icon
> - the ability to open a XUL panel attached to that icon when it is clicked,
> instead of a native menu.

Quite a bit more difficult than what this bug did. When do you need this?

Comment 41

[Florian Quèze [:florian]]

(In reply to Markus Stange [:mstange] from comment #40)

> Quite a bit more difficult than what this bug did. When do you need this?

So I'm still figuring out the exact plan, and this is the reason why I'm asking how difficult things are. The current target for the redesign in the mockup from bug 1058775 is the Fx35 cycle, but I would be surprised if we didn't have to scope it down for 35, and implement some of the pieces in later cycles.