596173 - "Character Encoding" menu should be in top level of Firefox Menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Now, "Character Encoding" menu is in "Developer" submenu. However, in some locales, the menu isn't only for developers, so, localizers should be able to show the "Character Encoding" menu in top level.

I think that the menu should be near "Full Screen" because they are in "View" menu on menubar. And looks like "Zoom" will be there (bug 592147).

Comment 1

[dynamis (Tomoya ASAI)]

In short: I agree with Masayuki.

Details:
In languages which use multiple encoding, we still sometimes see garbaged
character page because of wrong encoding.
# One of the most famous FAQ in Japan is, how to change character encoding.

In that case users should be able to change character encoding from basic menu
(without knowledge how to show standard menu with alt key) and Bug 575182
included "Character Encoding" menu into the Firefox Menu.

But users (of locales with multiple encoding) don't think that changing
encoding is the feature for developers. They cannot notice about the menu if
it's under the "Developer" menu.
So localizers should be able to choose if "Character Encoding" menu is in top
level of Firefox Menu or under the "Developer".

Since general user think fixing wrongly selected encoding is view-related menu,
the menu should be next to "Zoom" and "Full Screen" menus.

To keep consistency with locales which choose to keep the menu under the
Developer menu, I feel "Character Encoding" menu should be placed near
"Developer" menu. Then users can easily find the menu even when they use
another locale Firefox.

Though I don't think this is good idea, another idea is, if we have placeholder
to insert menus by extensions, it means the placeholder of "any other menus"
and we can include "Character Encoding" menu too within the placeholder.
# above developer menu as far as I see this screenshot:
# https://bug592147.bugzilla.mozilla.org/attachment.cgi?id=470654

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

The ux team fully agrees with this proposal as well.  When preffed on it should appear above full screen.  Also, in that case we shouldn't remove it from the developer menu, just in case people have built up paths.  It's not great that it's redundant, but I think that's better than people feeling like the menu item is jumping around when they move between different Firefox locals.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have an additional request. Many our contributors (typically testers and developers) are using English nightly/hourly builds.  If their language is Japanese or something, they may want to see the menu on top level.

