951695 - Consider renaming "Character Encoding" to "Text Encoding"

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Anne (:annevk)]

Chrome and Internet Explorer name this entry "Encoding". Alternatively we could align with Safari which names it "Text Encoding".

Rationale: better to have similar naming for obscure menu items.

Comment 1

[Henri Sivonen (:hsivonen)]

Mere "Encoding" seems even more obscure than "Character Encoding".

Comment 2

[Anne (:annevk)]

I'm not a native speaker, but I think that "Character" does not help people much. "Text" might, but given that our major competitors are not using that, I'm not sure we should.

Comment 3

[Roman Müller]

I like this idea since it would make the label fit into width of the appmenu rather than being split by comma in the middle of Encoding word.

Encoding sounds good to me.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Let's go with "Text Encoding" as "text" is a less-technical term than "character" and I agree with Henri that "Encoding" by itself is a bit obscure.

This change will need to be made in four places:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/charsetMenu.dtd#5
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/fonts.dtd#66
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/fonts.dtd#67
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/fonts.dtd#69

We should also update the comment at http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#402

Note that since we are changing a localized string, we will need to change the entity name as well as update all of the places that it is referenced as well. For example, the three in fonts.dtd are referenced at http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/fonts.xul#247

You can read the section here for more information about best practices for changing existing strings, https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Comment 5

[Abhinav Koppula]

Attachment: Patch for review

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566600 [details] [diff] [review]
Patch for review

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

::: browser/app/profile/firefox.js
@@ +1597,5 @@
>  pref("devtools.telemetry.tools.opened.version", "{}");
>  
>  // Whether the character encoding menu is under the main Firefox button. This
>  // preference is a string so that localizers can alter it.
> +pref("browser.menu.showTextEncoding", "chrome://browser/locale/browser.properties");

