Closed Bug 1292527 Opened 3 years ago Closed 3 years ago

Hidden Mac menus should listen to changes to the original DOM attributes

Categories

(Core :: Widget: Cocoa, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Pike, Assigned: spohl)

References

Details

(Whiteboard: tpi:+)

Attachments

(1 file, 6 obsolete files)

We're rewriting how we localize Firefox, and one of the things that changes is that the translation of the UI becomes something that changes at runtime.

In particular, I notice that the "Quit Firefox" menu doesn't show any text anymore, and looking at https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuBarX.mm#505, I suspect that this code takes the content reallly early, and then never looks back.
...

Can we make the native menus update when the original DOM changes?

We want to use that for runtime translation, and locale switching at runtime. Also, we'd use this for just the basic translation of the menubar.

We're using the twig larch to land our work-in-progress, and that has test builds that you can look at, too.

PS: Sorry for the double post, I hit enter on CC completion, and that submitted the bug early.
Markus, can you evaluate how much work would it be to make native Mac menu react to DOM changes in Fx menu?

Is it a matter of simple MutationObserver or something more complex?

I think we can fix the order of operations easily and make the dom localized before Mac bindings take the strings, but we plan to enable dynamic l10n changes during the liftime of the app (like, string updates etc.) and it would be great if mac bindings could pick them up and update native menu.
Flags: needinfo?(mstange)
Is this bug specific to the application menu? We already have a mutation observer for regular menus. The code that Axel linked to looks special though.
One thing I should note is that macOS prevents us from making changes to the menu while it's visible. I hope you don't require that (it doesn't sound like you do).
Flags: needinfo?(mstange)
I don't know what's the difference between regular and special menus, but we'd like to get the application menu react to DOM changes.

Is that possible?
Flags: needinfo?(mstange)
It is possible. A bit of work, but not too much.

At the moment we create the application menu once, in nsMenuBarX::CreateApplicationMenu, which is called from nsMenuBarX::InsertMenuAtIndex the first time we get there.
Flags: needinfo?(mstange)
What's the eta on larch merging to central?
Flags: needinfo?(gandalf)
Priority: -- → P1
Whiteboard: tpi:+
Duplicate of this bug: 1303300
This is actually making our work on larch hard. Anybody trying to help larch (like jfkthame just now) hits this one pretty immidiately.

Thus it'd be great if we could fix this to ease the development on larch, and not try to fix this in time for the merge of larch.
Flags: needinfo?(gandalf)
Would you have time to look at this? We're getting closer to the landing milestone.
Flags: needinfo?(jmathies)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Would you have time to look at this? We're getting closer to the landing
> milestone.

We can try to fit this in with other high priority osx widget work we have going on. I'll make a note of it.
Flags: needinfo?(jmathies)
Just for info, I'm seeing other "disappearing" menu issues on a Larch build that I presume are due to this same issue(?).... for example, on opening the Page Info dialog, the titles of all the menus from File through to Tools disappear, leaving a gap in the menubar; the menus do still exist and can be pulled down, but when doing that, most (not all) of the menu items in them are also blank.

Same thing happens if I open the Library window via the Show All {Downloads,History,Bookmarks} commands, for instance.
I can reproduce the other items disappearing. It may be the same issue or not. Let's wait for :jimm to get us a MutationObserver hooked and see if it fixes it.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Would you have time to look at this? We're getting closer to the landing
> milestone.

Could you clarify when this landing milestone is scheduled for? Thanks!
Flags: needinfo?(gandalf)
(In reply to Stephen A Pohl [:spohl] from comment #13)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> > Would you have time to look at this? We're getting closer to the landing
> > milestone.
> 
> Could you clarify when this landing milestone is scheduled for? Thanks!

We're currently targeting to land this in Fx 53 within the next 1-2 weeks. This is one of two bugs that block us from being able to do that.

What's your timeline to get to it?
Flags: needinfo?(gandalf) → needinfo?(spohl.mozilla.bugs)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> What's your timeline to get to it?

I just finished cloning and building the larch repo. If you could point me to where we're dynamically changing the text/localization of these menus on larch, that would speed things up.
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(gandalf)
synced on IRC. Assigning to :spohl
Assignee: nobody → spohl.mozilla.bugs
Flags: needinfo?(gandalf)
Attached patch Patch (obsolete) — Splinter Review
Once we localize the hiddenWindow, this patch should "just work" to update the quit menu item in the application menu. As discussed via IRC, this patch is not expected to fix the other disappearing menu items in the regular menu (i.e. not the application menu). Setting feedback? as heads up in case you'd like to verify for yourself.
Attachment #8809533 - Flags: feedback?(gandalf)
Attached patch Patch (obsolete) — Splinter Review
Forgot to remove an old comment.
Attachment #8809533 - Attachment is obsolete: true
Attachment #8809533 - Flags: feedback?(gandalf)
Attachment #8809534 - Flags: feedback?(gandalf)
Filed bug 1316720 to populate documents with l20n.js.
See Also: → 1316720
Comment on attachment 8809534 [details] [diff] [review]
Patch

lgtm!
Attachment #8809534 - Flags: feedback?(gandalf) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Minor cleanup. Carrying over :gandalf's f+.
Attachment #8809534 - Attachment is obsolete: true
Attachment #8809587 - Flags: review?(mstange)
Attachment #8809587 - Flags: feedback+
https://hg.mozilla.org/projects/larch/rev/8616872c1e2299bd55ae435654de757f3175c494
Bug 1292527 - Hidden Mac menus should listen to changes to the original DOM attributes. r=mstange
Comment on attachment 8809587 [details] [diff] [review]
Patch

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

I still haven't understood this patch completely, so I'll need to continue my review tomorrow.

I'm rather confused by what's happening. Are you blowing away the whole menu bar whenever the application menu opens? -[MenuBarDelegate menuWillOpen:] creates a new nsMenuBarX, uses it to recreate the menubar and all contents, and destroys it again. What happens to the original nsMenuBarX that other things are holding on to? Are the mutation observers from it still in the right place? I guess you're not blowing away the DOM elements, only the native menu objects, but still...
Comment on attachment 8809587 [details] [diff] [review]
Patch

Yeah, I think what you should do instead is, give the delegate a pointer to the nsMenuBarX instance, and then only blow away + recreate the application menu, and don't create any new nsMenuBarX instances in the process. You also shouldn't need to add any global variables.
Attachment #8809587 - Flags: review?(mstange) → review-
Attached patch Patch (obsolete) — Splinter Review
You know, I had a patch that did just that very early on. I dismissed the patch because it didn't work. We later realized that the hiddenWindow wasn't being translated, but I forgot to go back and recheck this patch.

I cleaned this up and confirmed that things work properly.
Attachment #8809587 - Attachment is obsolete: true
Attachment #8811880 - Flags: review?(mstange)
Comment on attachment 8811880 [details] [diff] [review]
Patch

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

::: widget/cocoa/nsMenuBarX.mm
@@ -347,5 @@
>  
>  // Calling this forces a full reload of the menu system, reloading all native
>  // menus and their items.
> -// Without this testing is hard because changes to the DOM affect the native
> -// menu system lazily.

Is this comment not accurate anymore?

@@ +514,5 @@
> +void nsMenuBarX::MenuOpened()
> +{
> +  if (mNeedsRebuild) {
> +    ResetNativeApplicationMenu();
> +    ForceNativeMenuReload();

Isn't this still reloading all menus and not just the application menu?
(In reply to Markus Stange [:mstange] from comment #26)
> Comment on attachment 8811880 [details] [diff] [review]
> Patch
> 
> Review of attachment 8811880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsMenuBarX.mm
> @@ -347,5 @@
> >  
> >  // Calling this forces a full reload of the menu system, reloading all native
> >  // menus and their items.
> > -// Without this testing is hard because changes to the DOM affect the native
> > -// menu system lazily.
> 
> Is this comment not accurate anymore?

Well, we only used this for testing purposes before and I didn't want someone to think that this was still the case. I guess this depends on what your answer is to my comment below (i.e. whether or not we will actually use ForceNativeMenuReload() here).

> @@ +514,5 @@
> > +void nsMenuBarX::MenuOpened()
> > +{
> > +  if (mNeedsRebuild) {
> > +    ResetNativeApplicationMenu();
> > +    ForceNativeMenuReload();
> 
> Isn't this still reloading all menus and not just the application menu?

Yes, that's correct. I didn't see an obvious way to untangle CreateApplicationMenu() from the nsMenuX instance that's passed to it. Also, since this should occur so rarely, I wasn't sure if it merited the effort. If you think it does I can definitely take another look.
Could you rename nsMenuBarX::MenuOpened() to nsMenuBarX::ApplicationMenuOpened() and, instead of calling ForceNativeMenuReload, call CreateApplicationMenu(mMenuArray[0])?
Attached patch Patch (obsolete) — Splinter Review
Yes, of course! :-)
Attachment #8811880 - Attachment is obsolete: true
Attachment #8811880 - Flags: review?(mstange)
Attachment #8811915 - Flags: review?(mstange)
Comment on attachment 8811915 [details] [diff] [review]
Patch

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

Thanks!

::: widget/cocoa/nsMenuBarX.h
@@ +20,5 @@
>  class nsIWidget;
>  class nsIContent;
>  
> +// MenuBarDelegate is used to receive Cocoa notifications.
> +@interface MenuBarDelegate : NSObject<NSMenuDelegate>

Actually, let's rename this to ApplicationMenuDelegate.

::: widget/cocoa/nsMenuBarX.mm
@@ +516,5 @@
> +void nsMenuBarX::ApplicationMenuOpened()
> +{
> +  if (mNeedsRebuild) {
> +    ResetNativeApplicationMenu();
> +    CreateApplicationMenu(mMenuArray[0]);

It would probably be a good idea towrap this inside an if (!mMenuArray.IsEmpty()).
Attachment #8811915 - Flags: review?(mstange) → review+
Attached patch Patch (obsolete) — Splinter Review
Addressed feedback, carrying over r+.

:gandalf, just a heads up that there is a new patch for this. It might be easiest to back out the changeset in comment 22 and apply this patch instead.
Attachment #8811915 - Attachment is obsolete: true
Flags: needinfo?(gandalf)
Attachment #8812143 - Flags: review+
yeah, I'll do that before the next merge to larch. Land on central at will.
Flags: needinfo?(gandalf)
https://hg.mozilla.org/mozilla-central/rev/3d7142a4f06d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1319911
This was backed out in bug 1319911 due to the reported regression there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Markus, looking for your input here:

We create one nsMenuBarX instance per browser window. However, sApplicationMenu is a global and its delegate is set to the most recent window created. The issue in bug 1319911 was caused by the fact that nsMenuBarX's destructor reset the global application menu. However, it's not as easy as not resetting the application menu because there is a greater problem here:
1. First window opens and creates an nsMenuBarX instance ("menubarx1"), sApplicationMenu's delegate is set to menubarx1.
2. Second window opens a second nsMenuBarX instance ("menubarx2"), sApplicationMenu's delegate is set to menubarx2.
3. Second window closes.
4. Content in application menu changes (due to runtime localization for example).
5. sApplicationMenu no longer has a valid delegate since the second window was closed and can't rebuild the application menu.

I would obviously like to avoid keeping a list of possible windows and/or delegates for sApplicationMenu when windows get closed.

Note that the patch in comment 21 did not suffer from this issue because the delegate was global as well.

Do you have any thoughts on how to proceed here?
Flags: needinfo?(mstange)
May have a fix. Clearing n-i for now.
Flags: needinfo?(mstange)
Attached patch PatchSplinter Review
As discussed via IRC. I think something like this could work. We only init a delegate for the first window, which is assumed to be the hiddenWindow, and set it as the application menu's delegate.
Attachment #8812143 - Attachment is obsolete: true
Attachment #8814212 - Flags: review?(mstange)
Comment on attachment 8814212 [details] [diff] [review]
Patch

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

Looks good, sorry for the delay. I assume you have tested that this works. :-)
Attachment #8814212 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #39)
> Comment on attachment 8814212 [details] [diff] [review]
> Patch
> 
> Review of attachment 8814212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, sorry for the delay. I assume you have tested that this works.
> :-)

Thank you! Yes, I've verified that the reported regression is fixed and that we continue to have the ability to localize the menu. Keeping fingers crossed that there are no other regressions. Will check in later tonight or tomorrow.
https://hg.mozilla.org/mozilla-central/rev/896204eba388
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.