Closed Bug 1185845 Opened 5 years ago Closed 4 years ago

Fix localization issues in one-off searches: remove contatenations, "Change Search Settings" is hard-coded

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: flod, Assigned: nhnt11)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a landing from bug 1171344
http://hg.mozilla.org/mozilla-central/diff/b2a96402ed45/browser/locales/en-US/chrome/browser/search.properties

It's really bad from a localization point of view.

> searchFor=Search for 
> searchWith= with:

1) Unless I'm mistaken, whitespaces before and after the separator are simply ignored unless they're in escaped form.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Property_Files#Escape_non-ASCII_Characters

2) Concatenations are bad.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Avoid_concatenations_use_placeholders_instead

Unless the are huge technical limitations, we should avoid them at all costs and use something like "Search for #1 with:", and replace #1 with whatever needed.

> searchWithHeader=Search with:

Is this concatenated with the name of the searchengine? Having said that concatenations are bad, if you decide to take that road you always need to provide a "before" and "after" string. Not having that forces a grammar structure on other languages.
Side note: escaped spaces are pretty confusing, and might confuse some tools too.
Some context on why it was done this way in bug 1171344:
The design specified in bug 1106057 has the query text styled differently than the rest of the string. It is probably possible to format the string by replacing the "#1" or whatever with HTML code (rather unclean but it would work), but there was precedent in the main searchbox code so we went the concatenation route.

As for the searchWithHeader - no, there is no concatenation going on there. It stays as "Search with:".
(In reply to Nihanth Subramanya [:nhnt11] from comment #2)
> Some context on why it was done this way in bug 1171344:
> The design specified in bug 1106057 has the query text styled differently
> than the rest of the string. It is probably possible to format the string by
> replacing the "#1" or whatever with HTML code (rather unclean but it would
> work), but there was precedent in the main searchbox code so we went the
> concatenation route.

This is the involved code, isn't it?
https://hg.mozilla.org/mozilla-central/rev/b2a96402ed45#l7.561

I don't think replacing the placeholder with the <span> HTML code would be that awful.

> As for the searchWithHeader - no, there is no concatenation going on there.
> It stays as "Search with:".

Thanks (missed the UX design in bug 1106057, so couldn't figure out how this was actually used).
Also realized that "Change Search Settings" is hard-coded
https://hg.mozilla.org/mozilla-central/rev/b2a96402ed45#l7.744
Summary: Fix localization issues in one-off searches → Fix localization issues in one-off searches: remove contatenations, "Change Search Settings" is hard-coded
Attached patch Patch (obsolete) — Splinter Review
(In reply to Francesco Lodolo [:flod] from comment #4)
> Also realized that "Change Search Settings" is hard-coded
> https://hg.mozilla.org/mozilla-central/rev/b2a96402ed45#l7.744

That's embarrassing.
Attachment #8637104 - Flags: review?(adw)
Comment on attachment 8637104 [details] [diff] [review]
Patch

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

Thanks! String-wise it looks good, only one localization comment to update.

::: browser/locales/en-US/chrome/browser/search.properties
@@ +40,5 @@
>  
>  # LOCALIZATION NOTE (searchWithHeader):
>  # The wording of this string should be as close as possible to
>  # searchFor and searchWith. This string will be used instead of
>  # them when the user has not typed any keyword.

Need to update this comment too.
Attachment #8637104 - Flags: feedback+
Attached patch Patch v1.1Splinter Review
Updated comment.
Attachment #8637104 - Attachment is obsolete: true
Attachment #8637104 - Flags: review?(adw)
Attachment #8637112 - Flags: review?(adw)
Attachment #8637112 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/f389c9c1eafb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1208141
You need to log in before you can comment on or make changes to this bug.