1691553 - Fluent labels for dynamically created menuitems are not displayed

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Emma Malysz]

On macOS, document.l10n.setAttributes correctly updates the label on the menuitem, but when the menubar popup is shown, it displays a blank string.

It sounds like this could be a timing issue.

To reproduce:

1. Add this to browser/locales/en-US/browser/menubar.ftl:

menu-history-reopen-all-tabs =
    .label = Reopen All Tabs

2. Change https://searchfox.org/mozilla-central/rev/7067bbd8194f4346ec59d77c33cd88f06763e090/browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm#263-266 to aDocument.l10n.setAttributes(restoreAllElements,'menu-history-reopen-all-tabs'); (Note this isn't "correct" and will reset the app-menu items and reopen all windows button as well, but just a quick way to reproduce)
3. Navigate to the menubar's History --> Recently Closed Tabs and notice the button on the bottom displays a blank string

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

This may or may not be a Fluent DOM bindings bug. It may be that this is an artifact of native menupopup.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: mac-bug.diff

I was able to create a minimized testcase. STR:

1. Apply the patch on m-c
2. Rebuild
3. Open Firefox
4. Open new tab
5. Close it
6. Click on History > Recently Closed Tabs
7. Wait a second

Current Result:

Console log shows a delayed change of the menuitem element's label to "Foo", but visually the menu doesn't update.

Expected Result:

The visible Mac native menu should update its label in reaction to DOM update

Side note: this could also be solved, if onpopupshowing callback could be resolved asynchronously in https://searchfox.org/mozilla-central/rev/b32d4ca055ca9cf717be480df640f8970724a0ce/browser/base/content/browser-menubar.inc#270

But at the moment, if I switch it to async to wait for the menu to be asynchronously localized the popup doesn't show at all.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Brendan - you worked on the similar issue for main mac menu. Do you have any suggestions on how to address a popup menu?

Comment 4

[Markus Stange [:mstange]]

I'm working on the menu code at the moment, I will take a look.

Comment 5

[Brendan Dahl [:bdahl]]

I don't have any concrete ideas off the top of my head. Hopefully the menu changes above will fix it.

Comment 6

[Markus Stange [:mstange]]

The problem is here: https://searchfox.org/mozilla-central/rev/526a5089c61db85d4d43eb0e46edaf1f632e853a/widget/cocoa/nsMenuItemX.mm#284
If the label changes, we queue an update for the next time the menu is opened. We don't update the menu while it's open.

We can change that.

However, refreshing a menu item label while the menu is open is going to create an unstable experience on all platforms. It would be much better to put the right string in place ahead of time. Can we preload it earlier? For example, can we make sure all submenus are translated when a root menu is opened?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

However, refreshing a menu item label while the menu is open is going to create an unstable experience on all platforms.

Can you describe the unstability here? The foundational premise behind Fluent DOM is that "l10n frame" is happening alongside event frames and animation frames and is asynchronous and dynamic. This allows us to do things like change locale on fly, or update translation on fly and it does require that we can at any point run such frame and update translations.

The initial "no translation -> first translation" is no different in that mental model and is meant to be asynchronous and non-blocking.

I can see an argument that "menus are different" in that they're not persistent UI components and most of the time they're short lived so if a l10n frame affects a state of the menu data, the next reopen would use the "new data" which means the new translation.

That would be an okay model if the instability you're describing justified special-casing menus, but from what I understand that problem does not go away after close/reopen cycle because the state of native menu has been already populated prior to l10n frame and on reopen the state is not updated either.

Can we preload it earlier? For example, can we make sure all submenus are translated when a root menu is opened?

We can preload, which would unfortunately require quite a bit of JS refactor since the code in question is written in such a way that makes it not async and not wait for an l10n frame to be completed before sending it, but there are two architectural shortcomings entangled:

1. "preloading" wouldn't solve the live l10n frame updates. If we'd "preload" (meaning, wait for the initial contentful l10n frame to complete), and then want to update the l10n in any later frame, the updated state wouldn't get reflected because native menus are not reactive, right?
2. The bottom line here is that onpopupshowing is not able to handle an asynchronous function. I don't know if this is a limitation of W3C spec, or our platform, but if we want to "preload" without async-in-sync dirty-hacks, we need a model in which we can call an asynchronous function in result of a user clicking on a menu and say "once this asynchronous functions completes, open the menu" and at the moment I don't know how to do that.

Can you advise?

Comment 8

[Markus Stange [:mstange]]

With "instability" I mean the visual flashing effect, where the user is first shown a menu with an empty item, and that item then changes to an item with a translated label. It would be better if the first paint of the menu on the screen contained all the finished contents.

