Closed Bug 943732 Opened 6 years ago Closed 5 years ago

Port the new Character Encoding menu to SeaMonkey

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(seamonkey2.25? fixed, seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed)

RESOLVED FIXED
seamonkey2.28
Tracking Status
seamonkey2.25 ? fixed
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed
seamonkey2.28 --- fixed

People

(Reporter: emk, Assigned: neil)

References

(Depends on 1 open bug)

Details

(Whiteboard: [Australis:M-])

Attachments

(5 files, 3 obsolete files)

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
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!)
Attached patch WIP (obsolete) — Splinter Review
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)
Is the "Customize Character Encoding" dialog going away as well? (View -> Character Encoding -> Customize list...)
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...)

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 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+
(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?
Attachment #8362167 - Flags: feedback?(iann_bugzilla) → feedback+
(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.
Attachment #8362167 - Attachment is obsolete: true
Attachment #8368973 - Flags: review?(iann_bugzilla)
Attachment #8368973 - Flags: feedback?(philip.chee)
(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
See Also: → 936442
Version: unspecified → Trunk
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
(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
(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.
Attached patch Proposed patch (obsolete) — Splinter Review
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+
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 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
Attachment #8369749 - Flags: feedback?(philip.chee) → feedback+
(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?
Attachment #8369749 - Flags: review?(iann_bugzilla) → review+
(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.
Attached patch Trunk patch (obsolete) — Splinter Review
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)
Whiteboard: [Australis:M-]
Attachment #8381483 - Flags: review?(philip.chee) → review?(iann_bugzilla)
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 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)
(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)
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?
Attached patch Trunk patchSplinter Review
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)
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+
Comment on attachment 8368973 [details] [diff] [review]
2.25 branch patch

Cancelling obsolete feedback request.
Attachment #8368973 - Flags: feedback?(philip.chee)
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+
Pushed comm-central changeset 9a838a1dd341.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
Attachment #8369749 - Attachment description: Proposed patch → 2.26 branch patch
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+
Missing charsetOvery.xul in aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8413567 - Flags: review?(philip.chee)
Depends on: 1002468
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+
Blocks: 990749
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.