Closed Bug 1451992 Opened 7 years ago Closed 7 years ago

Migrate the XUL of Preferences subdialogs

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(12 files, 24 obsolete files)

Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent.
59 bytes, text/x-review-board-request
jaws
: review+
flod
: review+
Details
Bug 1451992 - Spawn errors when encountering missing/empty search l10n ids.
59 bytes, text/x-review-board-request
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::ClearSiteData to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Fonts to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Permissions to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::SelectBookmark to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Bug 1451992 - Migrate Preferences::Subdialogs::Translation to Fluent.
59 bytes, text/x-review-board-request
zbraniecki
: review+
zbraniecki
: review+
Details
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Lots of small daunting patches, but gonna get through them. Each patch is complete and each patch works on its own (they build on one another) and the real bottle neck are migration scripts which are slow to write. I'd like to land it all at once so I'll be adding review requests as I get the migrations ready. Not much for reviewers here. I originally started with more of dtd+properties, but in the interest of keeping this landable I narrowed it down to just migrate DTD first. It's very simple code with little creativity and very few refactors. The carrot is that once we land this we're down to 97 DTD entities in all preferences code! Mostly weird cases and strings imported from other modules. I'll keep working on them, but this patchset is the last major one I'd like to fit into 61.
Comment on attachment 8965561 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent. https://reviewboard.mozilla.org/r/234358/#review239994 Just a few comments to add, migration looks OK. Great to split that string in 3 actual strings. Took me a while to figure out how to trigger this dialog, and can only get to some cases. ::: browser/locales/en-US/browser/preferences/applicationManager.ftl:14 (Diff revision 1) > +app-manager-remove = > + .label = Remove > + .accesskey = R > + > +app-manager-handle-webfeeds = The following applications can be used to handle Web Feeds. > +app-manager-handle-protocol = The following applications can be used to handle { $type } links. Comment: $type is the URI scheme of the link (e.g. mailto:) ::: browser/locales/en-US/browser/preferences/applicationManager.ftl:15 (Diff revision 1) > + .label = Remove > + .accesskey = R > + > +app-manager-handle-webfeeds = The following applications can be used to handle Web Feeds. > +app-manager-handle-protocol = The following applications can be used to handle { $type } links. > +app-manager-handle-file = The following applications can be used to handle { $type } content. Comment: $type is ? I assume the file (MIME?) type, but I haven't been able to trigger this dialog for files. ::: browser/locales/en-US/browser/preferences/applicationManager.ftl:17 (Diff revision 1) > + > +app-manager-handle-webfeeds = The following applications can be used to handle Web Feeds. > +app-manager-handle-protocol = The following applications can be used to handle { $type } links. > +app-manager-handle-file = The following applications can be used to handle { $type } content. > + > +app-manager-web-app-info = This web application is hosted at: Let's add a comment explaining that these strings are followed, on a new line, by the URL or path of the application. It could also be a section comment. Path is an assumption on my side, since I've been able to trigger this dialog only for web handlers.
Attachment #8965561 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8965561 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent. https://reviewboard.mozilla.org/r/234358/#review239996 ::: browser/locales/en-US/browser/preferences/applicationManager.ftl:15 (Diff revision 1) > + .label = Remove > + .accesskey = R > + > +app-manager-handle-webfeeds = The following applications can be used to handle Web Feeds. > +app-manager-handle-protocol = The following applications can be used to handle { $type } links. > +app-manager-handle-file = The following applications can be used to handle { $type } content. OK, I was able to trigger this (obviously right after sending the comments). I think it's the MIME type, e.g. application/binary, and path is displayed if it's a local app
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review240060 ::: browser/components/preferences/colors.xul:28 (Diff revision 1) > > <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/> > <script type="application/javascript" src="chrome://global/content/preferencesBindings.js"/> > > <keyset> > - <key key="&windowClose.key;" modifiers="accel" oncommand="Preferences.close(event)"/> > + <key data-l10n-id="colors-close-key" modifiers="accel" oncommand="Preferences.close(event)"/> Where is windowClose.key coming from? It's not in colors.dtd or preferences.dtd ::: browser/locales/en-US/browser/preferences/colors.ftl:9 (Diff revision 1) > + > +colors-window = > + .title = Colors > + .style = > + { PLATFORM() -> > + [mac] width: 41em mac -> macos ::: browser/locales/en-US/browser/preferences/colors.ftl:22 (Diff revision 1) > + .accesskey = O > + > +colors-page-override-option-always = > + .label = Always > +colors-page-override-option-auto = > + .label = Only with High Contract themes typo: Contrast
Migration is "hardcoding" the shortcut for window.close
Attachment #8965640 - Attachment is patch: true
Attachment #8965640 - Attachment mime type: text/x-python-script → text/plain
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review240110 ::: browser/components/preferences/colors.xul:28 (Diff revision 1) > > <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/> > <script type="application/javascript" src="chrome://global/content/preferencesBindings.js"/> > > <keyset> > - <key key="&windowClose.key;" modifiers="accel" oncommand="Preferences.close(event)"/> > + <key data-l10n-id="colors-close-key" modifiers="accel" oncommand="Preferences.close(event)"/> chrome://global/locale/preferences.dtd So it comes from https://hg.mozilla.org/l10n/gecko-strings/file/default/toolkit/chrome/global/preferences.dtd
Comment on attachment 8965562 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent. https://reviewboard.mozilla.org/r/234360/#review240116 LGTM, though it'd be nice if we didn't have to duplicate the OK/Cancel and close key stuff for every subdialog... ::: browser/components/preferences/in-content/privacy.xul:350 (Diff revision 1) > <button id="changeBlockList" > class="accessory-button" > flex="1" > data-l10n-id="tracking-change-block-list" > preference="pref.privacy.disable_button.change_blocklist" > - searchkeywords="&button.cancel.label; &button.ok.label;"/> > + search-l10n-ids="blocklist-window.title, blocklist-desc, blocklist-button-cancel.label, blocklist-button-ok.label"/> Huh. I guess we're just converting this, but colour me unconvinced that it makes sense to have people search for the OK/Cancel labels here. Maybe Jared has more details on why we do this? ::: browser/locales/en-US/browser/preferences/blocklists.ftl:10 (Diff revision 1) > +blocklist-window = > + .title = Block Lists > + .style = width: 55em > + > +blocklist-desc = You can choose which list { -brand-short-name } will use to block Web elements that may track your browsing activity. > +blocklist-close-key = It looks like this is now no longer shared between subdialogs. Similar comments could likely apply to the ok/cancel buttons (though I guess the access keys there might make things tricky...). Would it make sense to use a shared file for this instead, so localizers don't have to localize the same thing twice, and we don't end up shipping identical content all over the place?
Attachment #8965562 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #21) > > +blocklist-close-key = > > It looks like this is now no longer shared between subdialogs. Similar > comments could likely apply to the ok/cancel buttons (though I guess the > access keys there might make things tricky...). Would it make sense to use a > shared file for this instead, so localizers don't have to localize the same > thing twice, and we don't end up shipping identical content all over the > place? The migration is actually not changing the current situation. We have 6 of these defined in DTDs subdialogs, to the point that I was surprised when looking at colors.dtd, and I wasn't able to find it. https://transvision.mozfr.org/?recherche=windowclose.key&repo=gecko_strings&sourcelocale=en-US&locale=it&search_type=strings_entities Having a file with shared strings is nice, but potentially problematic. Keyboard shortcuts work, access keys likely not, because the context (and available characters) change.
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review240120 ::: commit-message-0b9ec:1 (Diff revision 1) > +Bug 1451992 - Migrate Preferences::Subdialogs::Color to Fluent. nit: the dialog and file are called colors
Comment on attachment 8965565 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent. https://reviewboard.mozilla.org/r/234366/#review240106 ::: browser/locales/en-US/browser/preferences/connection.ftl:9 (Diff revision 1) > + > +connection-window = > + .title = Connection Settings > + .style = > + { PLATFORM() -> > + [mac] width: 44em macos ::: browser/locales/en-US/browser/preferences/connection.ftl:28 (Diff revision 1) > + .accesskey = y > +connection-proxy-option-system = > + .label = Use system proxy settings > + .accesskey = u > +connection-proxy-option-auto = > + .label = Audo-detect proxy settings for this network typo: Auto ::: browser/locales/en-US/browser/preferences/connection.ftl:77 (Diff revision 1) > +connection-proxy-reload = > + .label = Reload > + .accesskey = e > + > +connection-proxy-autologin = > + .label = Do not prompt for authentication if password is saved<Paste> Extra <Paste>
Attachment #8965640 - Attachment is obsolete: true
stas noted on IRC that we should use FTL.Function for PLATFORM, not FTL.Identifier I copied that fragment from existing migrations, so they likely need fixing at some point for consistency.
Attachment #8965662 - Attachment is obsolete: true
Attachment #8965663 - Attachment is obsolete: true
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review240130 ::: browser/components/preferences/languages.xul (Diff revision 1) > <row> > <!-- This <vbox> is needed to position search tooltips correctly. --> > <vbox> > <menulist id="availableLanguages" oncommand="gLanguagesDialog.onAvailableLanguageSelect();" > - label="&languages.customize.selectLanguage.label;" > + data-l10n-id="languages-customize-select-language"> > - label2="&languages.customize.selectLanguage.label;"> Is label2 going to be set by run-time without any further code? ::: browser/locales/en-US/browser/preferences/languages.ftl:22 (Diff revision 1) > +languages-customize-moveup = > + .label = Move Up > + .accesskey = U > + > +languages-customize-movedown = > + .label = Move down Move Down
Attachment #8965680 - Attachment is patch: true
Attachment #8965680 - Attachment mime type: text/x-python-script → text/plain
> Where is windowClose.key coming from? It's not in colors.dtd or preferences.dtd Yeah, there were several strings inconsistently repeated across the dialogs like close key and button ok/cancel (but sometimes save/cancel or ok/close). I opted to separate them because it's not that much work for localizers over a shared file while such `shared.ftl` would likely become a burden over time. In the light of the planned Preferences UX refresh later this year (rumors, but a lot of them ;)), I think keeping it simple and ftl-per-xul is going to carry forward the best and if we'll end up wanting a shared.ftl file, we always can add it - way easier than separating out :) > Is label2 going to be set by run-time without any further code? No, I need to add `data-l10n-attrs="label2"` there, but overall this is very questionable and I wanted to ask my reviewer if we really need the `label2` here.
Comment on attachment 8965569 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Permissions to Fluent. https://reviewboard.mozilla.org/r/234374/#review240140 ::: browser/locales/en-US/browser/preferences/permissions.ftl:12 (Diff revision 1) > + .style = width: 45em > + > +permissions-close-key = > + .key = w > + > +permissions-address = Addess of website Address ::: browser/locales/en-US/browser/preferences/permissions.ftl:28 (Diff revision 1) > +permissions-allow = > + .label = Allow > + .accesskey = A > + > +permissions-site-name = > + .label = Webite Website ::: browser/locales/en-US/browser/preferences/permissions.ftl:35 (Diff revision 1) > +permissions-status = > + .label = Status > + > +permissions-remove = > + .label = Remove Website > + .accesskey = e R
Attachment #8965690 - Attachment is obsolete: true
> Huh. I guess we're just converting this, but colour me unconvinced that it makes sense to have people search for the OK/Cancel labels here. Maybe Jared has more details on why we do this? Yeah, I'm also pretty unconvinced that the current selection of search keys really makes sense. So much of it is just listing words like "OK', "Cancel", "Click", "Save" etc. but since this can be adjusted without any changes to l10n, I try to follow closely and once we complete the migration we can look at the result and reselect strings. Does it sound reasonable?
Attachment #8965566 - Attachment is obsolete: true
Attachment #8965677 - Attachment is obsolete: true
Attachment #8965680 - Attachment is obsolete: true
Attachment #8965682 - Attachment is obsolete: true
Attachment #8965691 - Attachment is obsolete: true
Attachment #8965693 - Attachment is obsolete: true
Attachment #8965700 - Attachment is obsolete: true
Attachment #8965704 - Attachment is obsolete: true
Comment on attachment 8965567 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Fonts to Fluent. https://reviewboard.mozilla.org/r/234370/#review240198 ::: browser/locales/en-US/browser/preferences/fonts.ftl:50 (Diff revision 2) > + .label = Kannada > +fonts-langgroup-khmer = > + .label = Khmer > +fonts-langgroup-korean = > + .label = Korean > +# Translate "Latin" as the name of Latin (Roman) script, not as the name of the Latin language. --> Extra --> ::: browser/locales/en-US/browser/preferences/fonts.ftl:104 (Diff revision 2) > + > +fonts-minsize-none = > + .label = None > + > +fonts-allow-own = > + .label = Allow pages to choose their own fonts, instead of our selections above our -> your
Comment on attachment 8965561 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent. https://reviewboard.mozilla.org/r/234358/#review240204 ::: browser/locales/en-US/browser/preferences/applicationManager.ftl:20 (Diff revisions 1 - 2) > +# Variables: > +# $type (String) - the URI scheme of the link (e.g. mailto:) > app-manager-handle-protocol = The following applications can be used to handle { $type } links. > + > +# Variables: > +# $type (String) - the MIME type (e.g. application/binary) or a local path Only # $type (String) - the MIME type (e.g. application/binary)
Attachment #8965750 - Attachment is obsolete: true
The try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b8162023d1c643642dc3aa2d640a4c5a95234cb Adding a few more r? on the next set of patches to accommodate for different bandwidth of reviewers but trying to keep the queue relatively evenly distributed.
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review240248 ::: browser/components/preferences/languages.xul:79 (Diff revision 6) > </row> > <row> > <!-- This <vbox> is needed to position search tooltips correctly. --> > <vbox> > <menulist id="availableLanguages" oncommand="gLanguagesDialog.onAvailableLanguageSelect();" > - label="&languages.customize.selectLanguage.label;" > + data-l10n-id="languages-customize-select-language" data-l10n-attrs="label2"> Gijs - I didn't touch it because I don't fully understand how the JS code uses this, but I'd love to get rid of `label2` here. Is it possible?
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review240252 reviewed the migration only (written by Flod).
Attachment #8965564 - Flags: review?(gandalf) → review+
Comment on attachment 8965565 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent. https://reviewboard.mozilla.org/r/234366/#review240254 Reviewed the migration only (written by Flod)
Attachment #8965565 - Flags: review?(gandalf) → review+
See Also: → 1452637
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review240628 ::: browser/components/preferences/colors.xul:22 (Diff revision 4) > role="dialog" > helpTopic="prefs-fonts-and-colors" > - ondialoghelp="openPrefsHelp()" > -#ifdef XP_MACOSX > - style="width: &window.macWidth; !important;"> > -#else > + ondialoghelp="openPrefsHelp()"> > + > + <link rel="localization" href="browser/preferences/colors.ftl"/> > + <script type="text/javascript" src="chrome://global/content/l10n.js"></script> Nit: All the other scripts in this file (on the next few lines) use `application/javascript`. In fact, the only non-devtools, non-test users of `text/javascript` in the entire tree are other instances of l10n.js ...
Attachment #8965564 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8965565 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent. https://reviewboard.mozilla.org/r/234366/#review240634 ::: browser/locales/en-US/browser/preferences/connection.ftl:16 (Diff revision 4) > +connection-disable-extension = > + .label = Disable Extension This was shared before https://dxr.mozilla.org/mozilla-central/source/python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py#189 . Why do we need 2 copies now?
Attachment #8965565 - Flags: review?(gijskruitbosch+bugs) → review+
> Nit: All the other scripts in this file (on the next few lines) use `application/javascript`. In fact, the only non-devtools, non-test users of `text/javascript` in the entire tree are other instances of l10n.js ... Good point. Filed bug 1452680 as a follow up :) > Why do we need 2 copies now? Similarly to close-key and buttons, I decoupled them to reduce the interdependency between subdialogs. I can try to merge them back but that would either required shared.ftl or for connection.xul and another subdialog to both load preferences.ftl (pretty large FTL file). Would that be your preference?
Blocks: 1452680
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review240650 ::: browser/components/preferences/languages.xul:79 (Diff revision 6) > </row> > <row> > <!-- This <vbox> is needed to position search tooltips correctly. --> > <vbox> > <menulist id="availableLanguages" oncommand="gLanguagesDialog.onAvailableLanguageSelect();" > - label="&languages.customize.selectLanguage.label;" > + data-l10n-id="languages-customize-select-language" data-l10n-attrs="label2"> The issue is that the `label` property of a `<menulist>` (and its mirrored attribute) get updated if you select an item. The UI here is just a bit clumsy - the dropdown is effectively labeling itself, and then takes the label of the choice you make. Then when you click 'Add', we want to set the label back to "Select a language to add...". But because the 'real' label attribute is overwritten, we have to have a backup attribute. The simplest way to get rid of this would be to copy the label attribute with some JS once localization has happened. I don't know if fluent exposes events or promises that make this easily possible. Separately, I think the whole thing would be clearer if instead of "label2" we used a more meaningful equivalent, like "backuplabel" or "defaultlabel" or something, and added a comment before the `setAttribute` call that currently restores the label to the "original" label after the user adds a language. (I tried to find out how it worked through code archaeology, but this was just part of a giant patch for "new preferences" from ben goodger in the Fx1 times. So that wasn't much help, hence asking for a comment. I eventually analyzed it by just using the inspector and checking what happened to attributes as I was using the UI.) ::: browser/locales/en-US/browser/preferences/languages.ftl:31 (Diff revision 6) > + .label = Remove > + .accesskey = R > + > +languages-customize-select-language = > + .label = Select a language to add… > + .label2 = { languages-customize-select-language.label } Yeah, this seems like a confusion that shouldn't be in the l10n file, but should be in the JS instead.
Attachment #8965568 - Flags: review?(gijskruitbosch+bugs)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #91) > > Why do we need 2 copies now? > > Similarly to close-key and buttons, I decoupled them to reduce the > interdependency between subdialogs. I can try to merge them back but that > would either required shared.ftl or for connection.xul and another subdialog > to both load preferences.ftl (pretty large FTL file). > > Would that be your preference? Ehhh. I'm ambivalent. I think in general, sharing identical strings is good (for l10n), but equally, having lots of fully identical strings are often UX smells. For instance (as I think most people know by now) a traditional [OK] [Cancel] dialog choice (which you could use in lots of places) is better rendered with a clearer set of verbs like [Save file][Don't save] (which you can't just use in lots of places). So sharing strings in the face of good UX maybe isn't actually all that useful/likely. I would expect that in many cases we need more in the string than just "Disable extension", or if not then at least some of the meaning comes from surrounding text, like "uBlock Origin is controlling your tracking protection preferences" that is in the same line (but not part of the button) or whatever, and in some languages maybe the button text needs to vary based on such context (in this case, the 'tracking protection' part of things, ie the thing the add-on is controlling and for which the user wants control back, or the name of the add-on, or whatever). On the other hand, as we've discussed previously, where the context is an arbitrary string (in this case, an add-on name), l10n can't really deal with that. That is, if the arbitrary string can have different grammatical properties (gender, number, case, whatever), there's no way our code can (currently) infer those and use them to select the best l10n string based on that arbitrary string. So perhaps the context-stuff isn't actually relevant and we should just share the string. So I'm on the fence, it depends on factors that I don't have good insights into (ie how likely is it that different languages will want (either now or "soon") different strings for the different places where we use this string). Do what seems best to you. :-) If we do get a shared file I might prefer a separate shared.ftl over the giant preferences one for reasons of perf + clarity - though on the perf front, do we not cache stuff in such a way that once a .ftl file has been in use once, it's ~free for another consumer to use it?
Flags: needinfo?(gijskruitbosch+bugs)
> on the perf front, do we not cache stuff in such a way that once a .ftl file has been in use once, it's ~free for another consumer to use it? Unfortunately no, we cache a context (which has a hash document.url+FTLresourceList), so every next opening of the same window/dialog is free, but loading a new context requires a new fetch even if the file has been fetched before. The proposal to change that is in bug 1384236 but encountered opposition from the Fluent API design perspective (it requires a not-really-public API to be exposed). Ok, I'll talk it through with Flod and based on your feedback we'll decide on this. Thanks for the feedback!
Triage: Giving this a priority to get it off the triage list.
Priority: -- → P1
Comment on attachment 8965562 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent. https://reviewboard.mozilla.org/r/234360/#review240890 This is somewhat closer than to an r+ with comments, but given the amount of attachments, making an explicit r- flag to not overlook this. The migration recipe has a bug in it. ::: python/l10n/fluent_migrations/bug_1451992_preferences_blocklists.py:43 (Diff revision 3) > + ], > + ), > + FTL.Message( > + id=FTL.Identifier('blocklist-desc'), > + value=REPLACE( > + 'browser/chrome/browser/preferences/blocklist.properties', This is coming from `browser/chrome/browser/preferences/preferences.properties`
Attachment #8965562 - Flags: review?(l10n) → review-
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review240896 As Gijs, I'd like to see the label2 stuff done first. I wonder if we can only have label2 (with a better name), and leave the label foo to code?
Attachment #8965568 - Flags: review?(l10n)
Thanks Gijs, Axel! I applied your feedback and attempted to clean up the languages, but just a bit. I considered adding a disabled menuitem which would populate the label, but that felt like a larger refactor, so I sticked to what you suggested. I also populated the remaining review requests. Since the surface of the patchset is quite large, I'd appreciate help with the review of this. Jaws, Stas - if you won't have time for this anytime soon, let me know and I'll shuffle the assignments to balance the load.
Comment on attachment 8965560 [details] Bug 1451992 - Spawn errors when encountering missing/empty search l10n ids. https://reviewboard.mozilla.org/r/234356/#review241092
Attachment #8965560 - Flags: review?(jaws) → review+
Comment on attachment 8965569 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Permissions to Fluent. https://reviewboard.mozilla.org/r/234374/#review241094 Only tested and reviewed the migration to ensure if produces the correct ftl file
Attachment #8965569 - Flags: review?(gandalf) → review+
Comment on attachment 8965561 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent. https://reviewboard.mozilla.org/r/234358/#review241102
Attachment #8965561 - Flags: review?(jaws) → review+
Comment on attachment 8965563 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ClearSiteData to Fluent. https://reviewboard.mozilla.org/r/234362/#review240094 ::: browser/locales/en-US/browser/preferences/clearSiteData.ftl:19 (Diff revision 1) > +clear-site-data-cookies = Cookies and Site Data > + .accesskey = S > + > +clear-site-data-cookies-info = You may get signed out of websites if cleared > + > +clear-site-data-cache = Cache Web Content Cached ::: browser/components/preferences/clearSiteData.xul:40 (Diff revision 4) > - <checkbox id="clearSiteData" checked="true" > + <checkbox id="clearSiteData" checked="true" /> > - accesskey="&clearSiteData.accesskey;" /> > <vbox> > - <label for="clearSiteData" id="clearSiteDataLabel" value="&clearSiteData.label;" /> > - <description class="option-description">&clearSiteData.description;</description> > + <label for="clearSiteData" > + id="clearSiteDataLabel" > + control="clearSiteData" Question unrelated to l10n: does a label need both control and for attributes? I searched for some documentation, but failed to find anything useful.
Attachment #8965563 - Flags: review+
Comment on attachment 8965563 [details] Bug 1451992 - Migrate Preferences::Subdialogs::ClearSiteData to Fluent. https://reviewboard.mozilla.org/r/234362/#review241106
Attachment #8965563 - Flags: review?(jaws) → review+
Comment on attachment 8965567 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Fonts to Fluent. https://reviewboard.mozilla.org/r/234370/#review241108
Attachment #8965567 - Flags: review?(jaws) → review+
Attachment #8965563 - Flags: review?(stas)
Comment on attachment 8965571 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent. https://reviewboard.mozilla.org/r/234378/#review241110 ::: browser/locales/en-US/browser/preferences/siteDataSettings.ftl:26 (Diff revision 6) > +site-data-column-last-used = > + .label = Last Used > + > +site-data-remove-selected = > + .label = Remove Selected > + .accesskey = r R
Attachment #8965571 - Flags: review?(jaws) → review+
Comment on attachment 8965565 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent. https://reviewboard.mozilla.org/r/234366/#review241112 ::: browser/locales/en-US/browser/preferences/connection.ftl:26 (Diff revision 5) > +connection-proxy-option-no = > + .label = No proxy > + .accesskey = y > +connection-proxy-option-system = > + .label = Use system proxy settings > + .accesskey = u U ::: browser/locales/en-US/browser/preferences/connection.ftl:32 (Diff revision 5) > +connection-proxy-option-auto = > + .label = Auto-detect proxy settings for this network > + .accesskey = w > +connection-proxy-option-manual = > + .label = Manual proxy configuration > + .accesskey = m M ::: browser/locales/en-US/browser/preferences/connection.ftl:64 (Diff revision 5) > + .accesskey = K > +connection-proxy-socks5 = > + .label = SOCKS v5 > + .accesskey = v > +connection-proxy-noproxy = No Proxy for > + .accesskey = n N ::: browser/locales/en-US/browser/preferences/connection.ftl:83 (Diff revision 5) > + .accesskey = i > + .tooltip = This option silently authenticates you to proxies when you have saved credentials for them. You will be prompted if authentication fails. > + > +connection-proxy-socks-remote-dns = > + .label = Proxy DNS when using SOCKS v5 > + .accesskey = d D
Comment on attachment 8965567 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Fonts to Fluent. https://reviewboard.mozilla.org/r/234370/#review241270 ::: browser/components/preferences/fonts.xul:14 (Diff revision 7) > -]> > - > <dialog id="FontsDialog" type="child" class="prefwindow" > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > - title="&fontsDialog.title;" > + data-l10n-id="fonts-window" > + data-l10n-attrs="title, style" Do we need to allow "style" here? ::: browser/locales/en-US/browser/preferences/fonts.ftl:11 (Diff revision 7) > + .title = Fonts > + > +fonts-window-close = > + .key = w > + > +fonts-header = Fonts for Perhaps rename this to `fonts-langgroup-header` and add a short group comment above it? Something as simple as: ## Font groups by language ::: browser/locales/en-US/browser/preferences/fonts.ftl:74 (Diff revision 7) > +fonts-langgroup-canadian = > + .label = Unified Canadian Syllabary > +fonts-langgroup-other = > + .label = Other Writing Systems > + > +fonts-proportional-header = Proportional Maybe add a group comment above this? ## Default fonts and their sizes
Attachment #8965567 - Flags: review?(stas) → review+
Comment on attachment 8965572 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Translation to Fluent. https://reviewboard.mozilla.org/r/234380/#review241310 TIL this feature existed. ::: python/l10n/fluent_migrations/bug_1451992_preferences_translation.py:46 (Diff revision 6) > + id=FTL.Identifier('translation-close-key'), > + attributes=[ > + FTL.Attribute( > + FTL.Identifier('key'), > + COPY( > + 'browser/chrome/browser/preferences/translation.dtd', I see that this string exists in browser/chrome/browser/preferences/translation.dtd but perhaps it would be better for consistency to migrate it from toolkit/chrome/global/preferences.dtd as you do in other patches?
Attachment #8965572 - Flags: review?(stas) → review+
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review241352
Attachment #8965568 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8965569 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Permissions to Fluent. https://reviewboard.mozilla.org/r/234374/#review241356
Attachment #8965569 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8965570 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SelectBookmark to Fluent. https://reviewboard.mozilla.org/r/234376/#review241358
Attachment #8965570 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8965572 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Translation to Fluent. https://reviewboard.mozilla.org/r/234380/#review241362
Attachment #8965572 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8965571 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent. https://reviewboard.mozilla.org/r/234378/#review240170 ::: browser/components/preferences/siteDataSettings.xul:37 (Diff revision 1) > <description id="settingsDescription"></description> > <separator class="thin"/> > > <hbox id="searchBoxContainer"> > <textbox id="searchBox" type="search" flex="1" > - placeholder="&searchTextboxPlaceHolder;" accesskey="&searchTextboxPlaceHolder.accesskey;"/> > + data-l10n-id="site-data-search-textbox"/> Do we really need an accesskey on a placeholder? Does this even work?
(In reply to Francesco Lodolo [:flod] from comment #125) > Comment on attachment 8965571 [details] > Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent. > > https://reviewboard.mozilla.org/r/234378/#review240170 > > ::: browser/components/preferences/siteDataSettings.xul:37 > (Diff revision 1) > > <description id="settingsDescription"></description> > > <separator class="thin"/> > > > > <hbox id="searchBoxContainer"> > > <textbox id="searchBox" type="search" flex="1" > > - placeholder="&searchTextboxPlaceHolder;" accesskey="&searchTextboxPlaceHolder.accesskey;"/> > > + data-l10n-id="site-data-search-textbox"/> > > Do we really need an accesskey on a placeholder? Does this even work? Answering my own question: surprisingly enough, it works, even if it's not visible in the UI. I guess it's still something worth keeping for accessibility.
Comment on attachment 8965571 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent. https://reviewboard.mozilla.org/r/234378/#review241406 Stealing some r? from pike per IRC discussion ::: browser/components/preferences/in-content/privacy.xul:240 (Diff revision 6) > data-l10n-id="sitedata-settings" > - searchkeywords="&window2.title; > - &hostCol.label; > - &cookiesCol.label; > - &usageCol.label;"/> > + search-l10n-ids=" > + site-data-settings-window.title, > + site-data-column-host.label, > + site-data-column-cookies.label, > + site-data-column-storage.label, Is the final comma really needed? I see one-liners don't have that. ::: browser/components/preferences/siteDataRemoveSelected.xul:43 (Diff revision 6) > #ifndef XP_MACOSX > hidden="true" > #endif > - >&removingDialog1.title;</label> > + /> > <separator class="thin"/> > - <description id="removing-description">&removingSelected1.description;</description> > + <description id="removing-description" data-l10n-id="site-data-removing-decs"/> typo: decs -> desc
Attachment #8965571 - Flags: review+
Comment on attachment 8965570 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SelectBookmark to Fluent. https://reviewboard.mozilla.org/r/234376/#review241412 This should read as an r+ with the style fixed, but I'd like to check the migration changes. ::: browser/components/preferences/selectBookmark.xul:16 (Diff revision 7) > - > <dialog id="selectBookmarkDialog" > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > - title="&selectBookmark.title;" style="width: 32em;" > + data-l10n-id="select-bookmark-window" > + data-l10n-attrs="title" > + style="width: 32em;" I believe we should make this localizable, still migrate the title and write the "width: 32em;" into the style value. No practical change, but locales would be able to change it as needed in the future.
Comment on attachment 8965562 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent. https://reviewboard.mozilla.org/r/234360/#review240000 Migration fix looks good
Attachment #8965562 - Flags: review+
Attachment #8965571 - Flags: review?(l10n)
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review241424
Attachment #8965568 - Flags: review+
Attachment #8965562 - Flags: review?(l10n)
Attachment #8965568 - Flags: review?(l10n)
Attachment #8965570 - Flags: review?(l10n)
Comment on attachment 8965570 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SelectBookmark to Fluent. https://reviewboard.mozilla.org/r/234376/#review241428
Attachment #8965570 - Flags: review?(francesco.lodolo)
(In reply to Staś Małolepszy :stas from comment #120) > Comment on attachment 8965572 [details] > Bug 1451992 - Migrate Preferences::Subdialogs::Translation to Fluent. > > https://reviewboard.mozilla.org/r/234380/#review241310 > > TIL this feature existed. > > ::: python/l10n/fluent_migrations/bug_1451992_preferences_translation.py:46 > (Diff revision 6) > > + id=FTL.Identifier('translation-close-key'), > > + attributes=[ > > + FTL.Attribute( > > + FTL.Identifier('key'), > > + COPY( > > + 'browser/chrome/browser/preferences/translation.dtd', > > I see that this string exists in > browser/chrome/browser/preferences/translation.dtd but perhaps it would be > better for consistency to migrate it from > toolkit/chrome/global/preferences.dtd as you do in other patches? Some files have that shortcut, other don't. I think we need to migrate the one in the file, if available. From a practical point of view, nothing changes, since they are identical.
Comment on attachment 8965562 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent. https://reviewboard.mozilla.org/r/234360/#review241472
Attachment #8965562 - Flags: review+
Comment on attachment 8965568 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Languages to Fluent. https://reviewboard.mozilla.org/r/234372/#review241474
Attachment #8965568 - Flags: review+
Comment on attachment 8965567 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Fonts to Fluent. https://reviewboard.mozilla.org/r/234370/#review241476
Attachment #8965567 - Flags: review+
Comment on attachment 8965571 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SiteData to Fluent. https://reviewboard.mozilla.org/r/234378/#review241482
Attachment #8965571 - Flags: review+
Comment on attachment 8965570 [details] Bug 1451992 - Migrate Preferences::Subdialogs::SelectBookmark to Fluent. https://reviewboard.mozilla.org/r/234376/#review241486 Looks good with the migration fixed. ::: python/l10n/fluent_migrations/bug_1451992_preferences_selectbookmark.py:30 (Diff revisions 7 - 9) > 'selectBookmark.title' > ) > + ), > + FTL.Attribute( > + FTL.Identifier('style'), > + FTL.TextElement('width: 32em;') This needs to be CONCAT( FTL.TextElement('width: 32em;') )
Attachment #8965570 - Flags: review?(francesco.lodolo) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s af38764a9858e063fe7791287483ca89f64d2f68 -d 69bd463e5db0: rebasing 457244:af38764a9858 "Bug 1451992 - Spawn errors when encountering missing/empty search l10n ids. r=jaws" rebasing 457245:15434e943291 "Bug 1451992 - Migrate Preferences::Subdialogs::ApplicationManager to Fluent. r=flod,jaws" merging browser/locales/jar.mn rebasing 457246:186d5ee7eab0 "Bug 1451992 - Migrate Preferences::Subdialogs::Blocklists to Fluent. r=flod,Gijs" merging browser/components/preferences/in-content/privacy.js merging browser/locales/en-US/chrome/browser/preferences/preferences.properties merging browser/locales/jar.mn rebasing 457247:477d03d7fb23 "Bug 1451992 - Migrate Preferences::Subdialogs::ClearSiteData to Fluent. r=flod,jaws" merging browser/locales/jar.mn rebasing 457248:58a9e6365395 "Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. r=Gijs" merging browser/locales/jar.mn rebasing 457249:0ccced9f2e49 "Bug 1451992 - Migrate Preferences::Subdialogs::Connection to Fluent. r=Gijs" merging browser/components/preferences/connection.xul merging browser/locales/jar.mn warning: conflicts while merging browser/components/preferences/connection.xul! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 12 changesets with 72 changes to 47 files remote: (ftl_check check enabled per config override) remote: remote: ************************ ERROR ************************* remote: You are trying to commit a change to an FTL file. remote: At the moment modifying FTL files requires a review from remote: one of the L10n Drivers. remote: Please, request review from either: remote: - Francesco Lodolo (:flod) remote: - Zibi Braniecki (:gandalf) remote: - Axel Hecht (:pike) remote: - Stas Malolepszy (:stas) remote: ******************************************************** remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.mozhooks hook failed abort: push failed on remote
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review241648
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review241650
Attachment #8965564 - Flags: review+ → review?(gandalf)
Comment on attachment 8965564 [details] Bug 1451992 - Migrate Preferences::Subdialogs::Colors to Fluent. https://reviewboard.mozilla.org/r/234364/#review241652
Attachment #8965564 - Flags: review?(gandalf) → review+
Attachment #8965560 - Attachment is obsolete: true
Attachment #8965561 - Attachment is obsolete: true
Attachment #8965562 - Attachment is obsolete: true
Attachment #8965563 - Attachment is obsolete: true
Attachment #8965564 - Attachment is obsolete: true
Attachment #8965565 - Attachment is obsolete: true
Attachment #8965567 - Attachment is obsolete: true
Attachment #8965568 - Attachment is obsolete: true
Attachment #8965569 - Attachment is obsolete: true
Attachment #8965570 - Attachment is obsolete: true
Attachment #8965572 - Attachment is obsolete: true
I hate mozreview...
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/48c075b47cc1 Spawn errors when encountering missing/empty search l10n ids. r=jaws https://hg.mozilla.org/integration/autoland/rev/6e34007a3dba Migrate Preferences::Subdialogs::ApplicationManager to Fluent. r=jaws,flod https://hg.mozilla.org/integration/autoland/rev/8ebf84e9825d Migrate Preferences::Subdialogs::Blocklists to Fluent. r=gijs,flod https://hg.mozilla.org/integration/autoland/rev/aae9d9d8e951 Migrate Preferences::Subdialogs::ClearSiteData to Fluent. r=jaws,flod https://hg.mozilla.org/integration/autoland/rev/ab889ec68ba2 Migrate Preferences::Subdialogs::Colors to Fluent. r=gijs,gandalf https://hg.mozilla.org/integration/autoland/rev/7870682a569d Migrate Preferences::Subdialogs::Connection to Fluent. r=gijs,gandalf https://hg.mozilla.org/integration/autoland/rev/ffa48a447e17 Migrate Preferences::Subdialogs::Fonts to Fluent. r=jaws,stas https://hg.mozilla.org/integration/autoland/rev/575d3f1fea78 Migrate Preferences::Subdialogs::Languages to Fluent. r=gijs,flod https://hg.mozilla.org/integration/autoland/rev/e8aea207d72d Migrate Preferences::Subdialogs::Permissions to Fluent. r=gijs,gandalf https://hg.mozilla.org/integration/autoland/rev/4ed8d57671c3 Migrate Preferences::Subdialogs::SelectBookmark to Fluent. r=gijs,flod https://hg.mozilla.org/integration/autoland/rev/ecb97b04b319 Migrate Preferences::Subdialogs::SiteData to Fluent. r=jaws,flod https://hg.mozilla.org/integration/autoland/rev/27e293c662c8 Migrate Preferences::Subdialogs::Translation to Fluent. r=gijs,stas
Attachment #8967218 - Flags: review?(jaws) → review+
Attachment #8967219 - Flags: review?(jaws)
Attachment #8967219 - Flags: review?(francesco.lodolo)
Attachment #8967219 - Flags: review+
Attachment #8967220 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967220 - Flags: review?(francesco.lodolo)
Attachment #8967220 - Flags: review+
Attachment #8967221 - Flags: review?(jaws)
Attachment #8967221 - Flags: review?(francesco.lodolo)
Attachment #8967221 - Flags: review+
Attachment #8967222 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967222 - Flags: review?(gandalf)
Attachment #8967222 - Flags: review+
Attachment #8967223 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967223 - Flags: review?(gandalf)
Attachment #8967223 - Flags: review+
Attachment #8967224 - Flags: review?(stas)
Attachment #8967224 - Flags: review?(jaws)
Attachment #8967224 - Flags: review+
Attachment #8967225 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967225 - Flags: review?(francesco.lodolo)
Attachment #8967225 - Flags: review+
Attachment #8967226 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967226 - Flags: review?(gandalf)
Attachment #8967226 - Flags: review+
Attachment #8967227 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967227 - Flags: review?(francesco.lodolo)
Attachment #8967227 - Flags: review+
Attachment #8967228 - Flags: review?(stas)
Attachment #8967228 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967228 - Flags: review+
Depends on: 1457252
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: