Closed Bug 1600528 Opened 3 months ago Closed 2 months ago

Migrate browser-context.inc to Fluent

Categories

(Firefox :: General, task, P1)

task

Tracking

()

RESOLVED FIXED
Firefox 73
Tracking Status
firefox73 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This bug is about migrating https://searchfox.org/mozilla-central/source/browser/base/content/browser-context.inc to Fluent.

It's a pretty tricky task since a lot of those strings are re-used around the browser UI codebase, but I think I have a path to bring up a clean design that will handle l10n and UI needs in a flexible way.

Blocks: 1579477
Assignee: nobody → gandalf

This is just the first two elements, but I think it's a good time to discuss whether the approach will work. NI'ing Stas, Flod, Axel and Gijs to get your perspectives because this feels like a bit more advanced use of Fluent features onto complex DOM, so want to work out a plan that looks good to all of us before I move forward.

Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francesco.lodolo)

I guess you found a couple of bugs with the linter:

  • As far as I can tell, the content can be parsed correctly.
  • The output with multiattributes messages is confusing.

Waiting for Axel to take a look there before filing bugs.

Going back to the patch, some questions. Using the bug, since the DIFF is already confusing with all the linter comments.

  1. Why should they be defined as terms? Terms have private attributes, and I'm not sure we want them.

  2. Right now there are 8 messages connected to the back/forward. What happens if one of the messages has an extra attribute (e.g. aria-label)? If that's not a problem, we could use the toolbar generic elements in the navigation.

## Toolbar Navigation Elements

toolbar-back =
    .aria-label = Back
    .label = Back
    .accesskey = B
    .tooltiptext = Go back one page

toolbar-forward =
    .aria-label = Forward
    .label = Forward
    .accesskey = F
    .tooltiptext = Go forward one page

## Navigation Bar Tooltips

navbar-tooltip-back =
    .value = { toolbar-back.tooltiptext }

navbar-tooltip-forward =
    .value = { toolbar-forward.tooltiptext }

## Toolbar Customization

toolbar-button-back =
    .label = { toolbar-back.label }

toolbar-button-forward =
    .label = { toolbar-forward.label }
  1. navigation-menu items have an accesskey but no label?
Flags: needinfo?(francesco.lodolo)

Yeah, Terms don't work this way. Given the patch doesn't work, I'll clear needinfo's on the other folks.

We could use message value and attribute references for consistency, though. Or re-use the data-l10n-id on multiple elements, but that requires that all attributes can be consistently set from the DOM bindings.

Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(gijskruitbosch+bugs)

The patch looks like it's trying to DRY the Fluent file, what with all the references to common terms. However, in my mind Fluent is much more WET (write everything twice) than DRY. I would probably try to repeat "Back" and "Forward" in the target messages as many times as needed.

If you decide to DRY it nonetheless, flod's version with message references is one way to go about it. It has the advantage of making it possible for tooling to verify that all attributes are present. It also has the downside of forcing these messages to exist at all, while some locales might choose to WET the target messages (navbar-tooltip-back, navbar-menu-back, etc.).

Alternatively, you could create multiple terms, one for each common phrase:

-toolbar-back-aria-label = Back
-toolbar-back-label = Back
-toolbar-back-accesskey = B
-toolbar-back-tooltiptext = Go back one page