So, I hope that this should be customizable with a pref (i.e., don't decide it at build time).

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

if we could do this with an about:config pref, I think that's the right level of control without incresing the complexity of our options window.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Adding a character encoding button to the toolbar customization pallete might also be a good way to give users the option of having easy access to this functionality (even faster than having to go into a submenu off of the main Firefox menu).

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #4)
> if we could do this with an about:config pref, I think that's the right level
> of control without incresing the complexity of our options window.

I agree, it's enough.

(In reply to comment #5)
> Adding a character encoding button to the toolbar customization pallete might
> also be a good way to give users the option of having easy access to this
> functionality (even faster than having to go into a submenu off of the main
> Firefox menu).

Sounds great.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

This work requires strings and locale specific checking. Blocking b7+

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, so the call here is to put it in the top-level menu, above "Full Screen", and have it be:

 - hidden pref based
 - also locale based

Comment 9

[Patrick Walton (:pcwalton)]

(In reply to comment #6)
> (In reply to comment #5)
> > Adding a character encoding button to the toolbar customization pallete might
> > also be a good way to give users the option of having easy access to this
> > functionality (even faster than having to go into a submenu off of the main
> > Firefox menu).
> 
> Sounds great.

I don't have an icon for this, and I don't trust my graphic design skills enough to draw one. Should I drop, add a text-only toolbar item, or file a followup bug?

Comment 10

[Patrick Walton (:pcwalton)]

Preliminary patch for the hidden pref and locale-specific setting created. Now testing on Windows.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Stephen might be able to provide a glyph here.

Comment 12

[Johnathan Nightingale [:johnath]]

And in the absence of stephen doing so in the time it takes you to produce a patch and get it reviewed - file (and nominate!) a follow up bug.

Thanks!

Comment 13

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached. Feedback, UI review, and review requested.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

So we're OK with the menus appearing both at the top level and under "Developer" for the users/locales that opt-in? Kind of seems like perhaps it should only be in one place or the other...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 476964 [details] [diff] [review]
Proposed patch.

The pref isn't customizable for localizers.

See http://mxr.mozilla.org/mozilla-central/search?string=font.language.group

A localizer pref must be defined with chrome URI which is a path to a properties file. And you must check the pref as string.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> A localizer pref must be defined with chrome URI which is a path to a
> properties file. And you must check the pref as string.

That's not true - the pref could just be added to firefox-l10n.js as a normal non-localized pref (same way general.useragent.locale is). I don't know whether that's now frowned upon for some reason, but we seem to be using it for some things similar to this (e.g. browser.fixup.alternate.suffix).

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> (In reply to comment #15)
> > A localizer pref must be defined with chrome URI which is a path to a
> > properties file. And you must check the pref as string.
> 
> That's not true - the pref could just be added to firefox-l10n.js as a normal
> non-localized pref (same way general.useragent.locale is).

Wow, there was a rule, localizers can change only a few prefs. Has the rule been gone? Sounds the new mechanism can change all prefs by localizers.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 476964 [details] [diff] [review]
Proposed patch.

>+  const showCharacterSetMenuPref = "browser.menu.showCharacterSetMenu";
>+  if (!gPrefService.prefHasUserValue(showCharacterSetMenuPref) ||
>+      !gPrefService.getBoolPref(showCharacterSetMenuPref)) {

You should add the pref to firefox.js with the desired default value and don't call prefHasUserValue.

Comment 19

[Axel Hecht [:Pike]]

Please don't endorse localizers to set prefs in firefox-l10n.js. It's a can of worms I'd rather not get in to.

Comment 20

[Patrick Walton (:pcwalton)]

(In reply to comment #19)
> Please don't endorse localizers to set prefs in firefox-l10n.js. It's a can of
> worms I'd rather not get in to.

In that case I'll move it to intl.properties. Should I rename the pref so that it starts with "intl." as well?

Comment 21

[Axel Hecht [:Pike]]

Hrm. I'm torn on whether this is a toolkit pref or not. I'd rather say not.

I think it belongs more into http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties.

I'd keep the name you had, fwiw.

That'd turn the pref value in a string, from all I can tell (jetlag), you want to make that bullet proof against people translating it, so the default should be what en-US does, and only the exact opposite as given in the localization note should switch it on.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> (In reply to comment #19)
> > Please don't endorse localizers to set prefs in firefox-l10n.js. It's a can of
> > worms I'd rather not get in to.
> 
> In that case I'll move it to intl.properties. Should I rename the pref so that
> it starts with "intl." as well?

I think "browser." is good thing. Whether the pref is needed or not depends on product.

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

I'll need a screen shot for the a quick ui-review.  Note that the option should still remain in the developer menu, and should be duplicated into the main menu.  The menu item should not have an icon (these are only given to the most commonly used items to make them easier to target quickly).

Comment 24

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

New version of the patch uses a localized string preference. Feedback and review requested.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 477224 [details] [diff] [review]
Proposed patch, version 2.

Note that I didn't actually apply the patch to do the review, but reading through it the behavior seems correct.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 477224 [details] [diff] [review]
Proposed patch, version 2.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  enabled = gPrefService.getComplexValue(showCharacterEncodingPref,
>+                                         Ci.nsIPrefLocalizedString);

nit: don't reuse "enabled" - "let extraCharsetMenuEnabled"?

>+  if (enabled.toString() !== "true") {

Use getComplexValue().data and avoid the toString()

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

>+// 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.showCharacterEncoding", "chrome://browser/locale/browser.properties");

This belongs in firefox.js.

Might help to have the l10n note be more explicit about this being a behavior change rather than an actual string. Perhaps with a "DON'T LOCALIZE UNLESS ..." with examples of why you'd want to change it.

r=me with those addressed.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 477224 [details] [diff] [review]
Proposed patch, version 2.

> diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
> --- a/modules/libpref/src/init/all.js
> +++ b/modules/libpref/src/init/all.js

Why don't you add the new pref to firefox.js rather than all.js? That isn't needed by other applications. And it refers browser/.

Comment 28

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.1.

New patch addresses review comments.

Comment 29

[Patrick Walton (:pcwalton)]

Comment on attachment 477238 [details] [diff] [review]
Proposed patch, version 2.1.

Oops, left a debug string in there.

Comment 30

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.2.

New patch removes the debug message.

Comment 31

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/02e202603e08

Comment 32

[Patrick Walton (:pcwalton)]

For the toolbar button follow-up, see bug 598516.