Closed
Bug 943732
Opened 11 years ago
Closed 10 years ago
Port the new Character Encoding menu to SeaMonkey
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(seamonkey2.25? fixed, seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed)
RESOLVED
FIXED
seamonkey2.28
People
(Reporter: emk, Assigned: neil)
References
(Depends on 1 open bug)
Details
(Whiteboard: [Australis:M-])
Attachments
(5 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
iannbugzilla
:
review+
philip.chee
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
iannbugzilla
:
review+
philip.chee
:
feedback+
iannbugzilla
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
philip.chee
:
review+
philip.chee
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
philip.chee
:
review+
philip.chee
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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.
Component: Internationalization → UI Design
Product: Core → SeaMonkey
Summary: Reintroduce .notForBrowser to charsetData.properties → Port the new Character Encoding menu to SeaMonkey
Reporter | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
Note that the patch I just attached to bug 940907 also fixes the charset menu for SeaMonkey. (Isn't code reuse wonderful!)
Assignee | ||
Comment 7•11 years ago
|
||
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.)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8362167 -
Flags: feedback?(philip.chee)
Attachment #8362167 -
Flags: feedback?(iann_bugzilla)
Comment 8•11 years ago
|
||
Is the "Customize Character Encoding" dialog going away as well? (View -> Character Encoding -> Customize list...)
Flags: needinfo?(neil)
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Flags: needinfo?(neil)
(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 11•11 years ago
|
||
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]
Attachment #8362167 -
Flags: feedback?(philip.chee) → feedback+
Assignee | ||
Comment 12•11 years ago
|
||
(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
Comment 14•11 years ago
|
||
> > 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.
Assignee | ||
Updated•11 years ago
|
status-seamonkey2.25:
--- → affected
status-seamonkey2.26:
--- → affected
Comment 16•11 years ago
|
||
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?
Attachment #8362167 -
Flags: feedback?(iann_bugzilla) → feedback+
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
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.
Attachment #8362167 -
Attachment is obsolete: true
Attachment #8368973 -
Flags: review?(iann_bugzilla)
Attachment #8368973 -
Flags: feedback?(philip.chee)
Comment 19•11 years ago
|
||
(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
Version: unspecified → Trunk
Assignee | ||
Comment 20•11 years ago
|
||
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.
Attachment #8368973 -
Attachment description: Proposed patch → 2.25 branch patch
Comment 21•11 years ago
|
||
(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
Depends on: 966759
Comment 22•11 years ago
|
||
(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.
Assignee | ||
Comment 23•11 years ago
|
||
Updated to work around changes from bug 936442.
Attachment #8369483 -
Flags: review?(iann_bugzilla)
Attachment #8369483 -
Flags: feedback?(philip.chee)
Attachment #8368973 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 24•11 years ago
|
||
With the extraneous messengercompose.xul changes removed, sorry about that.
Attachment #8369483 -
Attachment is obsolete: true
Attachment #8369483 -
Flags: review?(iann_bugzilla)
Attachment #8369483 -
Flags: feedback?(philip.chee)
Attachment #8369749 -
Flags: review?(iann_bugzilla)
Attachment #8369749 -
Flags: feedback?(philip.chee)
Comment 25•11 years ago
|
||
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 ?
Assignee | ||
Comment 26•11 years ago
|
||
If TB wants to keep shareglue.js they should probably move it to mail/ anyway.
Comment 27•11 years ago
|
||
Comment on attachment 8369749 [details] [diff] [review]
2.26 branch patch
f=me
Attachment #8369749 -
Flags: feedback?(philip.chee) → feedback+
Comment 28•11 years ago
|
||
(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 29•11 years ago
|
||
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?
Attachment #8369749 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 30•11 years ago
|
||
(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.
Assignee | ||
Comment 31•11 years ago
|
||
Trunk has changed slightly allowing some code removal. Previous attachment is still valid for comm-aurora (when I get around to requesting approval).
Attachment #8381483 -
Flags: review?(philip.chee)
Updated•11 years ago
|
Whiteboard: [Australis:M-]
Assignee | ||
Updated•11 years ago
|
Attachment #8381483 -
Flags: review?(philip.chee) → review?(iann_bugzilla)
Comment 32•11 years ago
|
||
Comment on attachment 8369749 [details] [diff] [review]
2.26 branch patch
[Triage Comment]
a=me for c-a
Attachment #8369749 -
Flags: approval-comm-aurora+
Comment 33•11 years ago
|
||
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"));
Attachment #8381483 -
Flags: review?(iann_bugzilla)
Flags: needinfo?(neil)
Assignee | ||
Comment 34•11 years ago
|
||
(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.
Flags: needinfo?(neil)
Assignee | ||
Comment 35•11 years ago
|
||
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...
Attachment #8368973 -
Flags: approval-comm-beta?
Assignee | ||
Updated•11 years ago
|
tracking-seamonkey2.25:
--- → ?
Assignee | ||
Comment 36•11 years ago
|
||
Bah, I must have forgotten to run hg diff again, my tree had the correct code :-(
Attachment #8381483 -
Attachment is obsolete: true
Attachment #8389367 -
Flags: review?(philip.chee)
Assignee | ||
Comment 37•11 years ago
|
||
Comment 38•11 years ago
|
||
Comment on attachment 8368973 [details] [diff] [review]
2.25 branch patch
a=me for comm-beta
Attachment #8368973 -
Flags: approval-comm-beta? → approval-comm-beta+
Assignee | ||
Comment 39•11 years ago
|
||
Comment 40•11 years ago
|
||
Comment on attachment 8368973 [details] [diff] [review]
2.25 branch patch
Cancelling obsolete feedback request.
Attachment #8368973 -
Flags: feedback?(philip.chee)
Comment 41•11 years ago
|
||
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.
Attachment #8389367 -
Flags: review?(philip.chee) → review+
Assignee | ||
Comment 42•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-seamonkey2.27:
--- → affected
status-seamonkey2.28:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
Assignee | ||
Updated•11 years ago
|
Attachment #8369749 -
Attachment description: Proposed patch → 2.26 branch patch
Comment 43•11 years ago
|
||
Comment on attachment 8389367 [details] [diff] [review]
Trunk patch
[Triage Comment]
a=me for comm-aurora-sm-2.27
Attachment #8389367 -
Flags: approval-comm-aurora+
Assignee | ||
Comment 44•11 years ago
|
||
Comment 45•11 years ago
|
||
Missing charsetOvery.xul in aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 46•11 years ago
|
||
Attachment #8413567 -
Flags: review?(philip.chee)
Comment 47•11 years ago
|
||
Pushed to comm-aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/ffacfc0539ea
Comment 48•11 years ago
|
||
Comment on attachment 8413567 [details] [diff] [review]
bug_943732_bf.diff
a=Ratty for comm-aurora bustage fix
Attachment #8413567 -
Flags: review?(philip.chee)
Attachment #8413567 -
Flags: review+
Attachment #8413567 -
Flags: approval-comm-aurora+
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•