Closed Bug 1501886 Opened 6 years ago Closed 5 years ago

Migrate browser-menubar to Fluent

Categories

(Firefox :: Menus, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(3 files, 3 obsolete files)

As part of bug 1501881 we'll want to migrate the browser-menubar.inc to Fluent.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Depends on: 1441035
Component: General → Menus
Priority: -- → P3
the idea here is to introduce a lazy JSM component in toolkit, which will get constructed at the startup with the root element and the path to JSON file with the data.

Then, on MacOS we would initialized right after the browser is interactive, and on Windows/Linux by default we'd only initialize it when the user unhides the toolbar.

The initialization will trigger an async I/O for the JSON which will then get populated to DOM or sent to MacOS.


For live changes, I'd like to allow users to register promises to be resolved when the menu is initialized before the content is dumped into DOM, and some event to be registered every time the menu changes (to allow code to inspect it if needed).
In line with bug 1501881, I'm considering first moving the current DOM-driven data structure to Fluent but localize it lazily after first paint.

Initial talos numbers look promising: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=87c3839e6336&newProject=try&newRevision=cf3eda5ae8bc&framework=1
Since you're the triage owner, I assume it's your call whether we can approach this strategy. I haven't yet test it on mac, but I did select the "View>Toolbars>Menu Bar" checkbox and restarted and the browser loaded normally with the top menubar appearing after the newtab is loaded.

I can see us going in one of the three directions:
 - make us eagerly localize it if the menu is visible by default, otherwise lazily
 - accept that it shows up late both in visible and hidden scenario
 - delay transitioning to Fluent until performance is on par (bug 1441035)

I'd like to verify that you're ok with me pursuing this direction before I invest more time in writing the full patch.
Flags: needinfo?(jaws)
The triage owner isn't the same as the submodule owner, so it's not really "my call". It's likely a mix of UX and Engineering people that should make this call (tradeoffs on user experience, performance, maintainability, etc).

I doubt we would be fine with "accept that it shows up late both in visible and hidden scenario" since we've done a fair amount of work to reduce the various number of paints during startup.

So I see us going with one of: a. "make us eagerly localize it if the menu is visible by default, otherwise lazily" or b. "delay transitioning to Fluent until performance is on par (bug 1441035)". I'm guessing eventually we could do both a. and b.

Stephen and Florian would be good people from both orgs to get feedback from, as they have both spent a good amount of time looking at startup.
Flags: needinfo?(shorlander)
Flags: needinfo?(jaws)
Flags: needinfo?(florian)
After looking at the other comments from bug 1501881 I'm okay with the menubar text appearing late as long as it's not shifting content around in position. Also, I think Mike's feedback on bug 1501881 is sufficient so I'll cancel Florian's needinfo.
Flags: needinfo?(florian)
(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #6)
> The triage owner isn't the same as the submodule owner, so it's not really
> "my call". It's likely a mix of UX and Engineering people that should make
> this call (tradeoffs on user experience, performance, maintainability, etc).
> 
> I doubt we would be fine with "accept that it shows up late both in visible
> and hidden scenario" since we've done a fair amount of work to reduce the
> various number of paints during startup.

A lot of the work around improving perceived performance has been in showing primary UI surfaces as soon as possible. This has mostly focused on the main window, but this would also apply to a visible menubar.

The video of the lag on the macOS menubar is not a regression we should take. 

Lazy loading for a hidden menubar is probably fine since it's not visible at startup.
Flags: needinfo?(shorlander)
> A lot of the work around improving perceived performance has been in showing primary UI surfaces as soon as possible. This has mostly focused on the main window, but this would also apply to a visible menubar.
> The video of the lag on the macOS menubar is not a regression we should take. 

Just to double check. This means that if we have a tradeoff (not necessarily in this bug, just in general in my area of work) where l10n takes 50ms, and we can show the UI without strings 50ms earlier, or 50ms later with strings, the better perceived performance is to always go for the latter?
Flags: needinfo?(shorlander)
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend)
Attachment #9023047 - Attachment is obsolete: true
Attachment #9023045 - Attachment is obsolete: true
Attachment #9070391 - Attachment is obsolete: true
Depends on: 1568133
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a06c08bf267
Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod

Brendan - so, the backout happened because I added

<linkset>
  <html:link rel="localization" href="branding/brand.ftl"/>
  <html:link rel="localization" href="browser/branding/sync-brand.ftl"/>
  <html:link rel="localization" href="browser/menubar.ftl"/>
</linkset>

to browser/base/content/macWindow.inc.xul which gets included all around, and in one place, there was no html namespace. It was browser/base/content/hiddenWindowMac.xhtml.

When I added it, the test passes, but the browser doesn't start on Mac with:

0:02.86 GECKO(20102) JavaScript error: resource://gre/modules/Fluent.jsm, line 636: TypeError: res.body is undefined

I assume this is because of https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#5701 ?

Does it mean that we treat browser.xhtml specially, but not browser/base/content/hiddenWindowMac.xhtml ?

Can you help me get the body in there as well?

Flags: needinfo?(gandalf) → needinfo?(bdahl)

The bug is reproducible with this patch on MacOS.

d'uh, I really shouldn't be allowed to set NI in the evening...

The reason it failed was because hiddenWindowMac.xhtml was loaded before browser.xhtml, and it triggered async loading of the 3 FTL files, and then browser.xhtml tried to load the same files but sync.

I added a warning in L10nRegistry to not return from cache if the cached value is a Promise and we want sync - this should help us discover the cases better than with a meaningless body.res is undefined.

Flags: needinfo?(bdahl)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f622156e9ea
Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Depends on: 1571181

Zibi, next time you do Fluent changes that remove stuff we use and cause a "burning tree" on a Saturday morning (bug 1571181), could you please let us know. Thanks in advance.

Regressions: 1571265
Regressions: 1571304
Regressions: 1570857
No longer depends on: 1568133
Blocks: 1291693
You need to log in before you can comment on or make changes to this bug.