It is still used by SeaMonkey. Currently SeaMonkey lists all blacklisted encodings in the encoding menu!

I think the fix that the summary calls for is not the right one. Removing entries that Gecko no longer accepts frim the old SeaMonkey menu structure would leave the menu very unbalanced, reintroducin .notForBrowser wouldn't appropriately prune the submenus and we should not keep cruft in m-c for SeaMonkey. Instead, SeaMonkey should fix its menu in c-c.

OK, then charsetMenu.properties and charsetMenu.dtd should be moved to toolkit instead of maintaining a copy of the files for each app.

FWIW, bug 942802 makes this non-dangerous--just bad UI.

I take it it's just the browser's menu that's affected here?

(In reply to neil@parkwaycc.co.uk from comment #4) > I take it it's just the browser's menu that's affected here? Yes, or at least that's supposed to be the case.

Note that the patch I just attached to bug 940907 also fixes the charset menu for SeaMonkey. (Isn't code reuse wonderful!)

I've written this as an overlay so we should be able to replace the charset menus in editor, mailnews and compose fairly easily too. (I don't know what direction Thunderbird wants to go with their charset menus yet.)

Is the "Customize Character Encoding" dialog going away as well? (View -> Character Encoding -> Customize list...)

(In reply to Philip Chee from comment #8) > Is the "Customize Character Encoding" dialog going away as well? (View -> > Character Encoding -> Customize list...) Bug 943252 suggested moving it to comm-central but I'm not sure whether that will actually happen.

(In reply to Philip Chee from comment #8) > Is the "Customize Character Encoding" dialog going away as well? (View -> > Character Encoding -> Customize list...) I don't make decisions for SeaMonkey, of course, but I suggest getting rid of the customizability. Using Firefox's menu design makes the menu short enough that the whole list fits on the screen and it seems like a bad idea to put a lot of effort into a UI feature that is almost unused.

Comment on attachment 8362167 [details] [diff] [review] WIP > + getBrowser().docShell.charset = aEvent.target.id.slice('charset.'.length); Everywhere else in this file double quotes are used, so "charset." > + function UpdateCharsetDetector(aNode) > + { > + var detector = GetLocalizedStringPref("intl.charset.detector"); > + var menuitem = aNode.getElementsByAttribute("detector", detector).item(0); Consider using getElementsByAttribute("detector", detector)[0] > + <menu id="charsetMenuAutodet" > + label="&charsetMenuAutodet.label;" > + accesskey="&charsetMenuAutodet.accesskey;" > + onpopupshowing="UpdateCharsetDetector(this);" > + oncommand="SelectCharsetDetector(event);"> > + <menupopup> Is there any reason onpopupshowing/oncommand aren't in the <menupopup>? > + <menuitem type="radio" > + name="detectorGroup" > + detector="ukprob" > + label="&charsetMenuAutodet.uk.label;" > + accesskey="&charsetMenuAutodet.uk.accesskey;"/> [comment: ISO country code for Ukraine is UA not UK]

(In reply to Philip Chee from comment #11) > > + getBrowser().docShell.charset = aEvent.target.id.slice('charset.'.length); > Everywhere else in this file double quotes are used, so "charset." D'oh, missed one! > > + function UpdateCharsetDetector(aNode) > > + { > > + var detector = GetLocalizedStringPref("intl.charset.detector"); > > + var menuitem = aNode.getElementsByAttribute("detector", detector).item(0); > Consider using getElementsByAttribute("detector", detector)[0] I like to use .item when using unchecked nodelist indices. > > + <menu id="charsetMenuAutodet" > > + label="&charsetMenuAutodet.label;" > > + accesskey="&charsetMenuAutodet.accesskey;" > > + onpopupshowing="UpdateCharsetDetector(this);" > > + oncommand="SelectCharsetDetector(event);"> > > + <menupopup> > Is there any reason onpopupshowing/oncommand aren't in the <menupopup>? Consistency with the parent menu. > > + <menuitem type="radio" > > + name="detectorGroup" > > + detector="ukprob" > > + label="&charsetMenuAutodet.uk.label;" > > + accesskey="&charsetMenuAutodet.uk.accesskey;"/> > [comment: ISO country code for Ukraine is UA not UK] [well it was UK once, shame that we stole the .uk domain!]

uk is a language tag. Not a country code

> > Consider using getElementsByAttribute("detector", detector)[0] > I like to use .item when using unchecked nodelist indices. Fair enough (In reply to Henri Sivonen (:hsivonen) from comment #13) > uk is a language tag. Not a country code Don't they speak Russian in the Ukraine?

(In reply to Philip Chee from comment #14) > Don't they speak Russian in the Ukraine? Russian is a minority language there and Ukrainian is the dominant and official language.

Comment on attachment 8362167 [details] [diff] [review] WIP Do we need to fork charsetMenu.dtd too? Presumably a later patch will change editor, mailnews and compose to use this overlay too?

(In reply to Ian Neal from comment #16) > Do we need to fork charsetMenu.dtd too? No, charsetMenu.dtd is itself already a fork of charsetOverlay.dtd > Presumably a later patch will change editor, mailnews and compose to use > this overlay too? Which is why I only asked for feedback so far.

This just uses the same menu everywhere. It might not be the best solution for mailnews or messengercompose but we don't know what Thunderbird is doing yet and they get to put it off for a few months anyway.

(In reply to neil@parkwaycc.co.uk from comment #17) > (In reply to Ian Neal from comment #16) >> Do we need to fork charsetMenu.dtd too? > No, charsetMenu.dtd is itself already a fork of charsetOverlay.dtd Famous last words: https://bugzilla.mozilla.org/show_bug.cgi?id=936442#c59

Comment on attachment 8368973 [details] [diff] [review] 2.25 branch patch That bug completely rewrote the way the menu was created, so it's no wonder it's regressed this patch. This patch should still work on the 2.25 branch though.

(In reply to Philip Chee from comment #19) > (In reply to neil@parkwaycc.co.uk from comment #17) > > (In reply to Ian Neal from comment #16) > >> Do we need to fork charsetMenu.dtd too? > > No, charsetMenu.dtd is itself already a fork of charsetOverlay.dtd > > Famous last words: > https://bugzilla.mozilla.org/show_bug.cgi?id=936442#c59 Gijs unbroke us I think: Bug 966759 - Resurrect deleted strings on mc in case we need holly on Aurora

(In reply to Philip Chee from comment #21) > (In reply to Philip Chee from comment #19) > > (In reply to neil@parkwaycc.co.uk from comment #17) > > > (In reply to Ian Neal from comment #16) > > >> Do we need to fork charsetMenu.dtd too? > > > No, charsetMenu.dtd is itself already a fork of charsetOverlay.dtd > > > > Famous last words: > > https://bugzilla.mozilla.org/show_bug.cgi?id=936442#c59 > > Gijs unbroke us I think: > Bug 966759 - Resurrect deleted strings on mc in case we need holly on Aurora It won't last long. Those strings will all die as soon as possible, as they're dead on m-c.

Updated to work around changes from bug 936442.

With the extraneous messengercompose.xul changes removed, sorry about that.

Comment on attachment 8369749 [details] [diff] [review] 2.26 branch patch >+++ b/suite/mailnews/messageWindow.xul >- <script type="application/javascript" src="chrome://messenger/content/shareglue.js"/> >+++ b/suite/mailnews/messenger.xul >-<script type="application/javascript" src="chrome://messenger/content/shareglue.js"/> Shouldn't the shareglue.js be moved into the TB ifdef in comm-central/mailnews/jar.mn ?

If TB wants to keep shareglue.js they should probably move it to mail/ anyway.

Comment on attachment 8369749 [details] [diff] [review] 2.26 branch patch f=me

(In reply to Henri Sivonen (:hsivonen) from comment #10) > (In reply to Philip Chee from comment #8) > > Is the "Customize Character Encoding" dialog going away as well? (View -> > > Character Encoding -> Customize list...) > > I don't make decisions for SeaMonkey, of course, but I suggest getting rid > of the customizability. Using Firefox's menu design makes the menu short > enough that the whole list fits on the screen and it seems like a bad idea > to put a lot of effort into a UI feature that is almost unused. If "the menu is short enough that the whole list fits on the screen", doesn't it mean that there are necessarily some encodings missing? And how do you know that the feature is "almost unused"? Unused by whom? IMHO it is useful, especially to "technical-minded" users like SeaMonkey's target audience, in that it allows both adding desired charsets and removing unused ones _according to the user's wishes_. This would allow keeping the list shorter than, say, half a screen height, while _also_ including all charsets that the user may want to use in function, not so much of what he sends (UTF-8 might well be enough there), but of what he receives, and I know that I get English-language messages in any of iso-8859-1, iso-8859-15, Windows-1252, utf-8, koi8-r, Big5, Shift-JIS, GB2312, and these are only the most frequent.

Comment on attachment 8369749 [details] [diff] [review] 2.26 branch patch I presume for composer/text editor you did not want to do an overlay on an overlay hence not putting the common menu id="charsetMenu" code into editingOverlay.xul?

(In reply to Ian Neal from comment #29) > (From update of attachment 8369749 [details] [diff] [review]) > I presume for composer/text editor you did not want to do an overlay on an > overlay hence not putting the common menu id="charsetMenu" code into > editingOverlay.xul? Well, I guess it might work, but I had to change the other two files anyway.

Trunk has changed slightly allowing some code removal. Previous attachment is still valid for comm-aurora (when I get around to requesting approval).

Comment on attachment 8369749 [details] [diff] [review] 2.26 branch patch [Triage Comment] a=me for c-a

Comment on attachment 8381483 [details] [diff] [review] Trunk patch > -function SetDocumentCharacterSet(aCharset) > +function ComposeSetCharacterSet(aEvent) > { > if (gMsgCompose) { > - gMsgCompose.SetDocumentCharset(aCharset); > + gMsgCompose.SetDocumentCharset(aEvent.target.id.slice("charset.".length)); Shouldn't this be: if (aEvent.target.hasAttribute("charset")) gMsgCompose.SetDocumentCharset(aEvent.target.getAttribute("charset"));

(In reply to Philip Chee from comment #33) > > + gMsgCompose.SetDocumentCharset(aEvent.target.id.slice("charset.".length)); > Shouldn't this be: > if (aEvent.target.hasAttribute("charset")) > gMsgCompose.SetDocumentCharset(aEvent.target.getAttribute("charset")); You're right about the id slice, but I don't need to check the attribute because the message compose window doesn't have a detector submenu.

Comment on attachment 8368973 [details] [diff] [review] 2.25 branch patch As I seem to already have aurora approval before having fixed the bug on trunk (due to the target moving) I guess I might as well ask for beta approval too...

Bah, I must have forgotten to run hg diff again, my tree had the correct code :-(

Comment on attachment 8368973 [details] [diff] [review] 2.25 branch patch a=me for comm-beta

Comment on attachment 8368973 [details] [diff] [review] 2.25 branch patch Cancelling obsolete feedback request.

Comment on attachment 8389367 [details] [diff] [review] Trunk patch Testing Message Compose: Tue Mar 25 2014 00:00:27 Error: ReferenceError: SetDocumentCharacterSet is not defined Source file: chrome://messenger/content/messengercompose/MsgComposeCommands.js Line: 1473 Which is somewhere here: > if (gMsgCompose && originalCharset != gMsgCompose.compFields.characterSet) > SetDocumentCharacterSet(gMsgCompose.compFields.characterSet); I think this should be ComposeSetCharacterSet() Aside from that everything else seems to work. r=me with the above fixed.

Pushed comm-central changeset 9a838a1dd341.

Comment on attachment 8389367 [details] [diff] [review] Trunk patch [Triage Comment] a=me for comm-aurora-sm-2.27

Pushed comm-aurora changeset fd88841bab2f.

Missing charsetOvery.xul in aurora.

Pushed to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/ffacfc0539ea

Comment on attachment 8413567 [details] [diff] [review] bug_943732_bf.diff a=Ratty for comm-aurora bustage fix