1501881 - [meta][Milestone 2] Migrate startup path from DTD to Fluent

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

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

Firefox uses DTD to localize majority of its main user interface.

DTD use is locking the browser in XUL environment and brings the thread of Yellow Screen of Death whenever any error in the localization system hits the runtime.

Because of that we want to migrate all of the UI on the startup path from DTD to Fluent.

Comment 1

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

Attachment: collect-entities.diff

A patch which allows us to dump all entity names referenced during startup.

Comment 2

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

Attachment: entities.txt

list of entity references called during startup

Comment 3

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

Attachment: entities-dedup.txt

deduplicated list of entities used in startup

Comment 4

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

If my analysis is correct, we have 995 entities which are used during the browser startup.
We need to move them to Fluent.

While bug 1441035 is still important for us, because it impacts the overall performance of Fluent, the idea we have for Fluent on startup is to attempt to move *all* of them to be loaded lazily after first paint.

The hypothesis here is that there's only one string that is visible in the UI when the browser is loaded - the placeholder in the url bar. And all of the strings, including this one, can be safely loaded after the first paint.

This would allow us to:
 - start migrating now independently of the work on bug 1441035
 - simplify the startup path by removing l10n till post first paint

In the proposal, we'd move all of the browser.xul code to `data-l10n-id`, but only inject the L10n linkset after firstPaint to trigger documentLocalization.

The main risks here are around keybindings/accesskeys that would get registered after first paint potentially impacting users ability to manipulate the UI from keyboard. This may also pose some unknown risks to accessibility that we'll have to verify against.

mconley, mossop, pike, gijs, jaws - setting NI on you to verify the proposed strategy.

Comment 5

[Dave Townsend [:mossop]]

I think there is more than one string that we'd need in the initial pass, but otherwise this seems sound to me!

Comment 6

[Axel Hecht [:Pike]]

Along those lines, I'm interested in non-default configuration around UI visibility and a11y.

I recall that we used to be able to do videos to record screen rendering, that could be helpful to evaluate impact?

Comment 7

[:Gijs (he/him)]

Without strings a11y labels won't exist, and so it's possible NVDA and other a11y tools would announce to the user that an unlabelled textbox gets focus (for the URL bar), and that'd be bad.

The menu bar toplevel items will likely need translating before first paint in the case where it is visible.

I'm a little concerned that we'd end up sacrificing organization for performance here if we end up needing to put the strings necessary for first paint localization in a special fluent file, away from the semantically related strings that aren't needed before firstpaint.

Also, this approach seems counterproductive (ie significant effort that will then get negated/obsolete) if the eventual goal is to have the fluent strings cached in startupcache with the localization not re-running iff the locales / build hasn't changed.

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #7)
> The menu bar toplevel items will likely need translating before first paint
> in the case where it is visible.

... which, of course, is "always" on macOS...

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 9021913 [details] [diff] [review]
collect-entities.diff

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

It seems to me like you should only care about lookups for *references* to general entities. There are probably only two lookup calls that matter.

::: parser/expat/lib/xmlparse.c
@@ +2324,5 @@
>                                  next - enc->minBytesPerChar);
>          if (!name)
>            return XML_ERROR_NO_MEMORY;
> +        // From XHTML
> +        entity = (ENTITY *)lookup(&dtd->generalEntities, name, 0, 1);

This is a lookup for a resolving a reference to a general entity.

@@ +4348,2 @@
>            declEntity = (ENTITY *)lookup(&dtd->generalEntities, name,
> +                                        sizeof(ENTITY), 1);

This is a lookup for *defining* a general entity, probably from a DTD, not for an entity reference.

@@ +5111,4 @@
>                                 next - enc->minBytesPerChar);
>          if (!name)
>            return XML_ERROR_NO_MEMORY;
> +        entity = (ENTITY *)lookup(&dtd->generalEntities, name, 0, 1);

This is also a lookup for a resolving a reference to a general entity.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> 
> mconley, mossop, pike, gijs, jaws - setting NI on you to verify the proposed
> strategy.

Going to echo a few things here that stick out to me:

1) Depending on the user's configuration or platform, there'll be more than one string displayed initially (the menubar on macOS, or if the user has the menubar enabled on Linux / Windows).

2) I want to hear more about the startup caching effort. To me, that's where the real win is, in the vast majority of cases. The empty or invalidated cache case is, imo, less common. Can you please outline how this interacts with our ideas for startup caching?

Comment 11

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

> Without strings a11y labels won't exist, and so it's possible NVDA and other a11y tools would announce to the user that an unlabelled textbox gets focus (for the URL bar), and that'd be bad.

