65973 - Customize Character Coding dialog needs UI fixup

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Henrik Gemal]

The "Customize Character Coding" needs some UI fixup:
- there are missing ":" after the words "Charactersets"
- doubleclick on charset should add/remove charset
- context button (up button only action when it's possible to move somthing up)
- use of word "Charactersets" is not very good. We refer to Character Coding all
over but in this dialog it's suddenly called Charactersets
- How can you add (User Defined) to your active charactersets

Basicly I dont really understand the dialog. How can you have multiple active
charsets. Some helptext would be nice.

Comment 1

[nhottanscp]

Add momoi to cc, is there a spec for this?

Comment 2

[Katsuhiko Momoi]

We don't have a written spec for this, however,
the Sidebar Customize dialog window is the model
we used for creating this dialog. 
Let me clarify some of the specs here as a guideline
to deal with this bug:

1. > there are missing ":" after the words "Charactersets"

     True. Should fix this.

2. > doubleclick on charset should add/remove charset

      In the sidebar dialog, it does add but it does not
      remove. Can we unify this part of the action
      with Sidebar and Accept Language dialog windows?
      I don't mid adding the "remove" function as suggested
      by gemal. But I think UI behavior should be 
      consistent across similar dialog windows.

3. > context button (up button only action when it's possible to move    
somthing up)

      I agree that this is lacking in the charset customize
      dialog but available in the Sidebar dialog. We should
      fix this problem.

4. > - use of word "Charactersets" is not very good. We refer to     
        Character Coding all over but in this dialog it's 
        suddenly called Charactersets.

    Again, agreed. This is bad. We should use the uniform wording
    to match the menu name, Character Coding.

5. > - How can you add (User Defined) to your active 
       charactersets?

     Is this a question or a rhetorical question?
     User-defined is used by people who need to use
     their own private fonts which must not use
     any specific encoding converters. We internally
     map these to PUA of Unicode and then map them
     back as they were. So this is simply a pass-through
     encoding and some users will find the menu useful. Thus it
     should be available as an active charset if desired
     by the user.


(Usage instructions on Character Coding Customize
  menu)

For Mail, there are 2 types of Customize menus:

a. For Messenger
b. For Compose window

a. For Messenger, use this menu to create a permanent
   set of Character coding items you want available on the
   1st tier of the Character Coding menu.
   These items will be always available whether or not
    they have been cached throug usage. (UP to 5 most 
    recently used character coding items are added to the 
    active list at the bottom of the 1st tier. The 6th one will
    be added to the end but the oldest one will be
    bumped off the list. The cache item will not be added
    if the same character coding item already exists 
    on the list.) So if the cache file is lost somehow, 
    the items on the active list will be still visible
    and available.) So if you are users of 2 or 3 different
    languages and specifying all the character coding
    items you need at all times would be useful.
  
    Additionally, I think we should clear the cache
    after a session is over so that the next time,
    we see the permanent (active) list items only.
    And then we begin building the cache list based
    on the usage patterns.

b. For the Mail Compose window:

   This list no cache but lists all the 'standard'
   mail charsets. Most users don't send messages 
   in 15 different languages. Thefore, this customize
   dialog offer an opportunity to customize the list.
   For example, for most Japanese users, Western (8859-1),
   Japanese (ISO-2022-JP) and Unicode (UTF-8) should be
   sufficient. This customize dialog is pretty 
   straightforward. 

Comment 3

[Katsuhiko Momoi]

BTW, to avoid misunderstanding, we do mention
the Customize dialog in both Browser and Mail
International UI specs for Character Coding menu at:

http://mozilla.org/projects/intl/

but specifics of the dialog itself are not there.

Comment 4

[nhottanscp]

Shanjian, would you take this? I think you have done similar fix for language
dialog.

Comment 5

[Katsuhiko Momoi]

I wonder who wrote the Customize dialog. CC'ing ben.

Comment 6

[Katsuhiko Momoi]

> I wonder who wrote the Customize dialog. CC'ing ben.

should read:

I wonder who wrote the Customize Sidebar dialog. CC'ing ben.
This is because part of what is requested does not
work in the Sidebar dialog, either.

Comment 7

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 8

[Mark Olson]

Attachment: patch - ui fixups

Comment 9

[Mark Olson]

My patch should address all issues noted, with the exception of the fact that I
changed the headers to Character Codings (note the plural), since it seemed (to
me) more reasonable.  I am willing to change that if there is public outcry.

Taking the bug while I'm here.

Comment 10

[Mark Olson]

I forgot to note, in addition to fixing this I removed an ineffectual QI (the
method already returned an nsISupportsArray), as well as an associated property
(method?) that was causing strict JS errors (and didn't exist/do anything
anyway).  There are also a few tabs removed and some ~80 column fixups to the xul.

Comment 11

[Mark Olson]

Attachment: same patch, but .setAttribute("disabled", ) -> .disabled =

Comment 12

[Peter "jag" Annema]

+  var activeCharsetSelected = (active_charsets.selectedItems.length > 0);
+  remove_button.disabled = !activeCharsetSelected;
+  if (activeCharsetSelected) {
+    up_button.disabled = !(active_charsets.selectedItems[0].previousSibling);
+    down_button.disabled = !(active_charsets.selectedItems[0].nextSibling);
+  }
+  else {
+    up_button.disabled = true;
+    down_button.disabled = true;
+  }
+  
+  add_button.setAttribute('disabled',
+                          (available_charsets.selectedItems.length == 0));

How about:

+  var activeCharsetSelected = (active_charsets.selectedItems.length > 0);
+  remove_button.disabled = !activeCharsetSelected;
+  add_button.disabled = !activeCharsetSelected;
+
+  var selectedItem = active_charsets.selectedItems[0];
+  up_button.disabled = !(activeCharsetSelected &&
selectedItem.previousSibling);
+  down_button.disabled = !(activeCharsetSelected && selectedItem.nextSibling);

?

r=jag if you at least change the line for add_button, the other change is just a
style thing so up to you :-)

Comment 13

[Mark Olson]

Jag, the add_button is enabled based on a different list.  As is, the change is
wrong.  However, it may or may not be worthwhile to get an
availableCharsetSelected variable to simplify that.  Your call.  Getting
selectedItem based on active[0] may or may not warn when no items are selected,
I'd have to check.

Comment 14

[Peter "jag" Annema]

Attachment: [patch] merely changing that last setAttribute to .disabled

Comment 15

[Peter "jag" Annema]

mao: now I know why I should sleep more often.

So I've simply changed that last setAttribute('disabled',...) to .disabled =,
didn't want to bother mao after yesterday's "let's drive mao nuts" session.

r=jag

Comment 16

[Mark Olson]

cc'ing Erik for sr, since this is i18n

Comment 17

[Erik van der Poel]

sr=erik

Comment 18

[Blake Ross]

Fix checked in.

Comment 19

[Yuying Long]

Fixed verified on 03-05 mtrunk build.