Defect - Plural form strings are excessively complex for both devs and translators

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwilde, Assigned: jwilde)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=3)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 859003
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=0
(Assignee)

Comment 1

5 years ago
p=1
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=0 → feature=defect c=firefox_start u=metro_firefox_user p=1

Updated

5 years ago
Blocks: 886790
No longer blocks: 859003
Priority: -- → P2
QA Contact: jbecerra
(Assignee)

Comment 2

5 years ago
Created attachment 777415 [details] [diff] [review]
v1
Attachment #777415 - Flags: review?(mbrubeck)
Attachment #777415 - Flags: feedback?(francesco.lodolo)
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.
(Assignee)

Comment 4

5 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?
Flags: needinfo?(ywang)
(Assignee)

Updated

5 years ago
Attachment #777415 - Flags: review?(mbrubeck)
Attachment #777415 - Flags: feedback?(francesco.lodolo)
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!
Flags: needinfo?(ywang)
(Assignee)

Comment 8

5 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.
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 778213 [details] [diff] [review]
fix-plurals-v2.patch
Attachment #777415 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Summary: Defect - Missing localization notes for plural form strings → Defect - Plural form strings are excessively complex for both devs and translators
(Assignee)

Comment 11

5 years ago
Created attachment 778217 [details] [diff] [review]
patch v2.1
Attachment #778213 - Attachment is obsolete: true
Attachment #778217 - Flags: review?(mbrubeck)
Attachment #778217 - Flags: feedback?(francesco.lodolo)
(Assignee)

Comment 12

5 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.
(Assignee)

Comment 13

5 years ago
Also, MarcoM: bumping this to p=3.
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=1 → feature=defect c=firefox_start u=metro_firefox_user 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.
Attachment #778217 - Flags: feedback?(francesco.lodolo) → feedback+
(Assignee)

Comment 15

5 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.
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
Attachment #778217 - Flags: review?(mbrubeck) → review+
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.
(Assignee)

Comment 20

5 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.
(Assignee)

Comment 21

5 years ago
Created attachment 780635 [details] [diff] [review]
patch v2.2

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!
Attachment #780635 - Flags: review?(mbrubeck)
Attachment #780635 - Flags: feedback?(francesco.lodolo)
(Assignee)

Updated

5 years ago
Attachment #778217 - Attachment is obsolete: true
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.
Attachment #780635 - Flags: review?(mbrubeck) → review+
Comment on attachment 780635 [details] [diff] [review]
patch v2.2

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

Looks good to me
Attachment #780635 - Flags: feedback?(francesco.lodolo) → feedback+
https://hg.mozilla.org/mozilla-central/rev/2acba061b849
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.