Agree. We don't want that to happen. I'm wondering if there's a way to communicate to the accessibility tools when the UI is ready to be read.

> The menu bar toplevel items will likely need translating before first paint in the case where it is visible.

I'd like to challenge that statement. Not because I need that to not be true, but just because I really am not sure if it is.

Here are two recordings:
 - Mac OS: https://bugzilla.mozilla.org/attachment.cgi?id=9022691
 - Linux: https://bugzilla.mozilla.org/attachment.cgi?id=9021968

In the linux case, I'd like to avoid the bar changing heights when filled with text, but except of that, I'm not sure if it really looks that bad. In case of MacOS, I'd argue that there's no user perceived difference.
Of course if we were to go for such thing, I'd want to verify with UX, but for now, I'm wondering if based on the videos you still think that that the top level menu has to be translated before the first paint?

> I'm a little concerned that we'd end up sacrificing organization for performance here if we end up needing to put the strings necessary for first paint localization in a special fluent file, away from the semantically related strings that aren't needed before firstpaint.


That's not my intent. My intent is only to control when do we localize browser.xul/xhtml.

Here's the whole patch I used to turn it lazy: https://hg.mozilla.org/try/rev/999fa6535028e5cfdb31a78018501b778c7509de - does it seem to you like a significant sacrifice of organization for performance?

> Also, this approach seems counterproductive (ie significant effort that will then get negated/obsolete) if the eventual goal is to have the fluent strings cached in startupcache with the localization not re-running iff the locales / build hasn't changed.

If we were to develop a DOM model that is quirky for the sake of this effort, and then, once performance is on par, had to refactor again, I'd agree.
But if we can keep introducing the DOM l10n annotations and just control when the initial localization happens, it seems to me like a low effort.

The value is that we'd be able to start migrating startup path already, without having to wait for the caching/C++ rewrites to happen.

Gijs, with the screencasts, my explanation and patch, does it affect your opinion anyhow?

Comment 12

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

(In reply to Mike Conley (:mconley) (:⚙️) from comment #10)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> > 
> > mconley, mossop, pike, gijs, jaws - setting NI on you to verify the proposed
> > strategy.
> 
> Going to echo a few things here that stick out to me:
> 
> 1) Depending on the user's configuration or platform, there'll be more than
> one string displayed initially (the menubar on macOS, or if the user has the
> menubar enabled on Linux / Windows).

Based on the screencasts I produced (and assuming I can easily fix the height shift on linux), do you still think that we need to paint it before first paint?

> 
> 2) I want to hear more about the startup caching effort. To me, that's where
> the real win is, in the vast majority of cases. The empty or invalidated
> cache case is, imo, less common. Can you please outline how this interacts
> with our ideas for startup caching?

I also am interested in the caching effort. I'm not sure if we're in the position to claim that it will be possible to cache localized XUL yet. Olli - does it seem to you like something we can do?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

I agree that delaying the strings on macOS looks like it'd be fine, so that's good!

On Linux, it's a little more jarring to get those strings late. Keeping the height stable would help - though we might want to run it by UX just to be sure.

Based on those videos, if UX feels okay with it, and a11y feels good with it, then I'm good with it. :)

Comment 14

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

> On Linux, it's a little more jarring to get those strings late. Keeping the height stable would help - though we might want to run it by UX just to be sure.

We could also translate the strings eagerly vs. lazily depending on if the menubar is visible or not.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

IMO that MacOS behavior looks ok. Linux less so if the menubar size changes.

I think caching XUL would require that we don't modify the document before localization has occurred.
It isn't clear to me whether that is ok requirement/limitation.

And need to keep XHTML caching in mind too.
How much do we want browser.xhtml development to look like normal web page development?

Comment 16

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> > Without strings a11y labels won't exist, and so it's possible NVDA and other a11y tools would announce to the user that an unlabelled textbox gets focus (for the URL bar), and that'd be bad.
> 
> Agree. We don't want that to happen. I'm wondering if there's a way to
> communicate to the accessibility tools when the UI is ready to be read.

I'm afraid I don't know, :Jamie or :marcoz or :surkov may have better answers.

> > The menu bar toplevel items will likely need translating before first paint in the case where it is visible.
> 
> I'd like to challenge that statement. Not because I need that to not be
> true, but just because I really am not sure if it is.
> 
> Here are two recordings:
>  - Mac OS: https://bugzilla.mozilla.org/attachment.cgi?id=9022691

