Closed Bug 575182 Opened 14 years ago Closed 14 years ago

Cannot change Character Encoding from Firefox Menu Button

Categories

(Firefox :: Menus, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: masayuki, Assigned: Margaret)

References

Details

(Keywords: intl, jp-critical)

Attachments

(1 file, 3 obsolete files)

The Firefox Menu Button doesn't have Character Encoding menu, so, we cannot change the page's encoding to right one when Fx failed to detect the right one.
FYI: There is bug 63054, so, if we don't want to add them to the Firefox Menu Button, now might be good chance designing new UI.
Component: Disability Access → General
QA Contact: disability.access → general
blocking2.0: --- → ?
Keywords: intl
Reproduces on Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100711 Minefield/4.0b2pre 

Same problem when changing encoding of Page Source
Blocking for at least a decision/resolution. I'm pretty sure we don't want the charset selector in the firefox menu, but I don't know what the solution is for when we get it wrong.
Assignee: nobody → limi
blocking2.0: ? → beta3+
Note that the old menubar is still available if you press F10 or Alt. Unfortunately, not many people seem to know that.
Limi/Faaborg: can we get a decision as asked for in comment 3? Do we want to selectively show the character encoding menuItem for locales which need it more than others (which tends to be the Asian ones, from what I can tell)
Keywords: uiwanted
Default placement is in the developer menu, locals can opt to move it to the overflow extension area so that it is directly in the main menu:

http://people.mozilla.com/~faaborg/files/20100718-firefoxButtonDetails/firefoxButton-i4.png
That sounds reasonable - is that where it is in beta3? I'm happy to move this to a b4 blocker that depends on some bug implementing the changes to the Firefox menu, but don't want to let this slip out of view.
Keywords: uiwanted
>That sounds reasonable - is that where it is in beta3?

As far as I can tell the Firefox menu isn't actively being worked on at the moment.  If the freeze is monday, we are looking at beta 4.
Not sure that Limi is the best assignee for this bug.
blocking2.0: beta3+ → beta4+
I'm would like to dupe this change forward to bug 583386 so that we can centralize discussion, work, and blocking status for beta 4.