(Terms with attributes don't work here because it's not possible to reference {-term.attr} inside a sentence; term attributes can only be used as selectors. This is done on purpose to prevent terms with attributes from being used as hashmaps of localizable values which wouldn't be checked by tooling.)

(In reply to Francesco Lodolo [:flod] from comment #3)

  1. Right now there are 8 messages connected to the back/forward. What happens if one of the messages has an extra attribute (e.g. aria-label)? If that's not a problem, we could use the toolbar generic elements in the navigation.

I don't know if having both a label and aria-label would matter in the cases where a label is already present, but I wouldn't be surprised if it mattered for the context menu case (where a label is not currently present because it's an icon), and I don't know what'd happen if we ended up with both a tooltiptext attribute and a tooltip attribute pointing elsewhere. I'd prefer to avoid that confusion.

  1. navigation-menu items have an accesskey but no label?

They have an icon, and an aria-label for accessibility.

(In reply to Francesco Lodolo [:flod] from comment #3)

I guess you found a couple of bugs with the linter:

  • As far as I can tell, the content can be parsed correctly.
  • The output with multiattributes messages is confusing.

Reporting from Slack. The latter still stands, but the linter is correct:

  1. Terms must have values
  2. Term's attributes may not be interpolated in patterns (i.e. { -term.someattribute} in message is not allowed).

Flod: I am concerned about my ability to continue migrating at any pace, if I'll have to introduce side effects to migrations that can lead to regressions or require additional tests updates. I don't understand every single UI piece in Firefox and I don't know what side effect adding an attribute would bring. I'd like to explore options which don't require changing the shape of elements in result of the migration.

Pike: We could use message value and attribute references for consistency, though.

Yep, that was my original intent, I changed it to term last second, and didn't test that it doesn't work :)

Stas: The patch looks like it's trying to DRY the Fluent file, what with all the references to common terms. However, in my mind Fluent is much more WET (write everything twice) than DRY. I would probably try to repeat "Back" and "Forward" in the target messages as many times as needed.

That's a main concern for me. I understand the WET/DRY and where you place Fluent on that scale, but I believe that this is a case where it's not a DRY vs WET. It's about consistency - which is exactly what Terms were introduced for.
Yes, we can have:

toolbar-item-back = Back
menuitem-back = Back
tooltip-back = Back
context-item-back = Back

toolbar-desc-back = Go back one page
menuitem-desc-back = Go back one page
context-desc-back = Go back one page

and translation tools would likely reduce the burden on localizers, but IMHO this is the wrong tradeoff. You're ending up with 3 units to describe the same thing that is meant to be one thing.
The real reason we at all have to deal with it is not that we want to have 3 versions of the Go back one page (which would be WET), it's because our DOM bindings are per-element-shape, and since the same content ends up in 3 shapes, we need 3 messages to bind them to the elements.

In my opinion its the right balance, but I wouldn't say that the goal is DRY.

Alternatively, you could create multiple terms, one for each common phrase:

We could! I think that such approach would be giving up on the value of compound messages tho. Separating accesskey from label means that potentially they can end up in two different locales, it's harder to write comments (semantic in the future?) for the "back" unit (group comments could help, but less optimal) and we're generally back to id-string pairs which for DOM are suboptimal.

I'll update the patch to use a message, but I'm open to alternative proposals. In the end I do believe that the decision should be between the 4 of you, and not mine.

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

Flod: I am concerned about my ability to continue migrating at any pace, if I'll have to introduce side effects to migrations that can lead to regressions or require additional tests updates. I don't understand every single UI piece in Firefox and I don't know what side effect adding an attribute would bring. I'd like to explore options which don't require changing the shape of elements in result of the migration.

I understand the concern, but I also believe speed of migration shouldn't be the primary measure of success.

Is there anyone who would know for sure if these changes have an effect?

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

  1. navigation-menu items have an accesskey but no label?

They have an icon, and an aria-label for accessibility.

Right, but what's the practical use of an accesskey in that case? How can it be used?

I understand the concern, but I also believe speed of migration shouldn't be the primary measure of success.

It should be one of the measures. There's a real cost of maintaining the 3-l10n-formats system in Gecko for 2 years now. There's also a real cost of delaying the features that full switch to Fluent enables.

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

That's a main concern for me. I understand the WET/DRY and where you place Fluent on that scale, but I believe that this is a case where it's not a DRY vs WET. It's about consistency - which is exactly what Terms were introduced for.

Consistency comes in different colors and shapes. I think of terms primarily as a tool to enforce the project-wide consistency of branding. Local-scale consistency is often better served by just writing the actual word inline and relying on external tools. I wouldn't want all translations to suddenly start using {-tab(case: "lower")} everywhere where we use the word "tab".

Yes, we can have:

toolbar-item-back = Back
menuitem-back = Back
tooltip-back = Back
context-item-back = Back

toolbar-desc-back = Go back one page
menuitem-desc-back = Go back one page
context-desc-back = Go back one page

and translation tools would likely reduce the burden on localizers, but IMHO this is the wrong tradeoff. You're ending up with 3 units to describe the same thing that is meant to be one thing.

I see your point. You're right that there's a trade-off involved and I think it's great that we're seeing these examples in real-life production code. It's how the good practices form :) I've learnt to be cautious about deciding that something "is the same thing"; it might be true here, but I've also seen enough of cases when there was a language for which my assumption didn't hold. I understand, however, that by using references localizers are still in control, because they can drop the placeable and use a hardcoded translation if needed. (We don't force the same references to be present in translations, do we?)

The real reason we at all have to deal with it is not that we want to have 3 versions of the Go back one page (which would be WET), it's because our DOM bindings are per-element-shape, and since the same content ends up in 3 shapes, we need 3 messages to bind them to the elements.

This has been on my mind a lot lately. I'd like to look for a way to decouple Fluent messages from the source structure of the DOM. For instance, what if you could re-use the same message id in all cases, and map Fluent attributes to DOM attributes differently for each?

toolbar-back =
    .label = Back
    .accesskey = B
    .tooltip = Go back one page
# An API proposal; it doesn't work that way right now.
<menuitem data-l10n-id="navbar-menu-back" data-l10n-attrs="aria-label:label accesskey tooltiptext:tooltip">
<description data-l10n-id="navbar-tooltip-back" data-l10n-attrs="value:tooltip">
<toolbarbutton data-l10n-id="toolbar-button-back" data-l10n-attrs="label">

I filed https://github.com/projectfluent/fluent.js/issues/437 to discuss this further.

Alternatively, you could create multiple terms, one for each common phrase:

We could! I think that such approach would be giving up on the value of compound messages tho.

In my mind, we'd use a flat list of terms to define a glossary, and then use compound messages to map the glossary onto DOM elements:

-toolbar-back-label = Back
-toolbar-back-accesskey = B
-toolbar-back-tooltiptext = Go back one page

navbar-menu-back =
    .aria-label = { -toolbar-back-label }
    .accesskey = { -toolbar-back-accesskey }
    .tooltiptext = { -toolbar-back-tooltiptext }

Separating accesskey from label means that potentially they can end up in two different locales

They're only separate as terms. The message falls back as a whole.

it's harder to write comments (semantic in the future?) for the "back" unit (group comments could help, but less optimal)

We have group comments for that :)

I wouldn't want all translations to suddenly start using {-tab(case: "lower")} everywhere where we use the word "tab".

Neither would I.
But this is not such a case. We have a single UI widget - a back button. It has multiple "real" widgets because that's how the toolbar is built. If you'd diverge the translation, you wouldn't and up with Back1 in one UI, and Back2 in another. You'd end up with Back1 and Back2 in the same UI widget, just in different "modes" (customize vs. regular).

That makes me feel that it should be treated like a single localization unit.

I filed https://github.com/projectfluent/fluent.js/issues/437 to discuss this further.

Ugh. That feels very crowded and dense. I'll defer to Gijs opinion here, but my initial take is that listing attribute mappings (which we did in XBL!) is less friendly for maintaining than my current patch.

They're only separate as terms. The message falls back as a whole.

Right, good point.
I struggle with the idea that accesskey and label of a back button are two separate translation units (terms).

We have group comments for that :)

I feel that it would require us to redefine how we use group comments.

So far we used groups to cluster a large pool of entries that are semantically related. Examples:

Here, we'd add 50 group comments for 50 UI elements. See https://searchfox.org/mozilla-central/source/browser/base/content/browser-context.inc - each <menuitem> will be expanded to something similar to what in my patch is the back and forward` buttons.

I would prefer to preserve Group Comments for a group of "context menu elements", rather than group for "back", group for "foward" and soon.

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

That makes me feel that it should be treated like a single localization unit.

That's a fair point, in particular about the customize vs. regular mode. I could disagree on the context menu one, but I'm not overly opinionated about it. On a larger scale, I think this highlights the fact that sometimes it's hard to draw the line and to define practices which should be followed 100% of the time.

Ugh. That feels very crowded and dense. I'll defer to Gijs opinion here, but my initial take is that listing attribute mappings (which we did in XBL!) is less friendly for maintaining than my current patch.

Let's have this discussion on GitHub, please.

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

I understand the concern, but I also believe speed of migration shouldn't be the primary measure of success.

It should be one of the measures. There's a real cost of maintaining the 3-l10n-formats system in Gecko for 2 years now. There's also a real cost of delaying the features that full switch to Fluent enables.

I want to chime in and strongly agree with this. We:

  • still have other roadblocks wrt fluent use (e.g. UA widgets, early startup / initialization issues, etc.)
  • can easily adjust fluent code after the migration because we have fluent-to-fluent migration recipes; we don't have that for properties/dtd themselves
  • will continue to struggle "imposing" that no new DTD strings should be added as long as there are large chunks of existing DTD-based code, which will further delay/complicate switching away from DTDs as long as it continues to be the case.

so I strongly believe we should err on the side of converting faster and (perhaps occasionally) less perfectly, over trying to come up with perfect l10n structures every time.

(In reply to Francesco Lodolo [:flod] from comment #9)

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

  1. navigation-menu items have an accesskey but no label?

They have an icon, and an aria-label for accessibility.

Right, but what's the practical use of an accesskey in that case? How can it be used?

The same way as usual? I just checked, they work...
I think they were kept for muscle memory. Perhaps it'd be nice if the tooltip indicated the access key, but that's an orthogonal problem.

I disagree, or at least there should be a middle ground. We're touching things that, in same cases, are the result of choices made over a decade ago. If we decide it's not useful to try improve things at this point, it's naive to think that they will be improved afterwards.

Also, note that I haven't said "we need to change this at all costs", I've asked "can someone tell what happens if we do X?". If the answer is no, then we need to keep things as they are, and so be it.

(In reply to Staś Małolepszy :stas from comment #11)

In my mind, we'd use a flat list of terms to define a glossary, and then use compound messages to map the glossary onto DOM elements:

-toolbar-back-label = Back
-toolbar-back-accesskey = B
-toolbar-back-tooltiptext = Go back one page

navbar-menu-back =
    .aria-label = { -toolbar-back-label }
    .accesskey = { -toolbar-back-accesskey }
    .tooltiptext = { -toolbar-back-tooltiptext }

I'm not a fan of this approach. It could help with consistency, but it seems like an abuse of terms. At that point I'd prefer to avoid references completely and just have the plain text repeated as value.

(In reply to Francesco Lodolo [:flod] from comment #16)

I disagree, or at least there should be a middle ground. We're touching things that, in same cases, are the result of choices made over a decade ago. If we decide it's not useful to try improve things at this point, it's naive to think that they will be improved afterwards.

I mean, that's clearly not true elsewhere in Firefox or we would never refactor or improve code quality anywhere? I'm not sure why you think we won't improve FTL code once it's landed.

Also, note that I haven't said "we need to change this at all costs", I've asked "can someone tell what happens if we do X?". If the answer is no, then we need to keep things as they are, and so be it.

(In reply to Francesco Lodolo [:flod] from comment #9)

Is there anyone who would know for sure if [adding additional attributes to elements has] an effect?

At a minimum, they'd cause confusion for Firefox developers ("why do we have both label and aria-label here, that makes no sense?"), and they could potentially cause bugs. Whether there'd be bugs is something that'd need checking on a case-by-case basis (for visual effects on all 3 desktop platforms, and for a11y/AT effects on all 3 desktop platforms), and that doesn't scale. We should just avoid doing it.

As I said in github, having fluent messages be DOM-shaped is a design choice that's part of Fluent. I think we've passed the point where we can make a different choice within Fluent. We've run into it before and I don't understand why this bug suddenly means we need to re-evaluate it.

This bug derailed quite a bit, I think.

Trying to get back to the actual code in question:

  • Terms as complex messages doesn't work, or, well, at least shouldn't
  • Complex messages with message references work in code, though there's a bit of argument of what's the master message, probably
  • Full scope single message might work, but not clear if there are unintentional consequences
  • Terms as simple entries work, but don't compound well
  • Individual compound messages for each use are soaking WET

Looking at how we expose these on Pontoon to localizers, it seems that we're in a hard place. One nice thing about the current DOM mapping is that Pontoon can help with the choice of accesskeys.
Sadly, that help is in the way when we use message references. See the attached screenshot I did on a test project on Pontoon.

Using individual terms would expose the same UI problem as in the screenshot, and not add supportive UI for the accesskey in the actual term.

Best support would exist for the single full-scope message.

Given we're talking 4 buttons that are shared in this way, I'm leaning towards one full message.

I wonder how the HTML version of this problem will look like, though.

Thanks for the summary, Axel. I feel like I've helped derail this bug, sorry about that. I'd like to cast a vote one more time towards the WET approach. All this time we've been talking about 3 messages for "Back" and 3 messages for "Forward". That's not a lot of repetition, and IIUC the authoring experience for the localizers would be best if they're repeated three times each. Otherwise, it does sound like we're trying to over-optimize this by factoring the common part out, which also makes the code harder to follow for developers, reviewers and localizers alike.

(In reply to Axel Hecht [:Pike] from comment #19)

Given we're talking 4 buttons that are shared in this way, I'm leaning towards one full message.

As I just pointed out on slack, there is no way not to have all 4 items end up with all of the FTL attributes though (label, aria-label, accesskey, tooltiptext) - and we do not want to do that.

Really, just the cleanliness aspect of having 2 bogus attributes is enough reason, but practically speaking, in this case, based on some quick testing I just did, the tooltiptext attribute (with a string to use as the text for the default tooltip) that we'd then add to the back-button would override the separate tooltip attribute (which points to a node to use as a tooltip), so this would actually break behaviour.

Thank you all for contributing your perspectives. I think it's a challenging, but very valuable part of the work we're doing.

Ultimately, it seems like it comes to personal preferences, and as the patch author I casted mine. So far, I have not seen an argument that would make me change my view that this is the locally optimal approach to migrating this piece of code right now.

We have a number of options:

  1. I can finish the patch based on the approach in my PR, get it reviewed by you and land.
  2. I can follow stas' proposal to not use any references and place the actual strings in every message separately, finish the patch, get reviewed and land.
  3. Someone can provide an alternative approach to those 2 strings that they verify works, doesn't cause regressions and I can scale it, get reviewed and land.
  4. We can mark this file as not ready to be migrated to Fluent and I will move on to migrate other pieces of browser.xhtml.

I want to point out that (4) would have a pretty severe consequences on our claims about Fluent readiness and timeline estimations.

I understand that this is challenging, and that each option presented here is not a perfect one.

My question is - can we come to conclusion on which of those approaches (or yet another one if I missed something) should we apply here based on the arguments posted in this bug so far?

Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francesco.lodolo)

Personally I prefer option 2). No references, just plain messages.

Flags: needinfo?(francesco.lodolo)

I second 2).

My primary reason at this point is that it allows us to migrate the code to HTML in whichever order we choose. If we tie message shapes of one part of UI to message shapes in others, the conversion to HTML will be more complicated.

Flags: needinfo?(l10n)

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

Ultimately, it seems like it comes to personal preferences, and as the patch author I casted mine. So far, I have not seen an argument that would make me change my view that this is the locally optimal approach to migrating this piece of code right now.

What's your take on the screenshot Axel posted in comment 18? I know that in the long term the issue should be fixed in Pontoon, in particular the UI for the accesskey, but right now it is what it is. Does it change your decision somehow?

It also appears that the currently implemented solution suffers from the problem which Gijs pointed out on GitHub: accesskey should be context-unique, making the case for "shared" accesskeys problematic.

What's your take on the screenshot Axel posted in comment 18?

Yes. I misread Axel's comment as a bug when handling term, not message references. I filed bug 1601002 to handle that.
If we can fix it in Pontoon within a short time, and that would be the only thing standing in the way, I'd be ok blocking this bug on it, but it seems like Axel and Flod prefer (2) independently of that issue (?).

Does it change your decision somehow?

Yes.

It also appears that the currently implemented solution suffers from the problem which Gijs pointed out on GitHub: accesskey should be context-unique, making the case for "shared" accesskeys problematic.

The accesskeys were shared in DTD, and in my approach in Fluent they can be decoupled per case, per locale as needed. Do you think this is not enough?

One more consideration for (2):

In (1), we have:

toolbar-forward =
    .tooltiptext = Go forward one page
    .label = Forward
    .accesskey = F

navbar-tooltip-forward = 
    .value = { toolbar-forward.tooltiptext }

navbar-menu-forward =
    .tooltiptext = { toolbar-forward.tooltiptext }
    .aria-label = { toolbar-forward.label }
    .accesskey = { toolbar-forward.accesskey }

toolbar-button-forward =
    .label = { toolbar-forward.label }

One day, we'll need to update the term Forward in this button to a new one from UX - say Fwd or Progress.

If that happens, in this model, the developer would:

  1. Find the data-l10n-id in the DOM
  2. Find the string in the FTL file
  3. Notice that it references another string
  4. Update all strings that reference toolbar-forward

That's actually similar to what you'd do today with the DTD and references.

But if we go with (2):

toolbar-forward =
    .tooltiptext = Go forward one page
    .label = Forward
    .accesskey = F

navbar-tooltip-forward = 
    .value = Go forward one page

navbar-menu-forward =
    .tooltiptext = Go forward one page
    .aria-label = Forward
    .accesskey =F

toolbar-button-forward =
    .label = Forward

then, the steps are:

  1. Find the data-l10n-id in the DOM
  2. Find the string in the FTL file
  3. Update it
  4. Notice that you're updating only one of multiple related messages
  5. Update all of them, but not unrelated ones that also have Forward in their name

This seems like a fragile model that may lead to inconsistent code, and I can't unfortunately say that stylistic changes are uncommon in the main browser toolbar.

I can see an argument that localization is not intended to help developers understand their DOM structure tho :)

But we could still try with (2). We could add group comment per item (Forward, Back and so on) helping to visually recognize a cluster of messages related to the same item.

And then, since we don't have GroupGroupComment, maybe we should extract browser-context.inc to a new FTL file that only holds translation units for those buttons?

Gijs: are you ok with such a tradeoff?

  1. Update all strings that reference toolbar-forward

Just to be explicit here, that's going to rev the string IDs for each string that refers to the toolbar master string.

And to come back to my argument, that's also needed if we just change .label to .value in an HTML refactor.

Yes! Thank you for clarifying the message!

The difference I see is that in the DTD model and proposal from my PR l10n de facto bonds the related items making the references easy to grep for in searchfox and even if you'd forget, fluent would report errors in the resolver (and fail on try I think) making it easier to avoid mistakes.

In the (2) we need a group comment and hope for the dev to ay attention.

Priority: -- → P1

The PMs in l10n-drivers are running a terminology and copy project together with the UX team. We'll build on industry standard tools for that, and I think having literal text is going to be more helpful to those tools than Fluent message or term references.

Flags: needinfo?(stas)

I have a preference for (1) because it's DRY-er.

I'm a bit confused about the issues with external tools (comment #30) and Pontoon - we've used this approach before / elsewhere with Fluent, though perhaps the extent of it was smaller then (e.g. https://searchfox.org/mozilla-central/source/toolkit/locales/en-US/toolkit/printing/printPreview.ftl#10 ). But in any case, any parsing tools will have to deal with message references and the full Fluent syntax regardless of our decision here (e.g. for terms for brand names, or variable usage). So I'm not super convinced by those objections.

I also don't know almost anything about Pontoon so the screenshot from comment #18 doesn't really make sense to me - what is it showing? The text context in comments here and the bug Zibi filed indicates that it shows that Pontoon doesn't deal with references when it comes to access keys, but I don't really know what that means concretely. Can localizers not translate those strings? Can it not suggest what access keys to use? Does it suggest the wrong thing but it can be overridden? Something else?

Equally, I wouldn't withhold review from a patch doing (2).

Flags: needinfo?(gijskruitbosch+bugs)

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

And then, since we don't have GroupGroupComment, maybe we should extract browser-context.inc to a new FTL file that only holds translation units for those buttons?

FWIW, I do not understand this suggestion. browser-context.inc is the main context menu, but "buttons" are what's in the toolbar, ie not the context menu. I can see two ways of grouping all this stuff - one is per overarching area of UI (ie one file for the toolbar, one for the context menu, one for the main menubar, etc. ) the other would be per-action, so you group all the 4 or so "back" controls together in FTL, all the forward controls, all the reload controls, etc. etc.
From the above comment, I don't know which you're advocating, nor why you're doing so and how it relates to "GroupGroupComment".

we've used this approach before / elsewhere with Fluent, though perhaps the extent of it was smaller then (e.g. https://searchfox.org/mozilla-central/source/toolkit/locales/en-US/toolkit/printing/printPreview.ftl#10 ).

Yep - already affected:
https://pontoon.mozilla.org/pl/firefox/toolkit/toolkit/printing/printPreview.ftl/?string=197410

Ok, so counting votes, we have:

  • Axel - (2)
  • Flod - (2)
  • Gijs - (1)
  • Zibi (1)
  • Stas (2)

As far as I know, Stas, Zibi and Gijs stated that they would be ok if we went with their less preferred option, Axel and Flod did not state such thing.

I'm going to update the patch to follow (2) and scale it to cover the whole browser-context.inc.

From the above comment, I don't know which you're advocating, nor why you're doing so and how it relates to "GroupGroupComment".

I suggested doing the latter. The rationale is that it would allow us to easier find related localization units and via them related UI widgets that they are bind to.
Doing the former would make it harder to connect toolbar-button-back to menuitem-back.

My comment about GroupGroupComment is that we don't have a way to do:

### Toolbar Items

## Forward

toolbar-forward = ...
menuitem-forward = ...
tooltip-forward = ...

## Back

toolbar-back = ...
menuitem-back = ...
tooltip-back = ...

That's why I suggested extracting them to a new file which would only contain such items grouped per-action.
But I'm ok keeping it in browser.ftl as well.

Do you have a preference in that?

Flags: needinfo?(gijskruitbosch+bugs)

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

From the above comment, I don't know which you're advocating, nor why you're doing so and how it relates to "GroupGroupComment".

I suggested doing the latter. The rationale is that it would allow us to easier find related localization units and via them related UI widgets that they are bind to.
Doing the former would make it harder to connect toolbar-button-back to menuitem-back.

Yes, but it would work better with lazy loading, which seems like something we might want for the context menu and the menubar...

My comment about GroupGroupComment is that we don't have a way to do:

### Toolbar Items

## Forward

toolbar-forward = ...
menuitem-forward = ...
tooltip-forward = ...

## Back

toolbar-back = ...
menuitem-back = ...
tooltip-back = ...

That's why I suggested extracting them to a new file which would only contain such items grouped per-action.
But I'm ok keeping it in browser.ftl as well.

Do you have a preference in that?

I think a single file for all these items is fine (one file for Back, one for Forward, etc. would be overkill I think). I think browser.ftl should really ideally stay quite small; browser.dtd/.properties became dumping grounds, and it'd be nice to avoid that this time 'round if we still can.

Flags: needinfo?(gijskruitbosch+bugs)

Ok. Updated the patch to represent the consensus. Please, verify that it matches your expectations and tomorrow I'll work on extending it to cover the whole browser-context.inc.

Thanks, that looks good to me.

I think having a dedicated file is helpful here. I'm a bit lost on which string is which, though, which makes me wonder, if the strings that are conceptually shared with the context menu should live in browser-context.ftl or in their respective locations? "View page source" is one of those that make me wonder.

Gijs?

I also talked to Jeff to make sure that some of the things here are part of our terminology database.

This wfm. One last question:

navbar-tooltip-back = 
    .value = Go back one page

navbar-menu-back =
    .tooltiptext = Go back one page

can we not even make these correspond with references (ie have navbar-tooltip-back.value be { navbar-menu-back.tooltiptext }), as there's no access keys in sight? Just feels like it'd save localizers work... Or is there yet other tooling that won't cope?

Flags: needinfo?(l10n)

Both options are a single click for localizers. One is "use 100% match of a translation memory match", the other is "use the placeable from English".

Probably depends on the localizer if they understand one more than the other or vice versa.

I'd decide this in the light of converting these elements away from XUL. If that conversion changes the attribute, we need to convert the actual translation of that element, as well as all references to it. That migration is possible, in my head, by doing fluent.sytnax.ast.NodeTransformer Transforms on a COPY_PATTERN. Not exactly beautiful, I guess. I intend to write about this as part of bug 1601321.

Flags: needinfo?(l10n)

I'd decide this in the light of converting these elements away from XUL.

bgrins - can you advise here what should we do?

Flags: needinfo?(bgrinstead)

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

I'd decide this in the light of converting these elements away from XUL.

bgrins - can you advise here what should we do?

We are still sorting out what to do with the special xul attributes. For the two mentioned here in particular:

  • tooltiptext (for all elements) - . I expect we will migrate tooltiptext to title to match the HTML name. I expect this would be something we will do ahead of actually moving away from XUL elements to make the migration to HTML easier (metabug 1596497). We'll have to deal with existing FTL strings in the process, so hopefully we'll have a smooth migration path for that (it seems fairly straightforward, I think?).
  • value (for label) - If we need to support the current xul:label[value] layout frame for some reason (middle cropping or some layout reason) then I think we should just keep using xul:label for it. I'd generally like to migrate xul labels with textContent to an html label-y Custom Element, although in that case it wouldn't change the DOM structure that we would localize.
Flags: needinfo?(bgrinstead)

The patch is now complete.

Gijs, Flod - can you review that it looks good? If so, I'll add a migration.

I left all non-browser sourced DTD entries because I don't want to migrate files from toolkit here. There aren't many of them fortunately.

Gijs - there's one piece that I haven't touch - https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#6764

It seems to me like it brings tooltip attribute to a bunch of elements and in theory I think we should add it to the FTL element, because otherwise we'll have to inflate the ID when we'll migrate this part.
The challenge for me is that it seems like the code lazily calculates the shortcut display and then applies the translation, so I assume that for FTL it would look sth like:

FTL:

navbar-menu-reload =
    .aria-label = Reload
    .accesskey = R
    .tooltip = Reload current page ({ $shortcut })

HTML:

          <toolbarbutton id="reload-button" class="toolbarbutton-1"
                         data-l10n-id="navbar-menu-reload"
                         data-l10n-args='{"shortcut": ""}'
                         command="Browser:ReloadOrDuplicate"
                         onclick="checkForMiddleClick(this, event);"
                         tooltip="dynamic-shortcut-tooltip">

and in JS:

for (let elem in dynamicShortcuts) {
    let {id, args} = document.l10n.getAttributes(elem);
    document.l10n.setAttributes(elem, id, { shortcut:  ShortcutUtils.prettifyShortcut(shortcut));
}

Does it look reasonable? Would you like me to tackle in this patchset or file a separate bug?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francesco.lodolo)

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

It seems to me like it brings tooltip attribute to a bunch of elements and in theory I think we should add it to the FTL element, because otherwise we'll have to inflate the ID when we'll migrate this part.
The challenge for me is that it seems like the code lazily calculates the shortcut display and then applies the translation, so I assume that for FTL it would look sth like:
(snip)
Does it look reasonable? Would you like me to tackle in this patchset or file a separate bug?

I think we should do this separately, in bug 1576067, where there is already substantial discussion about this problem. Ideally, we shouldn't do any lazy/JS-based filling in for it - but I think the patch is probably big enough as it is, so let's not try to boil the ocean.

Flags: needinfo?(gijskruitbosch+bugs)

Do you have an idea of how many strings it impact? If we don't do it here, we'll need to do an FTL2FTL migration later, not just for these strings, but also for those that reference them.

Flags: needinfo?(francesco.lodolo)

Do you have an idea of how many strings it impact? If we don't do it here, we'll need to do an FTL2FTL migration later, not just for these strings, but also for those that reference them.

Two - rg 'dynamic-shortcut-tooltip' ./browser/base/content/browser-context.inc

Great, thank you both.
I'm going to apply the feedback from the PR and write the migration.

Status: NEW → ASSIGNED

Completed the migration. The patch is ready for review

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c49bd4bc613d
Migrate browser-context.inc to Fluent. r=Gijs,flod,fluent-reviewers
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b0cd68ff761
Fix the HTML namespace in webext-panels.xhtml.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
You need to log in before you can comment on or make changes to this bug.