I agree this looks fine. I think we'll need to make sure that someone like spohl or mstange agrees this is workable in terms of how it works with cocoa widget things, I'm not the best person to give you guarantees there. It'd also be useful if you doublecheck that the preferences item and "about Firefox" in the app menu works (because at least historically, the items that end up in the app menu have... interesting... interactions with widget code in order to make it there), both directly and from the shortcut (for prefs), and what happens to the hidden window menus (ie if you close the browser window but not the app).

> > I'm a little concerned that we'd end up sacrificing organization for performance here if we end up needing to put the strings necessary for first paint localization in a special fluent file, away from the semantically related strings that aren't needed before firstpaint.
> 
> 
> That's not my intent. My intent is only to control when do we localize
> browser.xul/xhtml.
> 
> Here's the whole patch I used to turn it lazy:
> https://hg.mozilla.org/try/rev/999fa6535028e5cfdb31a78018501b778c7509de -
> does it seem to you like a significant sacrifice of organization for
> performance?

No, this seems fine organizationally, so that's great. I'm a bit confused because your comment earlier specifically called out the url bar string so I sort of assumed you wanted to put stuff we need "immediately" in one file, and everything else somewhere else. Anyway, if we can keep using whatever logical ftl files we'd use to separate strings from different parts/components, irrespective of how soon they're visible, I'm fine.

On that note though, I'm a bit worried about the idle task here. I'll defer to Florian or :mconley, but while this clearly gets the l10n stuff processed "soon" after the window opens on your high-performant machine, "idle" might take... a while... on other machines. Would be interesting to see this on the windows 2018 reference device, with session restore turned on or an "interesting" homepage (I dunno, yahoo news), for instance. I can try it out myself tomorrow but I don't have screen recording stuff set up on the machine, so not sure I can get a screencast. I'll have a look - assuming this works as-is on Windows? Or was it not a coincidence there was no Windows screencast? :-)

We could consider setting a time limit on the idle task for this particular case, but it's also possible we may need to use something other than an idle task. I just think we should be aware that they have downsides, for something user-facing / perceived-performance-sensitive like this, where "sometime after startup has died down" is a bit loose.

> > Also, this approach seems counterproductive (ie significant effort that will then get negated/obsolete) if the eventual goal is to have the fluent strings cached in startupcache with the localization not re-running iff the locales / build hasn't changed.
> 
> If we were to develop a DOM model that is quirky for the sake of this
> effort, and then, once performance is on par, had to refactor again, I'd
> agree.
> But if we can keep introducing the DOM l10n annotations and just control
> when the initial localization happens, it seems to me like a low effort.

Yeah, I agree, I was worried about this in terms of it affecting what files we put strings in. This plan seems fine, though we need to come up with a direction for the xul cache / fastload stuff. I'm not sure who's working on that and I don't want to stomp on toes, but while we can keep ignoring it for a while longer, sooner or later we'll have to tackle it one way or another. :-)

> The value is that we'd be able to start migrating startup path already,
> without having to wait for the caching/C++ rewrites to happen.

Yeah, this makes sense to me, assuming our perf metrics are happy and we sort out the rest of the details here.

> Gijs, with the screencasts, my explanation and patch, does it affect your
> opinion anyhow?

Yes, I'm more positive, though I still think we should iron out the details before "plumping" for it - if we have to back this type of stuff out "later" I assume it's going to be a pain for localizers.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, this looks nice. Are there strings which are not visible but are used by platform code and are needed at startup? The example I have in mind are XML parsing errors.

Comment 18

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

Attachment: WIP: Bug 1501881 - Instrument the build to dump L10n IDs during startup for DTD/Properties/FTL.

Comment 19

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

Hi Peter,

I updated my patch to dump data in a better structure and applied your feedback from while ago.

In .properties I was able to get the stack to the call that triggers the call, and I expect to be able to find a similar stack for Fluent.

Can you recommend a way to get a document and DTD file for the entity? Is it possible in this parser?

Comment 20

[:Gijs (he/him)]

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

Hi Peter,

I updated my patch to dump data in a better structure and applied your feedback from while ago.

In .properties I was able to get the stack to the call that triggers the call, and I expect to be able to find a similar stack for Fluent.

Can you recommend a way to get a document and DTD file for the entity? Is it possible in this parser?