The popupshowing event is a construction of our own making, so at least in theory, we could give popupshowing events a waitUntil(promise) method. However, what we can't change is the macOS API: If a native menu opens, we only get one synchronous notification for "the menu is about to open". If we don't complete our menu modifications during that synchronous callback, macOS will show the menu with the incomplete contents.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you for the context!

I think the cleanest architecture around those two models would be then:

1. Don't cache. Each menu opening should rebuild the menu from the DOM (to allow for new DOM to inform the menu data)
2. Make onpopupshowing handle async function

Is that possible?

Comment 10

[Markus Stange [:mstange]]

Let me clarify: Making popupshowing asynchronous cannot work for native menus on macOS. Because macOS does not give us a way to asynchronously delay the opening of a menu. It only gives us a synchronous callback. So I do not think this is feasible.
We either have to live with the flashing-on-open effect, or the DOM modifications need to happen before the menu is opened (or synchronously during popupshowing).

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Let me clarify: Making popupshowing asynchronous cannot work for native menus on macOS. Because macOS does not give us a way to asynchronously delay the opening of a menu. It only gives us a synchronous callback. So I do not think this is feasible.
We either have to live with the flashing-on-open effect, or the DOM modifications need to happen before the menu is opened (or synchronously during popupshowing).

That's confusing. You said that we control onpopupshowing, so I thought of it as a Mozilla-specific declarative API for front end engineers to declare that when something is clicked, a given menu should open.
If we do control it, we should also be able to control what triggers the native menu opening, and it might be the completion of an async function.

What am I missing?

Comment 12

[Markus Stange [:mstange]]

We control the implementation but our implementation still needs to conform to the system API that we use as part of it.

When the user clicks on a native macOS menu, it's not Firefox that asks for a menu to open. Instead, macOS handles the click and opens the menu. As part of that, it tells Firefox "the menu is about to open", with a synchronous callback. And internally, during that callback, we fire a popupshowing event.

The only place where Firefox is the one to ask for the menu to open is with native context menus, once we have them (bug 34572) - we handle the click, and we ask the system to open a menu as part of our click handling. So for that particular case, we could indeed delay the opening of the menu. But even then, we could only delay opening of the "root" context menu. Once a submenu item inside this menu is hovered, macOS will open the submenu and we won't get a chance to delay that.

This bug was originally filed about a submenu of a menubar menu. There is no way for us to asynchronously delay the opening of a submenu.

Comment 13

