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

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Menus
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: masayuki, Assigned: pcwalton)

Tracking

({intl, jp-critical})

Trunk
Firefox 4.0b7
intl, jp-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings])

Attachments

(1 attachment, 3 obsolete attachments)

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).
blocking2.0: --- → ?
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
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.
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).
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.
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).
(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.
This work requires strings and locale specific checking. Blocking b7+
blocking2.0: ? → beta7+
Whiteboard: [strings]
(Assignee)

Updated

8 years ago
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
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
(Assignee)

Comment 9

8 years ago
(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?
(Assignee)

Comment 10

8 years ago
Preliminary patch for the hidden pref and locale-specific setting created. Now testing on Windows.
Stephen might be able to provide a glyph here.
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!
Keywords: icon
(Assignee)

Comment 13

8 years ago
Created attachment 476964 [details] [diff] [review]
Proposed patch.

Proposed patch attached. Feedback, UI review, and review requested.
Attachment #476964 - Flags: ui-review?(faaborg)
Attachment #476964 - Flags: review?(gavin.sharp)
Attachment #476964 - Flags: feedback?(masayuki)
Whiteboard: [strings] → [strings][has patch][needs review gavin][needs ui-r faaborg]
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 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.
Attachment #476964 - Flags: feedback?(masayuki) → feedback-
(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).
(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 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.
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.
(Assignee)

Comment 20

8 years ago
(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?
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.
(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.
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).
(Assignee)

Comment 24

8 years ago
Created attachment 477224 [details] [diff] [review]
Proposed patch, version 2.

New version of the patch uses a localized string preference. Feedback and review requested.
Attachment #476964 - Attachment is obsolete: true
Attachment #477224 - Flags: ui-review?(faaborg)
Attachment #477224 - Flags: review?(gavin.sharp)
Attachment #477224 - Flags: feedback?(l10n)
Attachment #476964 - Flags: ui-review?(faaborg)
Attachment #476964 - Flags: review?(gavin.sharp)
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.
Attachment #477224 - Flags: ui-review?(faaborg) → ui-review+
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.
Attachment #477224 - Flags: ui-review?(faaborg)
Attachment #477224 - Flags: ui-review+
Attachment #477224 - Flags: review?(gavin.sharp)
Attachment #477224 - Flags: review+
Attachment #477224 - Flags: feedback?(l10n) → feedback+
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/.
Whiteboard: [strings][has patch][needs review gavin][needs ui-r faaborg] → [strings][can land]
Attachment #477224 - Flags: ui-review?(faaborg) → ui-review+
(Assignee)

Comment 28

8 years ago
Created attachment 477238 [details] [diff] [review]
Proposed patch, version 2.1.

New patch addresses review comments.
(Assignee)

Comment 29

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

Oops, left a debug string in there.
Attachment #477238 - Attachment is obsolete: true
(Assignee)

Comment 30

8 years ago
Created attachment 477240 [details] [diff] [review]
Proposed patch, version 2.2.

New patch removes the debug message.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Attachment #477224 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/02e202603e08
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [strings][can land] → [strings]
Target Milestone: --- → Firefox 4.0b7
Keywords: icon
(Assignee)

Comment 32

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

Updated

8 years ago
Depends on: 616837

Updated

8 years ago
Blocks: 628183
You need to log in before you can comment on or make changes to this bug.