Not 100% sure if this is what you want, but I believe https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/parser/htmlparser/nsExpatDriver.cpp#530 is a consumer callback called by expat for every dtd file referenced from a document that it's parsing. Note that they're loaded as soon as they're referenced, not as soon as we get an entity for which the DTD is needed (which in any case we can't know, ie how would the parser know which of the DTDs has the relevant entity?). OpenInputStreamFromExternalDTD, which is called from there, has some handy local variables that I think get you what you want here? There are some different cases but the loading / triggering principal should either end up being system, or null, or have the document's URI. Off-hand I don't know how many of the loads on startup would use a document (I mean, I'd hope all of them, but who knows).

Does that help?

Comment 21

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

Does that help?

Yes! I was able to use it to get DTD and XML paths for the documents! Thank you!

Comment 22

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

I morphed this bug into what we call "Milestone 2" - making the whole startup path use Fluent and spun-off "Milestone 1" - making browser.xhtml use Fluent (bug 1579477).

Comment 23

[Eemeli Aro [:eemeli]]

The list of bugs blocking this should now be up to date, at least according to the most recent arewefluentyet M2 data.

There are additionally two bedrock bugs blocking this, due to showing the privacy policy on first load:

• mozilla/bedrock#11058: Specify sameSite property on cookies set by JS
• mozilla/bedrock#11326: Styles should not use the non-standard zoom property

Comment 24

[Eemeli Aro [:eemeli]]

With the completion of the DTD migration in bug 1786543, the benefits of the remaining part of the M2 goal (migration of .properties strings formatted during startup) is not really distinguishable from the benefits of migrating all .properties strings to Fluent, i.e. the remainder of M3.

To that end, I'm thinking of closing this as WONTFIX and seeking other ways of separating out other parts of the M3 change to provide intermediate goals, such as bug 1790189. This would allow us to simplify the arewefluentyet scripting quite a bit, and to better align the nominal goals with our actual practice.

If you have any concerns about this, please voice them here. I'm particularly interested in hearing if there's something I've missed, and that there would be a real-world benefit to getting rid of all .properties/StringBundle formatting during startup.

Comment 25

[:Gijs (he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #24)

With the completion of the DTD migration in bug 1786543, the benefits of the remaining part of the M2 goal (migration of .properties strings formatted during startup) is not really distinguishable from the benefits of migrating all .properties strings to Fluent, i.e. the remainder of M3.

To that end, I'm thinking of closing this as WONTFIX

FWIW rather than wontfix I'd argue in favour of moving the goalpost of this milestone back to just DTD and resolving this fixed (which is certainly how the bug originally got filed, see comment 0 which only mentions DTDs). We're not intentionally refusing to fix .properties things even if someone were to provide patches (which is how I think we try to treat WONTFIX), we're just most of the way done here.

and seeking other ways of separating out other parts of the M3 change to provide intermediate goals, such as bug 1790189. This would allow us to simplify the arewefluentyet scripting quite a bit, and to better align the nominal goals with our actual practice.

If you have any concerns about this, please voice them here. I'm particularly interested in hearing if there's something I've missed, and that there would be a real-world benefit to getting rid of all .properties/StringBundle formatting during startup.

The only benefit I'm aware of for the .properties usage is cached stringbundles and their effect on runtime language switching (ie they'll show whatever got cached on startup, so you get a mixed-language app until you restart). This is a bit more pronounced during startup than "stuff we load later", because the startup path is guaranteed to have run by the time we prompt the (new) user if their Fx locale differs from their OS locale during onboarding. As a result, it may be worth focusing on .properties file usage in that startup path to make that experience better. But that's a pretty limited justification.

An alternative would be focusing on an automated transformation from .properties to sync fluent use (potentially manually converting PluralForm use), which would remove the API surface completely and tidy up the loose end that way, but would leave potentially-undesirable/un-ergonomic fluent use when it should have been moved into markup or used async APIs. I'm not sure if this has been discussed already and rejected for reasons I'm not aware of.

Comment 26

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

I understand the M2 is costly to produce as it is hard to automate. My quiet hope was that we'll get to finish M2 rather than deprecate the milestone.
As Gijs pointed out, there are characteristics that are specific to startup that disproportionally affect UX.

If you were to reconsider addressing M2 rather than closing it:

• The number 220 is misleading as many calls are repeated. The actual number of callsites is likely much lower (~50?)
• Bug 1733498 is by far the biggest chunk of the remaining 220 .properties calls. If you were successful in moving this, I'd expect the picture to look much cleaner

If you decide to close M2, I'm comfortable with marking it as FIXED since DTDs have been removed, and as you suggested, figuring out between focusing on automated migrations, or migrating pluralized strings first.

Comment 27

[Francesco Lodolo [:flod]]

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

I understand the M2 is costly to produce as it is hard to automate. My quiet hope was that we'll get to finish M2 rather than deprecate the milestone.

Besides the calls being repeated, when I tried locally I got results completely different from the existing data generated by Eric. Not sure if that was caused by using a different platform, or if it depends on how long you wait before closing the browser (since calls are duplicated)?

Dropping M2 will likely allow us to generate the data in GitHub actions, unless we hit space limitations when cloning the unified repo. Worst case scenario, it's easy to reproduce on different machines.

Comment 28

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

Besides the calls being repeated, when I tried locally I got results completely different from the existing data generated by Eric. Not sure if that was caused by using a different platform, or if it depends on how long you wait before closing the browser (since calls are duplicated)?

I think it's platform dependent. I remember getting different results between Mac and Linux. I don't remember them to be "completely" different, but rather ~10-20% off which I accounted for larger stdev/noise.

Comment 29

[:Gijs (he/him)]

There are probably especially differences caused by macOS having to open a "real" hidden window (for the menubar that gets shown when all other windows closed but the app stays open), as well as just platform differences generally given different OS-specific code may need their own locale stuff - though I'd expect Windows and Linux to match more closely.

Comment 30

[Erik Nordin [:nordzilla]]

I don't have a strong preference either way regarding stopping or continuing to update the M2 milestone.

While the M2 milestone is certainly harder to automate, it doesn't take long to run on my desktop computer, and is something that I can easily do in the background while working on other tasks. In that regard I have no issue continuing to manually update the milestones, though I understand that the ability to fully automate the process would be desirable.

There was a gap in arewefluentyet recently because I was out of town without my computers, but it is now back up to date as of this week.

Whatever we decide here is fine with me.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

I'm aligned with Gijs here.

Comment 32

[Eemeli Aro [:eemeli]]

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

FWIW rather than wontfix I'd argue in favour of moving the goalpost of this milestone back to just DTD and resolving this fixed (which is certainly how the bug originally got filed, see comment 0 which only mentions DTDs). We're not intentionally refusing to fix .properties things even if someone were to provide patches (which is how I think we try to treat WONTFIX), we're just most of the way done here.

That does sound more appropriate than what I initially suggested.

The only benefit I'm aware of for the .properties usage is cached stringbundles and their effect on runtime language switching (ie they'll show whatever got cached on startup, so you get a mixed-language app until you restart). This is a bit more pronounced during startup than "stuff we load later", because the startup path is guaranteed to have run by the time we prompt the (new) user if their Fx locale differs from their OS locale during onboarding. As a result, it may be worth focusing on .properties file usage in that startup path to make that experience better. But that's a pretty limited justification.

It's also not a guaranteed benefit from a Fluent transition, unless that's going to DOM localization. In code that's calling Localization formatting methods directly, we may need to listen to intl:app-locale-changed events and reformatting then as necessary. I'm sure this isn't currently done everywhere that it ought to be.

An alternative would be focusing on an automated transformation from .properties to sync fluent use (potentially manually converting PluralForm use), which would remove the API surface completely and tidy up the loose end that way, but would leave potentially-undesirable/un-ergonomic fluent use when it should have been moved into markup or used async APIs. I'm not sure if this has been discussed already and rejected for reasons I'm not aware of.

In practice I've found that at least for the .properties we still have, the shape of the migrated Fluent API is rather different. In JS it's because in these cases we're often able to go to DOM localization, and in C++ it's because the APIs are just rather differently shaped, esp. when variables/placeholders are involved. There's also the matter that we don't really get any localiser benefit from such automated transitions. My current thinking is that we ought to be able to build the lower-level Intl.MessageFormat/MF2 APIs with this use case in mind, and be able to do a much more automated transition to that, rather than Fluent.

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

If you were to reconsider addressing M2 rather than closing it:

• The number 220 is misleading as many calls are repeated. The actual number of callsites is likely much lower (~50?)

I last looked at M2 in detail in March; back then we were down to 32 unique .properties strings in 14 files, with a pretty even split on ones being formatted from only JS, only C++, and both. I think we've since been able to drop that by about 6 strings. The easy work here has been done; the remainder is a slog. The M2 definition is also a bit fuzzy around the edges as different platforms produce slightly different results, and has odd dependencies like this. So chasing this tail to its completion is expensive, and doesn't really bring all that much benefit.

• Bug 1733498 is by far the biggest chunk of the remaining 220 .properties calls. If you were successful in moving this, I'd expect the picture to look much cleaner

Thanks for reminding me about that; seeing now if the abandoned patch for that might still land one day.