I'm keeping this bug separate for now since it already has blocking status. When bug 583386 picks up blocking, we might just do all of the changes over there.
(In reply to comment #10)
> I'm would like to dupe this change forward to bug 583386 so that we can
> centralize discussion, work, and blocking status for beta 4.
> 
> I'm keeping this bug separate for now since it already has blocking status.
> When bug 583386 picks up blocking, we might just do all of the changes over
> there.
It has blocking, so duping forward.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Bug 583386 was fixed but there is still no character encoding menu in Firefox button.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100821 Minefield/4.0b5pre
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to comment #12)
> Bug 583386 was fixed but there is still no character encoding menu in Firefox
> button.
> 
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100821
> Minefield/4.0b5pre

That was because the coder couldn't figure out how the code worked for this.  I tried to help him and also could not figure it out.
If a page doesn't tell its encoding by HTTP header or <meta>, we need to guess the right encoding by the binary pattern. Of course, that must be sometimes wrong. Then, users need to change the encoding manually.

And also, if a page tell wrong encoding, then, users need to change the encoding manually too.

Such pages are not major in current Web. However, it's not zero. So, if mismatch of encoding happens (that is called Mojibake in Japan, the term is very popular including light users) and there is no UI for changing the encoding, that makes serious a11y problem.
(In reply to comment #14)
> Such pages are not major in current Web. However, it's not zero. So, if
> mismatch of encoding happens (that is called Mojibake in Japan, the term is
> very popular including light users) 

Yes, there are specific locales where this problem is more pronounced than others. Those locales could have the character menu as a top-level option.
>and there is no UI for changing the
>encoding, that makes serious a11y problem.

there will be a UI, this bug is blocking+
(In reply to comment #15)
> (In reply to comment #14)
> > Such pages are not major in current Web. However, it's not zero. So, if
> > mismatch of encoding happens (that is called Mojibake in Japan, the term is
> > very popular including light users) 
> 
> Yes, there are specific locales where this problem is more pronounced than
> others. Those locales could have the character menu as a top-level option.

So, do you mean that the character encoding menu should be displayed only on some locales? If so, we should have a new hidden pref for displaying them forcibly. Nightly builds, some Alphas, and early Betas are not localized. So, testers and developers in such locales need to display the menus on En-US build too.
Every local will have access to character encoding through the developer menu.  We would like to introduce a pref for locales to opt in to additionally displaying it directly in the main menu itself as well.
This bug is currently assigned to Limi, but I'm pretty sure he's not going to be introducing this preference. Who's doing that work?
>Who's doing that work?

Johnath: this bug could use an owner
Assignee: limi → nobody
Keywords: jp-critical
Given that this is an open beta 4 blocker, and it obviously missed beta 4, shouldn't it now be moved to being a beta 6 blocker? (It's not showing up in the blocking queries at the moment).
Yes. I'm also just going to assign it so someone, eenie meenie miney Margaret!
Assignee: nobody → margaret.leibovic
blocking2.0: beta4+ → beta6+
Okay. Do I just need to add the character encoding menu to the developer menu? Or should I also introduce the pref to display it directly in the main menu, and if so, where in the main menu should it go?
Attached patch patch (obsolete) — Splinter Review
I added the "character encoding" menu at the bottom of the developer menu, below "work offline." Is that where we want it?

Also, I implemented the change by copying the xul from browser/base/content/browser-menubar.inc, but I noticed that there's an overlay in toolkit (toolkit/content/charsetOverlay.xul), so should I try using that instead?
Attachment #471192 - Flags: ui-review?(faaborg)
(In reply to comment #24)
> Created attachment 471192 [details] [diff] [review]
> patch
> 
> I added the "character encoding" menu at the bottom of the developer menu,
> below "work offline." Is that where we want it?

Here is a link to iteration 6 of the Firefox button where Alex has it like:

-------------
View Page Source
-------------
Character Encoding
-------------
Work Offline
Attached patch patch v2 (obsolete) — Splinter Review
I corrected the ordering of the menu, and I created a browser-charsetmenu.inc file to factor out the character encoding menu code.
Attachment #471192 - Attachment is obsolete: true
Attachment #471275 - Flags: review?(gavin.sharp)
Attachment #471192 - Flags: ui-review?(faaborg)
Attachment #471275 - Attachment is patch: true
Attachment #471275 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 471192 [details] [diff] [review]
patch

Let's make work offline last, so place this above work offline and below veiw source.  With that change ui-r+
Attachment #471192 - Flags: ui-review+
Comment on attachment 471275 [details] [diff] [review]
patch v2

>diff --git a/browser/base/content/browser-charsetmenu.inc b/browser/base/content/browser-charsetmenu.inc

>+# The Initial Developer of the Original Code is
>+# Mozilla Corporation.

This should be Netscape Communications Corporation.

>\ No newline at end of file

Fix that. Also this file has been infected with tabs, fix that too :)

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

>               <menuitem id="appmenu_pageSource"
>                         label="&viewPageSourceCmd.label;"
>                         command="View:PageSource"/>
>+			  <menuseparator/>

This also got tabbed.

r=me with that fixed.
Attachment #471275 - Flags: review?(gavin.sharp) → review+
Attached patch final patch (obsolete) — Splinter Review
Fixed license block and tabs.
Attachment #471275 - Attachment is obsolete: true
Keywords: checkin-needed
patching file browser/base/content/browser.xul
Hunk #1 FAILED at 566
Component: General → Menus
Keywords: checkin-needed
QA Contact: general → menus
This should apply after the patch for bug 589139 lands.
Attachment #471576 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/7da00dcb512e
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
How are we going to deal with accesskeys? They should be visible in the main menu, but we don't use them in Firefox menu.
alt = normal menu
alt-f = focused firefox menu (planned), followed by arrow key access to the sub-menu.
Maybe I wasn't clear enough - I meant we should display accesskeys in the main menu (on particular platforms), but we shouldn't display them in the Firefox menu. As the code for populating the Character encoding submenu is shared, we are currently displaying them in both main and Firefox menu.
That means as of now the Character encoding is the only submenu in the new Firefox Menu having the accesskey displayed. And that's pretty inconsistent - either every Firefox menu item should have an accesskey or there should be no accesskeys at all.
You can file a new bug to cover the inconsistency. (It would be kind of annoying to fix, and I don't think it's a blocking issue.)
Depends on: 598006
Depends on: 616836
You need to log in before you can comment on or make changes to this bug.