As Francesco Lodolo discussed in bug 867543, we need to add extra localization notes in order for localizers translate plural forms: "Sadly some of our tools (compare-locales) and some external tools used for localization rely on the presence of this comment to identify a string with plural form. # LOCALIZATION NOTE (STRINGID): Semi-colon list of plural forms. # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals The bad part is that his comment must be added before each string using a plural form."

p=1

Comment on attachment 777415 [details] [diff] [review] v1 Review of attachment 777415 [details] [diff] [review]: ----------------------------------------------------------------- Based on bug 883951 comment 43, perhaps we should not be using PluralForm here at all.

(In reply to Matt Brubeck (:mbrubeck) from comment #3) > Based on bug 883951 comment 43, perhaps we should not be using PluralForm > here at all. Sounds good. There's also some UI fugliness with trying to change the pluralization--the width of the buttons "jump" without animation. Instead of the following: - Pin top site(s) - Unpin top site(s) - Delete top site(s) - Restore top site(s) How about: - Pin to Top Sites - Unpin from Top Sites - Delete - Undo delete This more closely matches the grammar choices of the Windows 8 start screen, too. Yuan, does this work?

I think you need to consider also this comment https://bugzilla.mozilla.org/show_bug.cgi?id=883951#c43

By the way, some Unpin strings are seriously messed up. contextAppbar.unpin.tab=Unpin page;Unpin tabs contextAppbar.unpin.download=Unpin page;Unpin downloads contextAppbar.unpin.download=Unpin page;Unpin downloads

(In reply to Jonathan Wilde [:jwilde] from comment #4) > (In reply to Matt Brubeck (:mbrubeck) from comment #3) > > Based on bug 883951 comment 43, perhaps we should not be using PluralForm > > here at all. > > Sounds good. There's also some UI fugliness with trying to change the > pluralization--the width of the buttons "jump" without animation. > > Instead of the following: > - Pin top site(s) > - Unpin top site(s) > - Delete top site(s) > - Restore top site(s) > > How about: > - Pin to Top Sites > - Unpin from Top Sites > - Delete > - Undo delete > > This more closely matches the grammar choices of the Windows 8 start screen, > too. > > Yuan, does this work? The new strings look fine with me. Thanks Jonathan!

(In reply to Francesco Lodolo [:flod] from comment #6) > By the way, some Unpin strings are seriously messed up. > > contextAppbar.unpin.tab=Unpin page;Unpin tabs > contextAppbar.unpin.download=Unpin page;Unpin downloads > contextAppbar.unpin.download=Unpin page;Unpin downloads Good catch! Thanks for sending comment 43 along, too. It looks like we're going to rework the strings to avoid plurals entirely as described above. The current set of strings is just too large and complicated, regardless of the plural logic issues. If you wouldn't mind, I'll also feedback? you on the patch just to make sure everything checks out before landing.

(In reply to Jonathan Wilde [:jwilde] from comment #8) > If you wouldn't mind, I'll also feedback? you on the patch just to make sure > everything checks out before landing. I'll be glad to check it.

Comment on attachment 778217 [details] [diff] [review] patch v2.1 Review of attachment 778217 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/locales/en-US/chrome/browser.dtd @@ +22,5 @@ > > +<!-- START AND PANEL UI --> > + > +<!ENTITY topSitesHeader.label "Top Sites"> > +<!ENTITY recentHistoryHeader.label "Recent History"> We actually use these labels in a few places that aren't the Start screen. They're pretty generic headers for types of lists/grids of pages now, so renaming to reflect that. Also, renaming "history" strings to "recentHistory" since I have a hunch we'll end up using "Recent History" for abridged lists on the Start screen and the like and "History" for full lists. This is going to be a bit down the road, but wanted to prep for that before we freeze strings.

Also, MarcoM: bumping this to p=3.

Comment on attachment 778217 [details] [diff] [review] patch v2.1 Review of attachment 778217 [details] [diff] [review]: ----------------------------------------------------------------- Strings look good to me. ::: browser/metro/locales/en-US/chrome/browser.dtd @@ +34,1 @@ > The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' --> I'm wondering why not using a different character than ">" to make it more clear that it's not supposed to be localized.

(In reply to Francesco Lodolo [:flod] from comment #14) > Comment on attachment 778217 [details] [diff] [review] > patch v2.1 > > Review of attachment 778217 [details] [diff] [review]: > ----------------------------------------------------------------- > > Strings look good to me. Thanks for the review! :D > ::: browser/metro/locales/en-US/chrome/browser.dtd > @@ +34,1 @@ > > The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' --> > > I'm wondering why not using a different character than ">" to make it more > clear that it's not supposed to be localized. Agreed. Or better yet, since the '>' is intended to represent an arrow pointing to more items, we should just move it into a separate element after the label containing the string and remove the '>' entirely... Our existing RTL logic would handle all of the locale issues automatically, too.

Comment on attachment 778217 [details] [diff] [review] patch v2.1 Review of attachment 778217 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/content/appbar.js @@ +223,5 @@ > this.showContextualActions([]); > }, > > + _updateContextualActionLabel: function(aButton, aVerb, aSetName) { > + Util.dumpLn("btn: " + aButton + " verb: " + aVerb + " set name: " + aSetName); Please remove the debugging output before checkin. ::: browser/metro/locales/en-US/chrome/browser.dtd @@ +34,1 @@ > The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' --> In response to flod's comment: Jonathan recently pointed out this Windows 8 symbol font, which has "progressive disclosure arrows" we could use for this purpose: http://msdn.microsoft.com/en-us/library/windows/apps/jj841126.aspx We'll try that in a separate bug. ::: browser/metro/locales/en-US/chrome/browser.properties @@ +15,5 @@ > browser.search.contextTextSearchLabel2=Search %S for "%S" > > +# Contextual Appbar - Button Labels > + > +# LOCALIZATION NOTE (contextAppbar2.pin.*): "Pins" selected pages so they stay I'm not sure how the .* will interact with l10n tooling -- flod or Pike, do you know? Perhaps this should just be a "file-wide comment" with no () portion: https://developer.mozilla.org/en-US/docs/Localization_notes

Smart, I missed the localization comments :-\ The only safe way to ensure compatibility with existing tools is to provide all the keys the comment is referred to.

The plan is to move the '>' out of the strings and likely into CSS - or some similar solution that will still meet right-to-left considerations. That work will be a part of the responsive start page work (bug 892512) but may merits its own bug for tracking purposes.

We had the arrow because we were opening a new panel when clicking the labels. After the fix for bug 892046, they list of items will expand/shrink when you click the labels, so we'll probably change to a '+' or a down arrow. Best to remove from the string anyway.

In terms of plan of action moving forward with this: - Let's have the '>' get moved out of the strings with bug 892512. - I'll move the appbar2.pin.* style comments into a file-wide localization comment.

mbrubeck: Had to do some merging into the new snapped states. It passes all tests and seems to work, but wanted to pass it through review again to make sure I didn't mess anything up. flod: Moved the comments for pin and unpin into a file-wide comment. Could you let me know if it makes sense? Thanks!

Comment on attachment 780635 [details] [diff] [review] patch v2.2 Review of attachment 780635 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/content/appbar.js @@ +223,5 @@ > this.showContextualActions([]); > }, > > + _updateContextualActionLabel: function(aButton, aVerb, aSetName) { > + Util.dumpLn("btn: " + aButton + " verb: " + aVerb + " set name: " + aSetName); Please remove the debug output before checkin.

Comment on attachment 780635 [details] [diff] [review] patch v2.2 Review of attachment 780635 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me