[Markus Stange [:mstange]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #11)

You said that we control onpopupshowing, so I thought of it as a Mozilla-specific declarative API for front end engineers to declare that when something is clicked, a given menu should open.

No, popupshowing fires once a menu is already in the process of opening. It doesn't ask for a menu to be opened. It is a way to modify the menu contents just before it's shown, when something else has already requested the menu to be shown.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you for the context!

This bug was originally filed about a submenu of a menubar menu. There is no way for us to asynchronously delay the opening of a submenu.

That's a bit sad, since it seems like MacOS is enforcing synchronous behavior here.

To solve it, we need to ensure that the full menu is "complete" before we trigger opening of the top part according to your suggestion. In this particular scenario it would mean that https://searchfox.org/mozilla-central/rev/b32d4ca055ca9cf717be480df640f8970724a0ce/browser/base/content/browser-menubar.inc#236 History Menu would need to be populated asynchronously before we communicate to MacOS that its ready to be displayed.

:emalysz - that seems to be doable with some refactor to trigger await translateFragment on the whole menu, right?

:mstange - what about retranslations. If the user opened a menu, we updated locale and in result DOM, and the user reopens the menu, can we inform the Native Menu with new DOM content?

Comment 15

[Markus Stange [:mstange]]

So there are two parts here:

• In the Firefox native mac menu code, we have a limitation where any DOM modifications that happen after popupshowing (while the menu is open) are not propagated. They are only respected once the menu is opened again the next time. This is the part that we can fix.
• However, even if modifications while the menu are open will "work", we would like to avoid them because they lead to a flashing effect, where the user sees an "empty item" change into an "item with label". So we should also work on triggering the asynchronous translations ahead of time, so that there's a good chance that they're finished by the time the menu is opened.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #14)

History Menu would need to be populated asynchronously before we communicate to MacOS that its ready to be displayed.

The menubar is ready to be displayed once the browser window opens, which happens once the browser document's load event fires. So I think the best we can do is trigger the translations as soon as we can after window opening, and hope the DOM modifications are done once the user starts interacting with the menus.

For example:

• The names of the top level menu items in the menubar (e.g. "File", "Edit", "View", ...) need to be translated from the start.
• The items at the root level within each of the menus (e.g. "File -> New Tab") should also be translated from the start, otherwise there will be a flashing effect when the menu is opened.
• For items in a submenu, e.g. the "Manage Containers" string (at "File -> New Container Tab -> Manage Containers"), I was about to suggest that the translation could be delayed until the outer menu (e.g. the File menu) is opened. However, then I remembered the Help menu search. We currently don't support it very well, but we'd like to support it well in the future, and this search also requires the entire menu to be populated upfront (either before or inside a synchronous callback) so that it can be searched. So, in conclusion, we want to be able to populate the entire menu contents synchronously, because that's what macOS expects.

:mstange - what about retranslations. If the user opened a menu, we updated locale and in result DOM, and the user reopens the menu, can we inform the Native Menu with new DOM content?

So yes, once we support updating menu contents while the menus are open, that will address this. And for retranslations I think the flashing effect is acceptable because retranslations should happen extremely rarely. (Right?)

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

However, even if modifications while the menu are open will "work", we would like to avoid them because they lead to a flashing effect, where the user sees an "empty item" change into an "item with label".

This is not the case if we were to trigger the menu population when the parent menu opens, right?
I'm talking about the scenario like this:

1. User clicks on "Tools"
2. Our code calculates what "Web Developer" tools should be available and populates DOM of the "Tools>WebDeveloper>" submenu which is invisible
3. User can hover over "Web Developer" menuitem to open the submenu

In such case the lazy selection of items in Web Developer menu is okay, because it would require the user to rapidly select "Tools->WebDev" to open the third level menu and see the flash.

My concern is that saying that all of the menu structure has to be stable at the launch of the window is unrealistic as the menu is becoming more dynamic and engineers are adding event driven triggers to change l10n strings from "bookmark" to "unbookmark" depending on the bookmark state, or "close tab" vs "close tabs" depending on the number of tabs, or which web dev tools are registered based on extensions that can be enabled/disabled on fly.
So, the menu is live and reactive and the localization of it must be as well.

In result in the perfect world we would like to be able to block menupopup opening on some async promise, but if we can't we'd prefer to be able to lazy update a menuitem when it's parent's parent is being opened in a sort of predictive mode ("oh, the user is clicking Tools, let me ensure that all the right Web Developer items are ready because they may open it and I don't want to flash then").
But to be honest, if that fails and somehow the user opens a menu which is being updated, the flash of untranslated, or flash of translation update is a better tradeoff than seeing an empty menu.

The names of the top level menu items in the menubar (e.g. "File", "Edit", "View", ...) need to be translated from the start.

Do they? Technically, it would be ok to have the following sequence:

1. User opens the window
2. We trigger the localization of the menubar
3. The window opens
4. The localization finishes and the menuitems get populated with text

The only thing we'd change is that we wouldn't block the window opening on the localization finishing which might actually speed up perceived performance at the cost of "FOUC" of the menubar. I'm not convinced that this is a bad tradeoff and having technical capabilities to decide on the blockings is a good thing.

The items at the root level within each of the menus (e.g. "File -> New Tab") should also be translated from the start, otherwise there will be a flashing effect when the menu is opened.

Yes, but that we can do lazily:

1. Open window
2. Populate File/Edit/View
3. Window is usable
4. Lazily trigger population of the submenus

Again, we just move the chunk of code past first paint into the second 100ms of the window opening. It's very unlikely that the user will click on 'File" in the first 100ms after opening a window and if they do, then seeing a flash as the translation is populated is acceptable imho edge case behavior.

So yes, once we support updating menu contents while the menus are open, that will address this. And for retranslations I think the flashing effect is acceptable because retranslations should happen extremely rarely. (Right?)

Yes. And my hope is that we can turn this problem from technological limitation of our bindings to architectural decisions of the front end team (what to block on vs risking a FOUC).

Does that sound reasonable to you?

Comment 17

[Markus Stange [:mstange]]

Attachment: Bug 1691553 - Update NSMenuItem title when the label attribute changes, without requiring a menu rebuild. r=harry

I'm planning to write a test for this, in a follow-up patch.

Comment 18

[Markus Stange [:mstange]]

Testing with this patch I couldn't actually notice any flashing. Let's assume for now that this patch fixes the problem entirely.
I will file a new bug if I notice flashing.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec747d3d5f85
Update NSMenuItem title when the label attribute changes, without requiring a menu rebuild. r=harry

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

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