1779349 - Synced Tabs sidebar context menu English strings don't match their counterparts in the rest of the browser

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Shane Hughes [:aminomancer]]

These strings in the Synced Tabs sidebar context menu don't match these strings and this string in the tab context menu, places menus, and other locations in the browser. They should be changed like this:

Open in a New Tab :>> Open in New Tab
Open in a New Window :>> Open in New Window
Open in a New Private Window :>> Open in New Private Window
Bookmark This Tab… :>> Bookmark Tab

In bug 1779335 I suggested adding an Open in New Container Tab menuitem to the Synced Tabs sidebar context menu. However, at present, that will look weird surrounded by menuitems that say Open in a New x. And it seems inefficient to add a new string Open in a New Container Tab that's only temporary (as all the Open in a New x strings should be changed to Open in New x), especially when the correct string already exists and is already loaded in the relevant document.

So instead of wasting translators' time on that, I think bug 1779335 should just wait for these strings to change. First we need to remove the a article from all the associated strings and then we can add the new menuitem.

Comment 1

[Shane Hughes [:aminomancer]]

And by the way, we could avoid needing to retranslate all these strings if we just remove these strings from syncedTabs.ftl, and change the l10n IDs from the current ones to the IDs for the corresponding strings in places.ftl and tabContextMenu.ftl, and then either 1) add a localization link to tabContextMenu.ftl in browser.xhtml so it's loaded with the document, or 2) lazy load tabContextMenu.ftl when the Synced Tabs sidebar is loaded — like we currently do with syncedTabs.ftl.

The Synced Tabs sidebar context menu is actually in main-popupset.inc.xhtml, and there is some special behavior in the Synced Tabs sidebar to intercept contextmenu events and open that menu (in the main popupset). So although we're dealing with a sidebar document, the context menu itself is actually in the main window document. Consequently, it already has access to the places.ftl strings. We only need the Bookmark Tab string from tabContextMenu.ftl.

That seems to me to be a better solution than adding new, redundant strings. The main question I have is which is better: loading tabContextMenu.ftl from the start with a link element, or loading it via JS when the sidebar opens. The reason I would prefer the former is because there are already 2 async handlers lazily loading that file. 3 might be a bit out of control? But I suppose it would be best to get opinions from the owners of that code since I imagine there was a good reason for loading it lazily in the first place.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Hughes [:aminomancer] from comment #1)

But I suppose it would be best to get opinions from the owners of that code since I imagine there was a good reason for loading it lazily in the first place.

The synced tabs sidebar has never really been in great shape - it was an attempt to make a react-like model and the author left Mozilla very soon after landing it many years ago. The context menu code might post-date the initial landing, but from the sync-teams POV, feel free to do what makes the most sense while (obviously :) not causing obvious breakage.

Comment 3

[Shane Hughes [:aminomancer]]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #2)

(In reply to Shane Hughes [:aminomancer] from comment #1)

But I suppose it would be best to get opinions from the owners of that code since I imagine there was a good reason for loading it lazily in the first place.

The synced tabs sidebar has never really been in great shape - it was an attempt to make a react-like model and the author left Mozilla very soon after landing it many years ago. The context menu code might post-date the initial landing, but from the sync-teams POV, feel free to do what makes the most sense while (obviously :) not causing obvious breakage.

Great, I'll keep that in mind. Hey Gijs, do you have any opinion on moving tabContextMenu.ftl to browser.xhtml? And Dão, I suspect you have an interest in this as well since you reviewed bug 1658103 for me. That was when we added a handler to the "all tabs" menu to lazily translate the tab context menu (including loading tabContextMenu.ftl).

As far as I can tell, there are 2 main options if we want to avoid adding redundant strings.

1. Add <link rel="localization" href="browser/tabContextMenu.ftl"/> to browser.xhtml, and stop using data-lazy-l10n-id in the tab context menu. That would mean translateTabContextMenu becomes unnecessary.
2. Change translateTabContextMenu so that it also translates the Synced Tabs sidebar context menu. And then call that (it's in gBrowser so should be fine) or at least call MozXULElement.insertFTLIfNeeded("browser/tabContextMenu.ftl") in the sidebar DOMContentLoaded handler.

Comment 4

[:Gijs (he/him)]

(In reply to Shane Hughes [:aminomancer] from comment #1)

That seems to me to be a better solution than adding new, redundant strings. The main question I have is which is better: loading tabContextMenu.ftl from the start with a link element, or loading it via JS when the sidebar opens. The reason I would prefer the former is because there are already 2 async handlers lazily loading that file. 3 might be a bit out of control? But I suppose it would be best to get opinions from the owners of that code since I imagine there was a good reason for loading it lazily in the first place.

Only loading what you need and perf optimization was/is why we load it lazily.

I'd suggest just adding another MozXULElement.insertFTLIfNeeded callsite (they're pretty cheap and basically just insert the FTL link element at runtime if needed) rather than switching to including it in the hardcoded markup.

Comment 5

[Shane Hughes [:aminomancer]]

Attachment: Bug 1779349 - Simplify Synced Tabs sidebar context l10n. r=Gijs,markh

Replace some fluent strings that cover the same ground as existing
strings that see more use. This will make the (English) diction more
consistent between the context menus.

Comment 6

[Francesco Lodolo [:flod]]

Please don't reuse strings in a different context, especially when these strings include access keys.

Consistency is good, but that should be done by creating new strings with the right text for this menu (and likely different access keys based on the commands displayed in it).

Comment 7

[Shane Hughes [:aminomancer]]

(In reply to Francesco Lodolo [:flod] (OOO, back Jul 18) from comment #6)

Consistency is good, but that should be done by creating new strings with the right text for this menu (and likely different access keys based on the commands displayed in it).

The original access keys in the synced tabs context are all the same as the access keys in the tab and places contexts (to add new strings I could just copy from places.ftl and tabContextMenu.ftl, since the access keys were already consistent between these menus even though the strings themselves were slightly different. Then I could use a migration)

edit: So I basically left the original access keys as they were, since they already matched in English, and just changed the strings and their IDs. If you get a chance to look at it, I left a question for you in the diff on phabricator. Thanks 🙏

Comment 8

[Francesco Lodolo [:flod]]

Are the exact same menu items displayed in both menus (i.e. no additional items)? If so, the suggestion of copying the strings over via migration sounds flexible enough (locales can change the access keys if needed) and safe.

to add new strings I could just copy from places.ftl and tabContextMenu.ftl, since the access keys were already consistent between these menus even though the strings themselves were slightly different. Then I could use a migration

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a2ed4218abd
Simplify Synced Tabs sidebar context l10n. r=Gijs,fluent-reviewers

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1a2ed4218abd