We should leave the pref unchanged, since changing this will cause unnecessary work.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +186,5 @@
>      }
>  #endif
>  
>      let showCharacterEncoding = Services.prefs.getComplexValue(
> +      "browser.menu.showTextEncoding",

Since the pref name won't be changed, this change isn't needed.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +406,2 @@
>  # menu, set this to "true". Otherwise, you can leave it as "false".
> +browser.menu.showTextEncoding=false

It is OK to update the localization note, but this should not be renamed to showTextEncoding as that is not user-facing and will create unnecessary work for localizers.

::: browser/locales/en-US/chrome/browser/preferences/fonts.dtd
@@ +63,5 @@
>  <!ENTITY  allowPagesToUse.label                   "Allow pages to choose their own fonts, instead of my selections above">
>  <!ENTITY  allowPagesToUse.accesskey               "A">
>  
> +<!ENTITY languages.customize.Fallback.grouplabel  "Text Encoding for Legacy Content">
> +<!ENTITY languages.customize.Fallback.label       "Fallback Text Encoding:">

These entity names (e.g. "languages.customize.Fallback.grouplabel") will need to be changed since the value is being changed. See the last two paragraphs in comment #4 for more information about this.

@@ +68,1 @@
>  <!ENTITY languages.customize.Fallback.accesskey   "C">

Note also that the entity name for the accesskey will have to be changed too since it is dependent on the label. Changing the entity name gives a notice to localizers that they will need to update their translation.

::: browser/modules/BrowserUITelemetry.jsm
@@ +69,5 @@
>      ],
>    };
>  
>    let showCharacterEncoding = Services.prefs.getComplexValue(
> +    "browser.menu.showTextEncoding",

Since the preference isn't changing, this doesn't need to change.

::: mobile/android/app/mobile.js
@@ +241,5 @@
>  pref("accessibility.browsewithcaret_shortcut.enabled", false);
>  
>  // Whether the character encoding menu is under the main Firefox button. This
>  // preference is a string so that localizers can alter it.
> +pref("browser.menu.showTextEncoding", "chrome://browser/locale/browser.properties");

Ditto.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +112,5 @@
>      private static final String PREFS_SEARCH_RESTORE_DEFAULTS = NON_PREF_PREFIX + "search.restore_defaults";
>      private static final String PREFS_DATA_REPORTING_PREFERENCES = NON_PREF_PREFIX + "datareporting.preferences";
>      private static final String PREFS_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";
>      private static final String PREFS_CRASHREPORTER_ENABLED = "datareporting.crashreporter.submitEnabled";
> +    private static final String PREFS_MENU_TEXT_ENCODING = "browser.menu.showTextEncoding";

Since this is not user-facing it doesn't need to be changed.

::: mobile/android/base/resources/xml/preferences_display.xml
@@ +40,5 @@
>                          android:title="@string/pref_reflow_on_zoom"
>                          android:defaultValue="false"
>                          android:persistent="false" />
>  
> +        <ListPreference android:key="browser.menu.showTextEncoding"

This change won't be necessary since the preference name isn't changing.

::: mobile/android/chrome/content/browser.js
@@ +6649,5 @@
>  
>    sendState: function sendState() {
>      let showCharEncoding = "false";
>      try {
> +      showCharEncoding = Services.prefs.getComplexValue("browser.menu.showTextEncoding", Ci.nsIPrefLocalizedString).data;

This change won't be necessary since the preference name isn't changing.

::: toolkit/locales/en-US/chrome/global/charsetMenu.dtd
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!ENTITY charsetMenu.label             "Text Encoding">

The entity name here should be changed to charsetMenu2.label, and the accesskey entity name should be changed to charsetMenu2.accesskey as well.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

<jaws> shorlander: hey, are you OK with renaming Character Encoding to Text Encoding? (bug 951695)
<shorlander> jaws: even though I don't find "character" to be too technical or obscure in the context of an already technical and obscure item, "text" does imply the same thing while being more colloquial
<shorlander> jaws: I agree that just "encoding" is not enough
<shorlander> jaws: so, I am OK with it :) even though I don't see it as a huge win
<jaws> ok thanks!

Comment 8

[Tracy Walker [:tracy]]

Something to consider; we already use simply "Encoding" in Tools > Page Info > General

Comment 9

[Abhinav Koppula]

Attachment: characterEncodingRename.patch

Reverted back unnecessary changes and changed entity names and their references everywhere

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566981 [details] [diff] [review]
characterEncodingRename.patch

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

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #8)
> Something to consider; we already use simply "Encoding" in Tools > Page Info
> > General

Good catch, we should probably switch that one over to "Text Encoding" as well.

::: browser/locales/en-US/chrome/browser/preferences/fonts.dtd
@@ +64,5 @@
>  <!ENTITY  allowPagesToUse.accesskey               "A">
>  
> +<!ENTITY languages.customize.Fallback2.grouplabel  "Text Encoding for Legacy Content">
> +<!ENTITY languages.customize.Fallback2.label       "Fallback Text Encoding:">
> +<!ENTITY languages.customize.Fallback2.accesskey   "C">

This accesskey should be changed to "T".

::: toolkit/locales/en-US/chrome/global/charsetMenu.dtd
@@ +2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!ENTITY charsetMenu2.label             "Text Encoding">
> +<!ENTITY charsetMenu2.accesskey         "C">

The accesskey should be changed here to a lower-case "c". Usually we use the first-letter, but "T" is already taken by "Toolbars", and it would be nice here to keep using "c" since it will match muscle memory since this is what is referenced by the menus.

Comment 11

[Abhinav Koppula]

Attachment: characterEncodingRename2.patch - Changed PageInfo > General - "Encoding" to "Text Encoding" and changed accessKeys

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8567620 [details] [diff] [review]
characterEncodingRename2.patch - Changed PageInfo > General - "Encoding" to "Text Encoding" and changed accessKeys

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

This looks good. I built your patches and noticed that it is missing still one place. In Customize Mode (Firefox menu button > Customize), you'll see that there is a "Character Encoding" button.

This string is located at http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties#87.

Comment 13

[Abhinav Koppula]

Attachment: characterEncodingRename3.patch

Changed Customize > Character Encoding to Text Encoding and the corresponding tooltip

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8569323 [details] [diff] [review]
characterEncodingRename3.patch

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

Are you able to fold these patches together? If you are using Mercurial Queues, you can use `hg qfold` to fold patches together in to one patch.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +757,5 @@
>    }, {
>      id: "characterencoding-button",
>      type: "view",
>      viewId: "PanelUI-characterEncodingView",
> +    tooltiptext: "characterencoding-button.tooltiptext3",

This should be updated to characterencoding-button2.tooltiptext and you will also need to add a line for `label: "characterencoding-button2.label",` since the prefix of the label now no longer matches the ID ("characterencoding-button").

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +83,5 @@
>  
>  # LOCALIZATION NOTE (characterencoding-button.label): The \u00ad character at the beginning
>  # of the string is used to disable auto hyphenation on the button text when it is displayed
>  # in the menu panel.
> +characterencoding-button.label = \u00adText Encoding

The entity name, "characterencoding-button.label", needs to change since the value here has changed. Also, the localization note above it should have the referenced entity name updated.

We can use characterencoding-button2.label as the entity name here, since it is preferred that the suffix remain just "label".

Also, since we had to change the entity name for characterencoding-button.tooltiptext2, we should keep this consistent and rename that to characterencoding-button2.tooltiptext.

Comment 15

[Abhinav Koppula]

Attachment: bug-951695-patch.patch

merged all patches

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570598 [details] [diff] [review]
bug-951695-patch.patch

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

Looks good, nice job :)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

I pushed your changes to the fx-team repository. It should get merged to mozilla-central in about a day and then the following day the changes will appear in Firefox Nightly. Nice job! I'm looking forward to more of your work in bug 732688 and bug 1136526 :)

https://hg.mozilla.org/integration/fx-team/rev/6b4aa57860cf

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b4aa57860cf

Comment 19

[Ian Neal]

Is there a need to do the character encoding strings in:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd

Does the label for this test need fixing too?
http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_962884_opt_in_disable_hyphens.js#8

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

It would be nice to maintain consistency across platforms so we should update the Android strings to match desktop. I can't look in to the test right now but ideally it would match the browser's default behavior.