PDF viewer should use a single unicode character instead of "..." and should have localization comments

RESOLVED FIXED in Firefox 15

Status

()

Firefox
PDF Viewer
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: flod, Assigned: Gavin)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox15+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
http://hg.mozilla.org/mozilla-central/file/65f748c63f4c/browser/locales/en-US/pdfviewer/viewer.properties

loading=Loading... {{percent}}%

All Mozilla's products use "…" instead of "...". Also, that file need at least an initial localization comment explaining what should be left untranslated (variables).

text_annotation_type=[{{type}} Annotation]
This string needs a l10n comment explaining what "type" could be.
Created attachment 629965 [details] [diff] [review]
patch
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #629965 - Flags: review?(bdahl)

Comment 2

5 years ago
Comment on attachment 629965 [details] [diff] [review]
patch

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

::: browser/locales/en-US/pdfviewer/viewer.properties
@@ +11,3 @@
>  page_label=Page:
>  page_of=of {{pageCount}}
> +

I don't expect this to work in general. I'd expect a combi of double plurals here, similar to what we do in download manager. Notably, there could be text that depends on the current page number preceding the current page number.

I know this is a comment fix, but it makes the code problem more obvious.

@@ +24,5 @@
>  download_label=Download
>  bookmark.title=Current view (copy or open in new window)
>  bookmark_label=Current View
>  
> +# Side panel toolbar button tooltips

As a non-native speaker, I can't parse this string.

@@ +37,1 @@
>  thumbs_label=Thumbnails

What's the point of the re-ordering here? Why's thumbs_label separate from thumbs.title?

(yeah, _ and ., sob).

@@ +49,1 @@
>  thumb_page_canvas=Thumbnail of Page {{page}}

I wonder if these should use plural forms. flod?

@@ +83,5 @@
>  loading_error_indicator=Error
>  loading_error=An error occurred while loading the PDF.
>  
>  # Misc labels and messages
> +# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage

Can't match this comment to the string.
(In reply to Axel Hecht [:Pike] from comment #2)
> I know this is a comment fix, but it makes the code problem more obvious.

Yes, indeed. Too late to change this now, though... We can follow up on trunk?

> > +# Side panel toolbar button tooltips
> 
> As a non-native speaker, I can't parse this string.

Would "Tooltips for side panel toolbar buttons" be clearer?

> What's the point of the re-ordering here? Why's thumbs_label separate from
> thumbs.title?

I thought it'd make it clearer how the strings differed. Really these should use .label/.tooltip, but changing the string names is too much of a PITA at this point.

> > +# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage
> 
> Can't match this comment to the string.

Fixed this per discussion on IRC:
-# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage
+# LOCALIZATION NOTE (text_annotation_type): This is used as a tooltip.
+# "{{[type}}" will be replaced with an annotation type from a list defined in
+# the PDF spec (32000-1:2008 Table 169 – Annotation types).
+# Some common types are e.g.: "Check", "Text", "Comment", "Note"
Created attachment 629985 [details] [diff] [review]
updated patch
Attachment #629965 - Attachment is obsolete: true
Attachment #629965 - Flags: review?(bdahl)
> I thought it'd make it clearer how the strings differed. Really these should use .label/.tooltip, but changing the string names is too much of a PITA at this point

Currently PDF.js is using webL10n for localization of the HTML code. The dot sign is reserved for separation the element localization id from the HTML attribute name. So it will not be possible to change "xxx.yyy" to "xxx_yyy" and vice verse, without adding the complexity to the HTML code localization.
Created attachment 629987 [details] [diff] [review]
updated again

Note that the only functional change here is the change to the ellipsis.
Attachment #629985 - Attachment is obsolete: true
Attachment #629987 - Flags: feedback?(l10n)

Comment 7

5 years ago
Comment on attachment 629987 [details] [diff] [review]
updated again

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

The tooltip vs title story seems to be awkward, but not part of this patch, f=me to get some comments in for our l10n community.
Attachment #629987 - Flags: feedback?(l10n) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee5cd8316af
https://hg.mozilla.org/releases/mozilla-aurora/rev/36066dd58839
status-firefox15: --- → fixed
tracking-firefox15: --- → +
Target Milestone: --- → Firefox 16
(Reporter)

Comment 9

5 years ago
> @@ +49,1 @@
> >  thumb_page_canvas=Thumbnail of Page {{page}}
> 
> I wonder if these should use plural forms. flod?

I don't think so, I expect {{page}} to be a page number, and thumbnail to remain singular.

Unfortunately all these features land way too late on trunk, so we'll never get the chance to fix them before the switch to aurora (another example is GLCI, I just found 2 hard-coded strings...).

Another doubt:
page_scale_width=Page Width
page_scale_fit=Page Fit

I expect the first to be "Fit to Width" and the second "Fit to Page", at least if I got the real meaning of the first right.
https://hg.mozilla.org/mozilla-central/rev/eee5cd8316af
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.