Jonathan Wilde [:jwilde] Assignee
	Description • 11 years ago

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."

	Jonathan Wilde [:jwilde] Assignee
	Comment 1 • 11 years ago

p=1

	Matt Brubeck (:mbrubeck)
	Comment 3 • 11 years ago

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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 4 • 11 years ago

(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?

	Francesco Lodolo [:flod]
	Comment 5 • 11 years ago

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

	Francesco Lodolo [:flod]
	Comment 6 • 11 years ago

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

	Yuan Wang(:Yuan) – Mobile Firefox Design Lead
	Comment 7 • 11 years ago

(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!

	Jonathan Wilde [:jwilde] Assignee
	Comment 8 • 11 years ago

(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.

	Francesco Lodolo [:flod]
	Comment 9 • 11 years ago

(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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 12 • 11 years ago

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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 13 • 11 years ago

Also, MarcoM: bumping this to p=3.

	Francesco Lodolo [:flod]
	Comment 14 • 11 years ago

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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 15 • 11 years ago

(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.

	Matt Brubeck (:mbrubeck)
	Comment 16 • 11 years ago

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

	Francesco Lodolo [:flod]
	Comment 17 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him)
	Comment 18 • 11 years ago

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.

	Rodrigo Silveira [:rsilveira] [:rodms]
	Comment 19 • 11 years ago

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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 20 • 11 years ago

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.

	Jonathan Wilde [:jwilde] Assignee
	Comment 21 • 11 years ago

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!

	Matt Brubeck (:mbrubeck)
	Comment 22 • 11 years ago

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.

	Francesco Lodolo [:flod]
	Comment 23 • 11 years ago

Comment on attachment 780635 [details] [diff] [review]
patch v2.2

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

Looks good to me

	Jonathan Wilde [:jwilde] Assignee
	Comment 24 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2acba061b849

	Ed Morley [:emorley]
	Comment 25 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2acba061b849