Open Bug 1737951 Opened 3 years ago Updated 4 months ago

Visible delay/lag in fluent translation of context menuitems

Categories

(Firefox :: Menus, defect, P3)

Product:
Firefox
Component:
Menus
Platform:
All
Unspecified
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox93 --- affected
firefox94 --- affected
firefox95 --- affected

People

(Reporter: aminomancer, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

Context menuitems' l10n IDs or attributes change on popupshowing, i.e. before popupshown. But the actual label content changes later, causing visible lag.
236.11 KB, image/webp
Details
tabs-popup-still-flickering.mov
436.48 KB, video/quicktime
Details

Maybe this should belong in Localization Infrastructure and Tools, I'm not really sure what the genesis of the problem is.

It's kind of hard to describe so see the attachment.

STR:

  • Open 5-10 tabs.
  • Multiselect some of them.
  • Right click one of the multiselected tabs.
  • Context menuitems like "Move Tabs", "Close Tabs", "Send x Tabs to Device" are of plural form.
  • Now, without changing the multiselection, right click one of the non-multiselected tabs.
  • The context menu opens again. Now those context menuitems that were previously plural are singular. But the labels change after the context menu has visible opened. I checked and the setting of l10n attributes or IDs happens on popupshowing. So those attributes are changed before the popup is visually displayed. But the labels themselves, the visual text you see, are not shown until after the popup is displayed.
  • Repeat so you can see it happens every time.

For that reason I think the problem is simply that l10n is slow to update the label. Because the popup is only going to wait until javascript execution on popupshowing is finished. But setting l10n-id or l10n-attrs finishes without a hitch before the label has changed.

That is, complete translation (of the text content, value, tooltiptext or whatever attribute) does not block the javascript method call that initiated the translation. Whether it's document.l10n.setAttributes or just el.setAttribute("l10n-id", "something"), there's naturally gonna be some time in between the setting of the l10n attribute and fluent setting the final target attribute or content with the actual localized text.

Which is unavoidable obviously and I'm not saying that stuff should be blocking, but it seems to be slower than ideal. Also, I don't think it has always been like this. I only noticed it a couple months ago and I had been closely working with that context menu for months before that since I made several autoconfig scripts that add new submenus and menuitems (also with dynamic plural forms) to that menu.

Back then the problem did not seem to exist. My custom menuitems still adjust plurality without any visible lag, because they always set label attribute directly instead of using fluent, as compatible strings didn't exist for me to repurpose. So I just mean that I was messing with the context menu in the same way to test my own scripts, doing exactly what I do in the animation, alternating between plural and singular rapidly. And I never noticed a lag in the built-in menuitems until a couple months ago. So that's why I assume there doesn't need to be a lag, there must be some way it can be sped up enough that it happens before the first paint

Zibi, do you know why fluent is slow here, leading to flickering? We update the "close tab" / "close N tabs" at https://searchfox.org/mozilla-central/rev/a9e0a3f5e5f7cde941d419db967997aaa1f06b0f/browser/base/content/tabbrowser.js#6969-6972 which is called from popupshowing at https://searchfox.org/mozilla-central/rev/a9e0a3f5e5f7cde941d419db967997aaa1f06b0f/browser/base/content/main-popupset.inc.xhtml#7 . I would have assumed that because the localization information is all cached, this operation should be sync.

Flags: needinfo?(zbraniecki)

I would have assumed that because the localization information is all cached, this operation should be sync.

There is no notion of "sync" operation. It is always async, the fact that it's already loaded means that there's no I/O necessary and l10n frame happens before or after layout/paint (optimistically, there still may be an error due to formatting error that results in fallback).

In this case my guess is that the popupshowing is triggered before the l10n frame completes, while it should wait for it.

So the order, I guess, may be:

  1. data-l10n-id is updated triggering coalesced l10n frame
  2. popup is shown
  3. coalesced l10n frame is resolved leading to reflow and repaint

I imagine the order should be:

  1. data-l10n-id is updated triggering coalesced l10n frame
  2. coalesced l10n frame is resolved
  3. popup is shown

This can be achieved in multiple ways, one of them being (sketch):

let menu = document.getElementById("menu");
document.l10n.setAttributes(menu.newTabItem, "menu-new-tab", { count });
await document.l10n.translateFragment(menu); // this should be very fast since it's cached so we just want to prevent layout/paint from happening prematurely here.
menu.open();
Flags: needinfo?(zbraniecki)

Hi,

I was able to reproduce this in Nightly 95.0a1 (2021-10-29) (64-bit), Beta 94.0b9 and Release 93.0 (64-bit) on both Mac 11 and Ubuntu 20.04.LTS. I could see that there is a slight delay (the text has a slight delay in changing from plural to singular) from clicking the selection of multiple tabs to the menu when clicking the single tab.

I'll mark this ticket as NEW.

Regards,
Virginia

Severity: -- → S4
Status: UNCONFIRMED → NEW
Has Regression Range: --- → no
Has STR: --- → yes
status-firefox93: --- → affected
status-firefox94: --- → affected
status-firefox95: --- → affected
Ever confirmed: true
Hardware: Desktop → All
Priority: -- → P3
See Also: → 1738056

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

I would have assumed that because the localization information is all cached, this operation should be sync.

There is no notion of "sync" operation. It is always async, the fact that it's already loaded means that there's no I/O necessary and l10n frame happens before or after layout/paint (optimistically, there still may be an error due to formatting error that results in fallback).

Why is there no sync operation, and how hard would it be to fix that?

In this case my guess is that the popupshowing is triggered before the l10n frame completes, while it should wait for it.

No, popupshowing is what changes the l10n here.

So the order, I guess, may be:

  1. data-l10n-id is updated triggering coalesced l10n frame
  2. popup is shown
  3. coalesced l10n frame is resolved leading to reflow and repaint

Right, this happens right now.

I imagine the order should be:

  1. data-l10n-id is updated triggering coalesced l10n frame
  2. coalesced l10n frame is resolved
  3. popup is shown

This can be achieved in multiple ways, one of them being (sketch):

let menu = document.getElementById("menu");
document.l10n.setAttributes(menu.newTabItem, "menu-new-tab", { count });
await document.l10n.translateFragment(menu); // this should be very fast since it's cached so we just want to prevent layout/paint from happening prematurely here.
menu.open();

This isn't how things work. The user right clicking triggers the popup showing, in XUL code. That XUL code fires popupshowing and popupshown events, which the frontend can use to enable/disable menu items (for instance). There is no way for the frontend code to say "oh and by the way, please wait until X", and the XUL code likewise has no idea about l10n.

Of course we could rewrite the tab context menu code to open the menus "manually" instead of relying on the infrastructure that's already there, but that's a ton of work. Can we fix this more generically? In bug 1738056, it seems not only popup menus are affected, so it seems this is a more generic problem that should be solved at the DOM + l10n level, instead of rewriting most of the frontend (and even then, primitives are missing at the DOM/XUL/L10n level anyway).

Flags: needinfo?(zbraniecki)

Why is there no sync operation, and how hard would it be to fix that?

Because it would break the API design. We don't know what resources are loaded / stored in cache at any point, so we either can do:

  1. Execute async task
  2. If any resources need to be loaded, load async
  3. Format and apply

or:

  1. Check if all resources are loaded
  2. If they are then apply synchronously
  3. Otherwise load remaining
  4. Format and apply

We currently perform the former. Switching to the latter would require rethinking several assumptions (would we do the sync only in DOM bindings? Would we also want to allow for imperative API to have something like document.l10n.formatValuesAssumingLoaded() that is sync and throws? Would it lead to engineers writing try { syncVersion } catch (e) { await asyncVersion }? Would we want that?)

This isn't how things work. The user right clicking triggers the popup showing, in XUL code. That XUL code fires popupshowing and popupshown events, which the frontend can use to enable/disable menu items (for instance). There is no way for the frontend code to say "oh and by the way, please wait until X", and the XUL code likewise has no idea about l10n.

Gotcha. I think it's an unfortunate API decision around popupshowing, but maybe we could handle it differently - write popupmenu that stores item for singular, and item for plural, and flip synchronously which one is disabled?

Can we fix this more generically? In bug 1738056, it seems not only popup menus are affected, so it seems this is a more generic problem that should be solved at the DOM + l10n level, instead of rewriting most of the frontend

I think it can be fixed more generally, but I don't think it's an easy design decision to make. My thinking is that we really want two items:

  1. bug 1520659 would give us something like let whenL10nDone = document.l10n.translateElementsWithPromise([elem1, elem2]); that would allow engineers to instrument code to wait for some particular elements to be localized.
  2. consider allowing for synchronous l10n frame to be executed when all resources are loaded. My gut feeling is that we should not allow for imperative API to do that, but declarative bindings could. We should think about it more - this may introduce paper cuts like "if l10n was already loaded, the l10n frame is sync, otherwise is async, have you tested both?"
Flags: needinfo?(zbraniecki)

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

  1. bug 1520659 would give us something like let whenL10nDone = document.l10n.translateElementsWithPromise([elem1, elem2]); that would allow engineers to instrument code to wait for some particular elements to be localized.

I have no frame of reference for what the considerations would be for the sync l10n frame proposal you made. But to respond to your 1st suggestion, I think adding code to wait for a promise every time a translation needs to happen before some synchronous code would have a pretty significant impact. Maybe I'm misinterpreting something, but it seems like this issue should be present in all menupopups, for all menus/menuitems that have labels or access keys in fluent.

But I think the reason it is apparent in this particular menu is just because it's so easy to flip them back and forth. Most of the time menuitems' data-l10n-id is only set once. But it's not always set at startup. There are like 80 elements with data-lazy-l10n-id instead and that is set to the real attribute on user interaction like popupshowing or contextmenu or in some custom component methods that call openPopup.

So that shift only happens once per chrome window. But you can see it when you initially open certain menus, the tab context menu being one of them. Like in addition to these ones that lag when switching plurality, the other menuitems can go from blank to having labels when the context menu is opened first on the all-tabs menu. The only reason it doesn't happen when right-clicking a regular tab is because the lazy IDs are swapped with real IDs not just on the contextmenu event but also on mouseover and focus events in a pretty loose prediction of a right click.

And that's just the menuitems, I'm not really sure where else this should crop up. Maybe it would show up in arrowpanels too if they didn't have a 180ms transition. Anyway I guess I'm just suggesting that it sounds like putting on a lot of bandaids, rather than adding a sort of marginal extra tool for special applications. Just with how ubiquitous and generic this application is, requiring them to explicitly wait for a promise would be adding extra steps to a lot of the frontend code. Realistically it might just go unnoticed for a lot of elements. But in a perfect world where all those are fixed, that translateElementsWithPromise call would probably be getting duplicated in a huge number of various event handlers. Unless I totally misunderstood you, in which case I apologize

Maybe I'm misinterpreting something, but it seems like this issue should be present in all menupopups, for all menus/menuitems that have labels or access keys in fluent.

You are correct! This is also my read of the situation.

I think adding code to wait for a promise every time a translation needs to happen before some synchronous code would have a pretty significant impact.

I think it would mostly have an impact on code readability and maintenance. It's a paper cut. Performance wise, I wouldn't expect much impact, and jank should be better with async.

There are like 80 elements with data-lazy-l10n-id instead and that is set to the real attribute on user interaction like popupshowing or contextmenu or in some custom component methods that call openPopup.

Yeah, bug 1584189 is about trying to build a more consistent API out of the lazy approach. I think it's evidently important.

Anyway I guess I'm just suggesting that it sounds like putting on a lot of bandaids, rather than adding a sort of marginal extra tool for special applications.

Yes. I agree. The original motivator for the formatWithPromise was to be used in tests, not as a replacement for runtime design.
I filed bug 1738748 to discuss a potential solution, but inherently it is not an easy problem space to solve.

L10n is designed to use async I/O very deeply. I think it is the right decision - it reduces jank, allows for low memory overhead, brings new capabilities (RemoteL10n etc.).
But it also comes into friction against synchronous API design, which XUL has plenty of.

So, a strawman example of that is - in an ideal world, if I clicked on a button and a menu was to be opened up, but some very slow (say 5 seconds) lazy operation had to be completed before it shows up would we prefer to:

  1. Show "old" menu immediatelly and flicker to new strings once they're loaded?
  2. Show menu without strings for 5 seconds and then show strings?
  3. Don't show menu for 5 second and only paint it when the loading is done?

(1) feels like a wrong solution. (2) seems like a decent approach (I hypothetize that perceived performance of such scenarios would almost always be very positive) but the problem is that until we load the strings we don't know how layout will work so we risk reflows.
(3) is what Fluent optimized for.

Now, of course we don't want to ever be loading for 5 seconds, but the approach works irrelevant of if it's 5 sec or 20ms. The problem is on the other edge - do we always have to delay?

From the perceived performance perspective I claim it is irrelevant. Whether we react to your click by opening the popup in 0ms or 1ms (due to async task) makes no difference. What's left is how you write code so that it doesn't do get in the following scenario:

  1. Menu gets DOM updated which schedules l10n frame
  2. Menu opens up (with old data)
  3. l10n frame resolves applying new strings and causing flickr

In an ideal world, I'd say that having (2) wait for l10n frame resolution would be great, but I don't know if we can get there, and some form of "optimistic" l10n frame that is sync and bails to async seems like may give us a very good balance of common to edge case experience.

Yes I agree #3 is the most appealing option. I don't think #2 is a good option because it just has even more extreme problems of #1. For some elements string length will trigger a reflow so things will look even wackier than they already do, when every label in a menupopup is blank, jumping from like 20px wide to 300px. So just any way to bury #3 deeper in the inheritance chain would be good, or I will routinely forget to add it lol

The main issue, as Gijs pointed out, is that popupshowing is sync, while we are trying to perform an async operation before it gets opened.

There are three "pieces" of the equation that are working against us when we try to apply async translation in the popupshowing case:

  1. We use MutationObserver which coalesces all l10n changes in a single animation frame and triggers just one localization per animation frame.
  2. We spawn moz_task with regular priority
  3. We execute a Rust future to retrieve translations

(3) is potentially optimizable in bug 1738748, but fortunately, it seems to me that a combination of (1) and (2) is sufficient to solve this bug.

In bug 1746124 I was able to improve the performance of the async calls elevating their priority (2), and here you can solve (1).

To solve it you need to replace [0] the following:

    contextMoveTabOptions.setAttribute("data-l10n-args", tabCountInfo);
    // (...)
    document
      .getElementById("context_closeTab")
      .setAttribute("data-l10n-args", tabCountInfo);

with:

let closeTab = document.getElementById("context_closeTab");

document.l10n.pauseObserving();
closeTab.setAttribute("data-l10n-args", tabCountInfo);
contextMoveTabOptions.setAttribute("data-l10n-args", tabCountInfo);
document.l10n.resumeObserving();

document.l10n.translateElements([closeTab, contextMoveTabOptions]);

This means that you spin the localization immediately, rather than letting document's L10nMutationObserver coalesce it into a frame later. The pausing/resuming just guarantees that you avoid extraneous work.

Assuming there are more than two that use args, it may be nice to refactor this code to combine all l10n-related updates next to each other surrounded by pause/resume, and trigger translation of all of those elements together.

This is not lowest burden, I still believe bug 1738748 would be nice to pursue as it would generally improve performance of live-dom-l10n updates all around, but for cases where we need to race against animation frame, this seems like an okayish solution and it is available as soon as bug 1746124 lands.

:Gijs - does it seem acceptable to you? Do you think we can work with such approach to those timing-sensitive cases where we need l10n measured or we deal with popups? (for all "normal" cases I'd still recommend regular DOM updates).

[0] https://searchfox.org/mozilla-central/rev/a9e0a3f5e5f7cde941d419db967997aaa1f06b0f/browser/base/content/tabbrowser.js#6909-6972

Flags: needinfo?(gijskruitbosch+bugs)

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

:Gijs - does it seem acceptable to you? Do you think we can work with such approach to those timing-sensitive cases where we need l10n measured or we deal with popups? (for all "normal" cases I'd still recommend regular DOM updates).

I don't really like this because we'd need to update every single bit of menupopup/panel-related code that updates data-l10n-id (or args) dynamically - not just the tab context menu - and it creates a two-tier system where "normally" you can just call document.l10n.setAttributes and it will Just Work, but you cannot rely on this for popups/panels.
I would prefer a structural solution, such as teaching the document.l10n.setAttributes implementation about this itself. Of course, that has the same two-tier/specialcase effect but abstracts it away from frontend developers. I find it difficult to reason about whether this would still lead to unexpected confusion or not (ISTM having l10n happen sooner rather than later is always better?), so perhaps I'm missing some reason this is a bad idea. Generally speaking I really want the l10n system to be as DWIM as possible, and your proposed consumer-side solution moves us away from that so I can't say I'm enthusiastic...

Does that make sense?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(zbraniecki)

teaching the document.l10n.setAttributes implementation about this itself.

Do you suggest we identify cases where sync is needed and augment l10n.setAttributes to execute the operation immediately in such scenarios?

For example, can we detect we're in a popupshowing callback from the stack trace?

I really want the l10n system to be as DWIM as possible, and your proposed consumer-side solution moves us away from that so I can't say I'm enthusiastic...

I agree.

Flags: needinfo?(zbraniecki) → needinfo?(gijskruitbosch+bugs)

I think the problem with that is it's not just popupshowing events but pretty much anything that can conceivably result in an element being visually shown. So I don't think implementing contextual behavior is an ideal long-term solution. I agree with Gijs about the proposed solution, what I was trying to say earlier is that it's gonna yield a lot of redundant code and probably ultimately result in helper functions being made, and most of the calls being replaced with calls to those helper functions. So why can't we just implement it in l10n.setAttributes, e.g., add a 4th optional boolean parameter like bypassObserver? That still burdens frontend developers to know about this change, but Firefox has changes like that all the time. And maybe it's better than tacking on overhead of trying to deduce information about the caller every time JavaScript calls the method. Assuming there isn't a deeper solution obviously, but that's outside my bubble of comprehension.

I gotta say though, I can't imagine a scenario where I would use l10n.setAttributes where I wouldn't want it to do what I said synchronously. Because I'm virtually never gonna use that method except in an event callback or some other intrinsically asynchronous behavior. Anywhere where async is appropriate would probably be set up at the markup level. So why not just make l10n.setAttributes translate the passed elements by default? The pause/resume thing looks like it would be more cleanly implemented at the DOMLocalization level. If we're trying to jump the line, we always want to skip the observer too anyway.

By the way, by redundant code I just mean that seeing

document.l10n.pauseObserving();
el.setAttribute("data-l10n-id", ...);
document.l10n.resumeObserving();
document.l10n.translateElements([el]);

in every instance a script would otherwise call el.setAttribute("data-l10n-id", ...) or document.l10n.setAttributes(...) just seems kinda crufty. Although I guess it hinges on whether there are instances where we wouldn't want this "jump the line" behavior.

I think the problem with that is it's not just popupshowing events but pretty much anything that can conceivably result in an element being visually shown.

That's incorrect. In cases where you control when the paint happens there is a benefit of async l10n in jank reduction.
So, for dominant cases it makes a lot of sense that our l10n is coalesced per animation frame. The only place where it is a problem is when we can't control paint because it is sync.

So why can't we just implement it in l10n.setAttributes, e.g., add a 4th optional boolean parameter like bypassObserver?

That's a reasonable API change. We could even do both - add a param and set it to true in some stacks.

I gotta say though, I can't imagine a scenario where I would use l10n.setAttributes where I wouldn't want it to do what I said synchronously.

That is concerning for me. You should always want to delay everything as much as possible for performance reasons. The most common case of writing UI code is to have an event that triggers visual update.
Such visual update can happen in 10ms or 50ms without any impact on user experience, and thus should be the lowest async priority below everything else that Firefox, and frankly your computer, is doing.

Saying "In all scenarios I'd like to force my l10n to happen as fast as possible" leads to tragedy of commons and I would like to discourage that attitude.

in every instance a script would otherwise call el.setAttribute("data-l10n-id", ...) or document.l10n.setAttributes(...) just seems kinda crufty. Although I guess it hinges on whether there are instances where we wouldn't want this "jump the line" behavior.

I agree. And your comment about "people will just write helper wrappers" feels true to me as well and is a good indicator that we need to rethink the API so that helper wrappers are not as obviously needed.

My claim is that in 95% of cases you will not care about 10-20 ms or microtask-vs-task for when the update happens on screen. As long as it's within human visual reaction time budget (150-200ms), we actually benefit from setting visual update to the lowest possible priority to reduce jank.

Then there is a second class of scenarios where you can delay, but you rely on knowing when it's done because you want to perform some operation after l10n is applied. This is what bug 1520659 is about - giving developers ability to say:

document.l10n.setAttributes(elem, l10nId, l10nArgs)
  .then(() => { doSomethingThatDependsOnL10nBeingDone() });

Then the third is where we need l10n to be applied immediately, because we just don't have architecture to wait and it causes flicker. This is this case and in my experience of working with Fluent use the popupshowing is the dominant (or maybe only?) scenario in which it shows up.

============

Saying all of it, I'm not suggesting that we're done here. I recognize the problem space, and it is one that when we designed Fluent we didn't sufficiently address for various reasons. We need to improve that.

Based on this conversation, I'm suggesting:

  1. Add a method that allows for a promise to be returned when altering DOM elements l10n (bug 1520659)
  2. Add a parameter to setAttributes that triggers immediate localization rather than coalesced (this bug)
  3. Consider warm-case scenario in Localization class for cases where messages are loaded (https://github.com/projectfluent/fluent-rs/issues/244)

The (3) is most challenging but I think worth pursuing. (2) is the easiest, so I can take it on next.

:nordzilla, :eemeli - NI'ing you FYI. Are you ok with :aminomancer's suggestion for (2)?

Flags: needinfo?(enordin)
Flags: needinfo?(earo)

Well yeah, but you're usually only using DOMLocalization from javascript when you're trying to make things happen fast. It's a question of responsiveness. It's not like this is some really costly operation. Especially given your proposed changes. Situations where we don't care about how fast a label is translated are probably situations where we're not changing the translation in javascript anyway. I guess one exception is translating tooltips. So maybe having an extra parameter or just having two methods is appropriate. That does add a knowledge burden though.

Anyway, if setAttributes returned a promise then the majority of non-popupshowing applications would be obviated. I think you're right that, at least in terms of js, there isn't any other condition where a promise wouldn't suffice. I hadn't seen that bug, that seems like a better solution for everything else. So I guess in that respect you're right that popupshowing is kind of a special case.

It still seems kinda awkward to be detecting the event type from the caller, but I don't know much about Firefox's binary code, maybe it's more common than I realize. That would certainly solve all the translation lags I've actually seen in the chrome in a single commit, without anyone needing to learn anything new. My other ideas are pretty much hypothetical, but having a parameter to force it for edge cases seems like a good idea anyway.

Well yeah, but you're usually only using DOMLocalization from javascript when you're trying to make things happen fast. (...) Situations where we don't care about how fast a label is translated are probably situations where we're not changing the translation in javascript anyway.

That's not my experience. People use imperative DOM L10n state updates when they need things to be dynamic. For example, updating a message with amount of data stored in cache in preferences, or time since last visit to a website in bookmarks UI, or selected option from a menu.

I feel like your mental model of what is the common case (vs exception) is vastly different from mine.

My angle of approach is mostly restricted to tinkering with chrome frontend stuff for obscure user mods, so maybe I just have a naive view of the issue. Most of my time spent using fluent or reading js pertaining to fluent involves making and updating menus and buttons. I was just trying to help by offering my perspective, but I am sure you know much more about this than I do.

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

  1. Add a parameter to setAttributes that triggers immediate localization rather than coalesced (this bug)

I'd be okay with this. There is a clear need for being able to set/alter l10n content in a synchronous context, and this is a right place where to extend the current API to allow for that.

To bikeshed/validate the proposed API a bit, have I understood right that the mutation observer won't necessarily add the elements to its list of pending ones during the setAttributes() call, but just sometime after it? I just want to verify that we do need something like:

document.l10n.setAttributes(..., { immediate: true })
document.l10n.setAttributes(..., { immediate: true })

rather than e.g. making it possible to immediately process all pending changes?

document.l10n.setAttributes(...)
document.l10n.setAttributes(...)
document.l10n.flush()
Flags: needinfo?(earo) → needinfo?(zbraniecki)

but I am sure you know much more about this than I do.

I did not intend to discount your experience, I'm sorry. I flagged the difference in mental models as a sign that our difference in opinion is likely not rooted in difference in how we think the balance should be implemented, but what is the right balance in this situation.
And maybe that we need more data :)

To bikeshed/validate the proposed API a bit, have I understood right that the mutation observer won't necessarily add the elements to its list of pending ones during the setAttributes() call, but just sometime after it? I

The way this works at the moment is a response to a problem we were facing when engineers would write code that adds/removes/updates l10n-id multiple times in a single frame triggering a flock of localizations of which only the final one mattered. ( see bug 1389384 for details).

We switched to a batched approach where MutationObserver reacts to changes by adding elements to pendingElements and that batch is localized once per frame.

That behavior is still asynchronous tho! In optimistic scenario, all strings are loaded so the resolution of the async call is a micro-task, but the way the architecture is designed allows for a case where:

  1. Developer alters DOM l10n-id
  2. We add the element to pendingElements
  3. Animation Frame happens, we trigger Localization::FormatMessages
  4. That triggers async I/O for fallback
  5. Only once that is resolved the translation is applied

Due to (4) this can take longer.

I'm ok letting it stay like this (if we want to change (4) for cases where all messages are pre-loaded already, that's https://github.com/projectfluent/fluent-rs/issues/244), but the change here would be that for { immediate: true } we would instead do:

  1. Developer alters DOM l10n-id
  2. We immediatelly trigger Localization::FormatMessages
  3. That triggers async I/O for fallback
  4. Only once that is resolved the translation is applied

This would be exactly the same as my proposed:

l10n.stopObserving();
l10n.setAttributes();
l10n.translateElements();
l10n.resumeObserving();

but with easier to reason about API.

The only downside of it, is that it would trigger multiple localization calls in a (common I think) scenario like this:

l10n.setAttributes(elem1, null, { immediate: true });
l10n.setAttributes(elem2, null, { immediate: true });

Because each call would trigger formatMessages. That would happen, for example, in this code that we're discussing in this bug because a single menu update contains updates to multiple options.
That's why I recommended in comment 10 to factor this code in a way that combines all elements that get updated together and trigger a single localization.

To adapt it to the proposed API, we might want to add something like:

l10n.setAttributes([
  {element: elem1, id: undefined, attrs: { tabCount }},
  {element: elem2, id: undefined, attrs: { tabCount }},
], { immediate: true });

to allow for batching still.

WDYT?

Flags: needinfo?(zbraniecki) → needinfo?(earo)

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

teaching the document.l10n.setAttributes implementation about this itself.

Do you suggest we identify cases where sync is needed and augment l10n.setAttributes to execute the operation immediately in such scenarios?

For example, can we detect we're in a popupshowing callback from the stack trace?

I wouldn't do it based on the stack trace, but sooner based on there being a panel or popup element in the ancestor chain.

WRT the rest of the discussion, I'm somewhere in the middle between you both. ;-)

I think the translation being not-sync is not a problem literally everywhere, but I do agree there are issues in other places too - for me on macOS (esp. now that I updated to 11 which changes some styling aspects for sheets/modals) another notable case is native alert dialogs and wizards. I haven't dug into it but it seems to be something to do with the timing of setting the l10n ids for the buttons which means we first render the dialog with text-less buttons, and then the buttons suddenly get much bigger when they get filled with text and the entire dialog resizes. That's sad and janky. :-(

Flags: needinfo?(gijskruitbosch+bugs)

Filed bug 1746748 for this idea, let's discuss it there.

Assignee: nobody → zbraniecki
Status: NEW → ASSIGNED
Depends on: 1746748
Flags: needinfo?(enordin)
Flags: needinfo?(earo)
Assignee: zibi → nobody
Status: ASSIGNED → NEW

bug 1819664 and bug 1738056 comment 22 outline how to fix this.

Depends on: 1819664

I tried to use the support from bug 1819664 but there is still flicker (see video attachment, when the second popup comes up). This is the patch I tried: https://paste.mozilla.org/Myc10riS

If I had to guess, the visibility: hidden hack doesn't help on macOS with native menupopups.

Emilio, is that right or did I do something else wrong? The console does log that hasPendingL10nMutations is true when the popupshowing event fires, so AFAICT the JS code should be doing its job.

Assuming I'm right about the native macOS menu stuff, would it make sense to push this functionality into platform?

Flags: needinfo?(emilio)

Hmm, yeah, that seems right. Curious, if you use -moz-window-opacity: 0 rather than visibility: hidden, does that do the trick?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

Hmm, yeah, that seems right. Curious, if you use -moz-window-opacity: 0 rather than visibility: hidden, does that do the trick?

It doesn't appear to, no. :-(

I have another screen recording but it's functionally the same as the previous one.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)
Blocks: 1821057

So your patch needs a tweak, you're doing: addEventListener("DOML10nMutationsFinished", but it should be just "L10nMutationsFinished". With that your original patch works for me on Linux...

Flags: needinfo?(emilio)

Does -moz-window-opacity on popups hide them on macOS? I would expect them to but maybe they do not and is why your patch appears to work...

Flags: needinfo?(gijskruitbosch+bugs)

Ah, I suspect it doesn't. This is the only call where we read style information from the popup to the native menu. I suspect we need some code to hook the window opacity stuff up, but it should be feasible.

But anyways the problem you're seeing seems specific to native context menus on macOS, I can fix that.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #27)

So your patch needs a tweak, you're doing: addEventListener("DOML10nMutationsFinished", but it should be just "L10nMutationsFinished". With that your original patch works for me on Linux...

D'oh, sorry for not realizing that myself. That was in https://bugzilla.mozilla.org/show_bug.cgi?id=1738056#c22 , fwiw :P

(In reply to Emilio Cobos Álvarez (:emilio) from comment #30)

But anyways the problem you're seeing seems specific to native context menus on macOS, I can fix that.

Sorry, so, would that be a mac-specific core/C++ fix just to make the CSS work? And if so, would we want to use window-opacity or visibility: hidden?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #31)

Sorry, so, would that be a mac-specific core/C++ fix just to make the CSS work? And if so, would we want to use window-opacity or visibility: hidden?

Yeah, so that's what I was thinking of doing, but then I realized that native macOS menus don't seem to support dynamically getting hidden or so... So maybe we need to actually delay the opening of the popup. Markus do you know of any NSMenu API we could use to hide the context menu? Just in case I've overlooked something.

Flags: needinfo?(emilio) → needinfo?(mstange.moz)

I don't know of an API to make an open menu invisible, and I think it would be a weird API to have. If you want to delay opening the menu, just open the menu later.

Here's the code which fires popupshowing before asking the menu to open: https://searchfox.org/mozilla-central/rev/82828dba9e290914eddd294a0871533875b3a0b5/layout/xul/nsXULPopupManager.cpp#902

Could we make that part asynchronous somehow? For example, the popupshowing event object could have a .delayUntil(promise) method.

Flags: needinfo?(mstange.moz)

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

Could we make that part asynchronous somehow? For example, the popupshowing event object could have a .delayUntil(promise) method.

This would work for frontend, I think.

See Also: → 1861487

This also affects the Add "search engine" menuitem in the address bar context menu (the item for adding a search engine from a site's OpenSearch description). The label is dynamically generated, so it starts out with no label and updates after 20ms or so.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: