444411 - Migrate SeaMonkey's "Browser > Languages" preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Serge Gautherie (:sgautherie)]

<http://mxr.mozilla.org/seamonkey/find?string=%2Fpref-languages>
{{
/suite/common/pref/pref-languages-add.xul
/suite/common/pref/pref-languages.xul
/suite/common/pref/pref-languages.js
/suite/locales/en-US/chrome/common/pref/pref-languages.properties
/suite/locales/en-US/chrome/common/pref/pref-languages.dtd
}}

***

Should the "pref-languages v4" patch from bug 56680 be applied first/now ?
("I" could do it.)
Or will it be included in the (later) migration ?

Comment 1

[[e-mail]]

That looks like a substantial patch, it's probably worth keeping them separate.

Comment 2

[Stefan [:stefanh]]

(In reply to comment #0)
> <http://mxr.mozilla.org/seamonkey/find?string=%2Fpref-languages>
> {{
> /suite/common/pref/pref-languages-add.xul
> /suite/common/pref/pref-languages.xul
> /suite/common/pref/pref-languages.js
> /suite/locales/en-US/chrome/common/pref/pref-languages.properties
> /suite/locales/en-US/chrome/common/pref/pref-languages.dtd
> }}
> 
> ***
> 
> Should the "pref-languages v4" patch from bug 56680 be applied first/now ?

I just landed that patch.

Comment 3

[Stefan [:stefanh]]

I've started to look at this a bit.

Comment 4

[Stefan [:stefanh]]

Might be worth fixing bug 99379 at the same time. I also discovered that if you add languges and one of them already exists in the active languages listbox you'll end up with a "undefined" item among the newly added listitems (and in the pref string). This actually happens on my 1.1.10 build as well.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> I also discovered that if you
> add languges and one of them already exists in the active languages listbox
> you'll end up with a "undefined" item among the newly added listitems (and in
> the pref string). This actually happens on my 1.1.10 build as well.

Bug 446123 will fix this.

Comment 6

[Stefan [:stefanh]]

> Bug 446123 will fix this.
> 

Ah, right - the .length... Anyway, the patch that eventually will get posted here will probably be a major re-write of all the js. So, the best thing would probably be to do those changes in here.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> > Bug 446123 will fix this.
> 
> the best thing would probably be to do those changes in here.

Either way would be fine with me:
I filed the other bug so that these fixes don't get lost (only).

Comment 8

[Stefan [:stefanh]]

Attachment: Languages pane with 2 listboxes, standard font

Neil suggested that we could use 2 listboxes in the pane, one for the active languages and one for the languages you can add. Attaching a mockup of how it would look (don't pay attention to the text, just look at the layout). I'm not sure I like it - it gets a bit crammed and if the language names are long, they'll get cropped. In the screenshot I've scrolled to en-au, just to show the problem with cropped names. This is with the default font, will attach a screenshot where i hacked the font.

Comment 9

[Stefan [:stefanh]]

Attachment: Languages pane with 2 listboxes, smaller font in listboxes

In this variant, the cropping is no big deal - it still occurs, though. If we don't care about the cropping - isn't this a bit overwhelming for a user when he/she opens the panel?

Comment 10

[Karsten Düsterloh]

(In reply to comment #9)
> In this variant, the cropping is no big deal - it still occurs, though.

We could have tooltips with the full name (might require a tree, though, dunno by heart), so that's actually not that much of an issue.

> isn't this a bit overwhelming for a user when he/she opens the panel?

This, otoh, definitely is. 
We're throwing a lot of data into the user's face which he usually doesn't care about (=list of possible languages), while obscuring the actual settings by minimizing the available display space.

I don't think we should do this. The secondary dialog is sufficient.

The more interesting question is: should the secondary dialog do instant apply?
Would be cool and useful, but does the effort pay off?

Comment 11

[[e-mail]]

I decided that I should actually look at our existing dialog ;-)

One possible UI would be a menubutton (not a menulist) labelled "Add" and selecting an item from its menu would then add it to the list. However this doesn't allow for user-defined values. A separate textbox would be necessary.

Another possibility is an editable menulist however its labels would need to be the actual values to be added to the list so we would lose the descriptions.

Comment 12

[Stefan [:stefanh]]

> The more interesting question is: should the secondary dialog do instant apply?
> Would be cool and useful, but does the effort pay off?

In the version, where I keep the dialog, I was (am) going to write to prefs when the dialog is closed, then onsyncfrompreference takes care of the menulist in the pane (rebuilds it). Another option is to add listitems to the listbox in the pane when the dialog is closed and then rely on onsynctopreference. So, the idea was to not use instantapply in the second dialog - that is, keep the dialog as it is now (with OK/Cancel buttons).

Comment 13

[Stefan [:stefanh]]

(In reply to comment #11)
> I decided that I should actually look at our existing dialog ;-)
> 
> One possible UI would be a menubutton (not a menulist) labelled "Add" and
> selecting an item from its menu would then add it to the list. However this
> doesn't allow for user-defined values. A separate textbox would be necessary.

Then we'll loose the ability for adding multiple languages with one click. Not sure how important that is, though.

Comment 14

[Stefan [:stefanh]]

Attachment: WIP1

Just attaching a patch - it's far from being ready for review, though. Basically, "it works", but it needs lots of clean-up some re-structuring. In this patch I use 2 .js files - one for the prefpane and one for the dialog. It turns  out that this might not be such a good idea, because there are 2 functions that I need in both xul files.

Comment 15

[Stefan [:stefanh]]

Note to self: I need to build the languagedict from all the languages in the .properties file, not just the "true" ones.

Comment 16

[Stefan [:stefanh]]

Attachment: WIP2

Just putting another wip patch here. Not much have changed, just doing comment #15. I'll try and make the dialog a prefwindow with a "intl.accept_languages" preference element.

Comment 17

[Stefan [:stefanh]]

Attachment: WIP2

Oops, forgot to include the .js file...

Comment 18

[Stefan [:stefanh]]

Attachment: WIP3

Now with dialog as prefwindow - still some work left, though

Comment 19

[Stefan [:stefanh]]

Attachment: WIP4

Quite close... Some odd things happens when I add a not-allowed language in the dialog, though. Files not cleaned up yet. I'm leaning on having 2 .js files since there is only one function that both files need (AddListitem). I'm using onbeforeaccept, not so sure how wise that is.

Comment 20

[Stefan [:stefanh]]

Attachment: WIP5, quite close (Checkin: Comment 54)

OK, pretty close. However:

1) When I restore focus on the add languages dialog (on mac it's a sheet), I get a 2 (identical) warnings in the js console: "Warning: Empty string passed to getElementById().". I haven't been able to find out what is happening here - the same thing happens if you have entered a invalid language in textfield in the dialog. I tried IanN's proxy patch - no errors there, so it's probably me.

2) In the add languages dialog I use onbeforeaccept to trigger doCommand() and ondialogaccept for the check if a language is invalid or not. I'm not really sure that this is the right thing to do.

3) There are a few functions that probably needs some re-factoring and clean-up.

4) The usage of "let" could be a bit more consistent

Comments are welcome. However, I'll probably not be able to spend more time on this before I leave on vacation.

Comment 21

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

>+function SelectLanguage()
>+{
>+  if (activeLanguages.selectedItems.length)
>+  {
>     document.getElementById("remove").disabled = false;
>+    var selected = activeLanguages.selectedItems[0];
>     document.getElementById("down").disabled = !selected.nextSibling;
> 
>     document.getElementById("up").disabled = !selected.previousSibling;
>   }
>+  else
>+  {
>     document.getElementById("remove").disabled = true;
>     document.getElementById("down").disabled = true;
>     document.getElementById("up").disabled = true;
>   }
You have lost the pref locked part of the code and will need to put something back in.

Comment 22

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

>+++ b/suite/common/pref/pref-languages.js
>+//Dictionary of all available languages
>+var langObj = { availLanguageDict: [] };
> 
>+var activeLanguages;
Global variables should really start with g

Comment 23

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

> function ReadAvailableLanguages()
> {

>+        var str = stringNameProperty[0];
>+        var stringLangRegion = stringNameProperty[0].split('-');
Could always do =str.split('-') here

>+        var tit;
> 
>+        if (stringLangRegion[0])
As we don't do anything later on if !str why not test for it here as well i.e. if (str && stringLangRegion[0])
>+        {
>           var language;
>           var region;
If we set region to null here and test for nullness later on, we can probably drop useRegionFormat
>+          var useRegionFormat = false;

> 
>           try {
Bracing?
>             language = languagesBundle.getString(stringLangRegion[0]);
>           }
>           catch (ex) {
Bracing?
>             language = "";
>           }
> 
>+          if (stringLangRegion.length > 1)
>+          {
> 
>             try {
Bracing?
>               region = regionsBundle.getString(stringLangRegion[1]);
>+              useRegionFormat = true;
Drop this.
>             }
>             catch (ex) {
Bracing?
>             }
>           }
>           
>+          if (useRegionFormat)
if (region)
>             tit = prefLangBundle.getFormattedString("languageRegionCodeFormat",
>                                                     [language, region, str]);
>+          else
>             tit = prefLangBundle.getFormattedString("languageCodeFormat",
>                                                     [language, str]);
>+        }
> 
>+        if (str && tit)
>+        {
>+          langObj.availLanguageDict[i] = [];
>+          langObj.availLanguageDict[i].push(tit, str, curItem.value);
>+          i++;
>+        }
Move above if into previous brace and only test for tit!

Comment 24

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

>+function ReadActiveLanguages(aListbox)
>+{
>+  var prefString = document.getElementById("intl.accept_languages").value;
>+  var arrayOfPrefs = prefString.split(/\s*,\s*/);
>+  
>+  var listboxChildren = aListbox.childNodes;
> 
>+  // No need to rebuild listitems if languages in prefs and listitems match.
>+  if (InSync(arrayOfPrefs, listboxChildren))
>+   return undefined;
Would we still want to do a SelectLanguage() here?
> 
>+  while (aListbox.hasChildNodes())
>+    aListbox.removeChild(aListbox.firstChild);
> 
>+  arrayOfPrefs.forEach(function(aElement) {
Bracing?
>+    if (aElement)
>+    {
>+      let langTitle = GetLanguageTitle(aElement);
> 
>+      if (!langTitle)
>+       langTitle = '[' + aElement + ']';
> 
>+      let listitem = document.createElement('listitem');
>+      listitem.setAttribute('label', langTitle);
>+      listitem.id = aElement;
>+      aListbox.appendChild(listitem);
>+    }
>+  });
>+
>+  SelectLanguage();
>+
>+  return undefined;
> }
> 
>+// Checks whether listitems and pref values matches, returns false if not
>+function InSync(aPrefArray, aListItemArray)
>+{
>+  // Can't match if they don't have the same length
>+  if (aPrefArray.length != aListItemArray.length)
>+    return false;
> 
>+  return aPrefArray.every(IsTheSame, aListItemArray);
You should probably do this as an anonymous function e.g. http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/src/utils.js#1192
> }
> 
>+function IsTheSame(aElement, aIndex, aArray, aListItemArray)
> {
>+  return (aElement == this[aIndex].id);
> }

Comment 25

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

>+// Called on onsynctopreference
>+function WriteActiveLanguages(aListbox)
>+{
>+  var languages = 0;
>+  var prefString = "";
> 
>+  for (var item = aListbox.firstChild; item != null; item = item.nextSibling)
>+  {
>+    var languageid = item.id;
> 
>+    if (languageid.length > 1)
>+    {
>+      languages++;
>+      //separate > 1 languages by commas
>+      if (languages > 1)
>+        prefString += ", " + languageid;
>+      else
>+        prefString = languageid;
>     }
>   }
>+      
>+  return prefString;
> }
Use the Array.map functionality on the childNodes here with an anonymous function should be alot simpler.

Comment 26

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

>+function MoveUp()
> {
>+  var selectedItems = activeLanguages.selectedIndex;
>+  var selections = activeLanguages.selectedItems;
>+  if (activeLanguages.selectedItems.length == 1)
>+  {
>+    var selected = activeLanguages.selectedItems[0];
>+    var before = selected.previousSibling
>+    if (before)
>+    {
>+      before.parentNode.insertBefore(selected, before);
>+      activeLanguages.selectItem(selected);
>+      activeLanguages.ensureElementIsVisible(selected);
>     }
>   }
>   
>+  if (activeLanguages.selectedIndex == 0)
>   {
>     // selected item is first
>     var moveUp = document.getElementById("up");
>     moveUp.disabled = true;
>   }
> 
>+  if (activeLanguages.childNodes.length > 1)
>   {
>     // more than one item so we can move selected item back down
>     var moveDown = document.getElementById("down");
>     moveDown.disabled = false;
Just because we've been allowed to move up does not mean that the "Move Down" is not pref locked.
>   }
> 
>+  SelectLanguage();
>+  activeLanguages.doCommand();
>+}
> 
>+function MoveDown()
>+{
>+  if (activeLanguages.selectedItems.length == 1)
>+  {
>+    var selected = activeLanguages.selectedItems[0];
>+    if (selected.nextSibling)
>+    {
>+      if (selected.nextSibling.nextSibling)
>+      {
>+        activeLanguages.insertBefore(selected, selected.nextSibling.nextSibling);
>+      }
>+      else
>+      {
>+        activeLanguages.appendChild(selected);
>+      }
> 
>+      activeLanguages.selectItem(selected);
>     }
>   }
> 
>+  if (activeLanguages.selectedIndex == activeLanguages.childNodes.length - 1)
>   {
>     // selected item is last
>     var moveDown = document.getElementById("down");
>     moveDown.disabled = true;
>   }
> 
>+  if (activeLanguages.childNodes.length > 1)
>   {
>     // more than one item so we can move selected item back up 
>     var moveUp = document.getElementById("up");
>     moveUp.disabled = false;
Just because we've been allowed to move down does not mean that the "Move Up" is not pref locked.
>   }
> 
>+  SelectLanguage();
>+  activeLanguages.doCommand();
>+}

Comment 27

[Stefan [:stefanh]]

(In reply to comment #21)
> (From update of attachment 335939 [details] [diff] [review])
> >+function SelectLanguage()
> >+{
> >+  if (activeLanguages.selectedItems.length)
> >+  {
> >     document.getElementById("remove").disabled = false;
> >+    var selected = activeLanguages.selectedItems[0];
> >     document.getElementById("down").disabled = !selected.nextSibling;
> > 
> >     document.getElementById("up").disabled = !selected.previousSibling;
> >   }
> >+  else
> >+  {
> >     document.getElementById("remove").disabled = true;
> >     document.getElementById("down").disabled = true;
> >     document.getElementById("up").disabled = true;
> >   }
> You have lost the pref locked part of the code and will need to put something
> back in.

Yeah. I removed the old code and forgot to add new code back.

Comment 28

[Stefan [:stefanh]]

Re comment #26: I was originally thinking of including bug 168728, but now when I don't have time to look at this, I just left the old code almost as it was.

Comment 29

[Stefan [:stefanh]]

Ian, do you see 1) in comment #20 on a non-mac system?

Comment 30

[Robert Kaiser]

Who's taking this over from Stefan and driving it into the tree?

Comment 31

[Robert Kaiser]

adding blocking flag as a blocker depends on this fix.

reassigning to Ian for completion per his comment on the status meeting wiki page.

Comment 32

[Ian Neal]

Attachment: pref-languages patch v0.6

Changes since WIP5:
* Tidied up formatting in the files - bracing, return on new line, etc
* Code now respects pref locking
* General code simplification

Only issue left is the "Warning: Empty string passed to getElementById()." error, when bringing the Add Dialog box back into focus - hard to track down with JS Debugger being broken.

Asking for reviews now as the minor issue should not block any landing hopefully.

Comment 33

[[e-mail]]

One "flaw" with this system is that up/down/add/remove lose the selection.
(Well, I guess it's expected with remove, but...)

Comment 34

[[e-mail]]

Comment on attachment 337868 [details] [diff] [review]
pref-languages patch v0.6

>+function IsRFC1766LangTag(aCandidate)
I think this could be simplified through the use of regexps.
I tried to write a regexp to do the whole test:
/^([a-z]{2,3}(-(a[b-z]|q[a-l]|z[a-y]|[b-pr-wy][a-z]|[a-z][a-z][0-9a-z]{1,6}|[a-z][0-9][0-9a-z]{0,6}|[0-9][0-9a-z]{1,7}|))?|[ix]-[0-9a-z]{1-8})(-[0-9a-z]{1-8})*$/i
but you might prefer to do regexps for each section separately
e.g. /^(aa|zz|x[a-z]|q[m-z])$/i to disallow user-assigned ISO 3166 countries

Comment 35

[Karsten Düsterloh]

Comment on attachment 337868 [details] [diff] [review]
pref-languages patch v0.6

Let's start with a sidenote: for me, using Kubuntu 8.04 with standard theme Raleigh, the panel is too narrow for its content; the right edge splits the buttons about half. It seems as if the Character Encoding section (label + dropdown) causes this.

Second, you can't decently move a listbox entry up or down, because it loses focus right after the first movement.

Third, adding new languages is rather broken:
- You can enter the same language code several times, each attempt resulting in a
  new list entry.
- When selecting a language in the subdialog list and entering gibberish into the
  textbox, you get a warning and the dialog stays open. The selected entry, 
  though, still gets added to the main list, one instance for each mistyped try!

>+++ b/suite/common/pref/pref-languages-add.js
>+function OnLoadLangAdd()
>+{ 

Without any comment, this function name is just incomprehensible.
Please use meaningful names!

>+      if ((gAvailDict[i][2] == "true") &&
>+          (selectedLanguages.indexOf(gAvailDict[i][1]) == -1))
>+        gAvailableLanguages.appendItem(gAvailDict[i][0], gAvailDict[i][1]);

The name "gAvailDict" is just as bad. Characters have dropped in price recently!

>+function IsAlpha(aMixedCase)
>+{
>+  var allCaps = aMixedCase.toUpperCase();
>+  for (var i = allCaps.length - 1; i >= 0; i--)
>+  {
>+    let c = allCaps.charAt(i);
>+    if (c < 'A' || c > 'Z')
>+      return false;
>+  }
>+  return true;
>+}

Doh! You mean 
  return /^[a-z]*$/i.test(aMixedCase);
I suppose... ;-)

>+function IsAlphaNum(aMixedCase)
>+{
>+  var allCaps = aMixedCase.toUpperCase();
>+  for (var i = allCaps.length - 1; i >= 0; i--)
>+  {
>+    let c = allCaps.charAt(i);
>+    if ((c < 'A' || c > 'Z') && (c < '0' || c > '9'))
>+      return false;
>+  }
>+  return true;
>+}

return /^[a-z0-9]*$/i.test(aMixedCase);

>+function IsRFC1766LangTag(aCandidate)

This function looks overly complicated, and unnecessarily so - see Neil's comment above. At least it's got comments. ;-)

>+  var checkedTags = 0;
>+  if (tags[0].toLowerCase() != "x" && tags[0].toLowerCase() != "i")
>+  {
>+    if (tags[0].length < 2 || tags[0].length > 3 || !IsAlpha(tags[0]))
>+      return false;

I wonder if RegExp had been invented yet in ye olden days this code must have been written...
And tags[0] probably deserves its own variable.

>+  }
>+  else if (tags.length < 2)
>+    return false;
>+  else
>+  {
>+    if (tags[1].length < 1 || tags[1].length > 8 || !IsAlphaNum(tags[1]))
>+      return false;
>+    checkedTags++;
>+  }

All levels of a "branchy if" should have braces or none.

>+  for (var i = checkedTags; i < tags.length; i++)
>+  {
>+    if (tags[i].length < 1 || tags[i].length > 8 || !IsAlphaNum(tags[i]))
>+      return false;
>+    checkedTags++;
>+  }

Again, RegExp rulez da houz!!

>+function WriteAddedLanguages(aListbox)
>+{
>+  // selected languages
>+  var languages = aListbox.selectedItems;

Superfluous variable.

>+  if (gOtherField.value)

This variable name is yet again remarkably uninspired. :(
Please rename (incl. relevant functions below).

>+    let languageIds = gOtherField.value.replace(/\s/g, "").toLowerCase()
>+                                 .split(/\s*,\s*/);

Where would the \s in the split come from?

>+  var languageIds = gOtherField.value.replace(/\s/g, "").toLowerCase()
>+                                     .split(/\s*,\s*/);

See above.

>+  if (invalidLangs.length == 0)
>+    return true;

I'd prefer 
  !invalidLangs.length

>+  const errorMsg = prefLangBundle.getString("illegalOtherLanguage") + " " +
>+                   invalidLangs.join(", ");
>+  const errorTitle = prefLangBundle.getString("illegalOtherLanguageTitle");

consts should have a k prefix.

>+++ b/suite/common/pref/pref-languages-add.xul
>+    <listbox id="availableLanguages" flex="1" seltype="multiple"
>+             preference="intl.accept_languages"
>+             ondblclick="HandleDoubleClick();"
>+             onsynctopreference="return WriteAddedLanguages(this);"/>

One line per attribute, please, if they don't fit onto a single line.

>+      <label value="&languages.customize.others.label;" 
>+             accesskey="&languages.customize.others.accesskey;" 
>+             control="otherLanguages"/>
>+      <textbox id="otherLanguages" size="6" flex="1"/>
>+      <label value="&languages.customize.others.examples;" control="otherLanguages"/>

Hm, do these control attributes suffice for a11y?

>+++ b/suite/common/pref/pref-languages.js
>+// Dictionary of all available languages.
>+var gAvailDict = [];

Can we have a more meaningful name here, please?

> function ReadAvailableLanguages()
...
>+      var str = stringNameProperty[0];

You can get away wi' tat "name"...

>+          var tit;

.. but surely not wi' tit!

>+          if (tit)
>+          {
>+            gAvailDict[i] = [];
>+            gAvailDict[i].push(tit, str, curItem.value);
>+            i++;

gAvailDict[i++] = [tit, str, curItem.value];

>+function ReadActiveLanguages()
...
>+  var arrayOfPrefs = prefString.split(/\s*,\s*/);

These variable names...
One could have used x1, x2, x3 instead...

>+   return undefined;
...
>+  return undefined;

What's the use of that?! 
Especially, since these are the only returns of that method.

>+function InSync(aPrefArray)
...
>+  return aPrefArray.every(
>+    function(aElement, aIndex)
>+    {
>+      return aElement == gLanguages[aIndex].id;
>     }

Neat. :)

>+function MoveUp()
...
>+  var before = selected.previousSibling

Missing ;

>+++ b/suite/common/pref/pref-languages.xul
>+        <listbox id="activeLanguages"
>+                 flex="1" style="width: 0px; height: 0px;"

One line per attribute, please, if they don't fit onto a single line.

Comment 36

[Vlado Valastiak [:wladow]]

Comment on attachment 337868 [details] [diff] [review]
pref-languages patch v0.6

>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+  <prefpane id="languages_pane"
>+            label="&languages.customize.lHeader;"
>+            script="chrome://communicator/content/pref/pref-languages.js">
> 

We should rename string &languages.customize.lHeader; to &languages.customize.title; to be consistent with other prefpanes and remove obsolete string languages.customize.rHeader from pref-languages.dtd file.

These strings seem to be unused too:

languages.customize.engOption.label
languages.customize.engOrder.label
languages.customize.title.label
charset.customize.right.header
languages.customize.reorder.label
languages.customize.additional.label
languages.customize.add.label
languages.customize.save.popup

Comment 37

[Karsten Düsterloh]

Oh, btw: The menulist DefaultCharsetList collides with those on the character_encoding_pane, we need another exception in test_idcheck.xul, ll. 161+.

Comment 38

[[e-mail]]

Comment on attachment 337868 [details] [diff] [review]
pref-languages patch v0.6

Hopefully I've avoided echoing Mnyromyr, but if I have I apologise.

+  gLanguages = gActiveLanguages.getElementsByAttribute("label", "*");
Why not just gActiveLanguages.childNodes?

>               region = regionsBundle.getString(stringLangRegion[1]);
>-              use_region_format = true;
>+              useRegionFormat = true;
Hmm... isn't if (useRegionFormat) the same as if (region)?

>+        let langTitle = GetLanguageTitle(aElement);
>+        if (!langTitle)
>+          langTitle = '[' + aElement + ']';
GetLanguageTitle(aElement) || "[" + aElement + "]"

>+      return aElement == gLanguages[aIndex].id;
.value (this was the cause of the loss of selection!)

>+    gActiveLanguages.selectItem(nextNode)
Missing semicolon.

>+  if (gAvailDict)
gAvailDict is never null.

>+    for (var j = 0; j < gAvailDict.length; j++)
>+    {
>+      if (gAvailDict[j][1] == aKey)
>+        return gAvailDict[j][0];
>+    }
It looks as if pref-languages and pref-languages-add need different data, and it might be worth reorganising the data into two variables: an object that maps codes to language names and an array of language codes available for adding.

>+  var selected = gActiveLanguages.selectedItems[0];
This should use .selectedItem instead.

Comment 39

[Ian Neal]

Attachment: pref-languages patch v0.7

Changes since v0.7
* Sorted pane width problem on Linux (group label changed to Default Character Encoding and label before menulist shorted to Character Encoding).
* No longer loses focus after a move.
* No longer adds valid entries until invalid entries are got rid of.
* Changed function and variable names to have more meaning / conform to rules / be less rude!
* Simplified LanguageCode testing code.
* Sorted out correct number of attributes per line.
* Added missing ;s.
* Removed unused entities from dtd.
* Created mapping array to simplify code elsewhere.
* Switched to using childNodes instead of getElementsByAttribute.
* Use .selectedItem instead of .selectedItems[0].

Only issue I still cannot track down is the issue mentioned in comment 20 and 32.

Comment 40

[Ian Neal]

Attachment: pref-languages patch v0.7a

Changes since v0.7:
* Had a better thought about how to do the mapping array/object from Language Codes to Language Titles - was a bit obvious really, probably why it came to me whilst I was drifting off to sleep!

Comment 41

[[e-mail]]

(In reply to comment #20)
> When I restore focus on the add languages dialog I get 2 (identical) warnings
> in the js console: "Warning: Empty string passed to getElementById().".
> I haven't been able to find out what is happening here - the same thing
> happens if you have entered a invalid language in textfield in the dialog.
The <prefwindow> binding automatically adds persist="lastSelected screenX screenY" to all of its windows. When focus is restored Gecko tries to update the persisted values, which fails because the document element has no id.

Either provide an id or add persist=" " to override the prefwindow binding.

Comment 42

[Karsten Düsterloh]

I like ids. :)
Oh, and btw: gLanguageTitles should be initialized as an object, it's not an array... ;-)

Comment 43

[[e-mail]]

(In reply to comment #35)
> >+      var str = stringNameProperty[0];
> 
> You can get away wi' tat "name"...
> 
> >+          var tit;
> 
> .. but surely not wi' tit!
Tit for tat ;-)

Comment 44

[[e-mail]]

Comment on attachment 338589 [details] [diff] [review]
pref-languages patch v0.7a

Sorry for not spotting some of these nits before.

>+      if ((gLanguageNames[i][2] == "true") &&
We only ever look at the ones where this is true, so there's no point putting them into the array if it isn't. (And then you don't have to put that particular value into the array, see below.)

>+  if (/^(i|x)$/i.test(tags[0]))
Unless I'm mistaken I noticed you only pass in lower case strings, in which case you can leave off the i from all these regexps. You can also simplify (i|x) to [ix].

>+      if (/^(aa|zz|x[a-z]|q[m-z])$/i.test(tags[1]))
Because it uses | this one does need its ()s but the other ones don't.

>+  for (var i = checkedTags; i < tags.length; i++)
>+  {
>+    if (!/^([a-z0-9]{1,8})$/i.test(tags[i]))
>+      return false;
>+    checkedTags++;
You don't use this variable any more. Or you could rewrite this as a while (checkedTags < tags.length) loop instead.

>+    let languageIds = languages.replace(/\s/g, "").toLowerCase().split(",");
Nit: .replace(/\s+/g, "") is minusculely more efficient.

>+            gSelectedLanguages.indexOf(languageId) == -1)
This doesn't quite work; if I remove en then I can't add it back via the textbox, only the list. It might be worth holding gSelectedLanguages as an array, as you split it once already, although you would then have to join it again (but at least when adding languages you can concat before joining.)

>+<prefwindow xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            title="&languages.customize.add.title.label;"
>+            type="child"
>+            onload="OnLoadAddLanguages();"
>+            onbeforeaccept="DoBeforeAccept();"
>+            ondialogaccept="return OnAccept();"
>+            style="width: 41ch; height: 28em;">
As mentioned, this needs an id, so that it can persist its position.

>+        var language;
var language = ""; so you don't have to reset it in the catch below.

>+        if (title)
Hmm, won't title always be set at this point?

>+          gLanguageNames[i] = [title, str, curItem.value];
if (curItem.value == "true")
  gLanguageNames.push([title, str]);

>+    gActiveLanguages.removeChild(gActiveLanguages.firstChild);
Nit: .lastChild

>+        let langTitle = (aKey in gLanguageTitles) ? gLanguageTitles[aKey] :
>+                        "[" + aKey + "]";
Nit: when splitting a ?: I like to split after the ? when the : will fit, but  please use gLanguageTitles.hasOwnProperty(aKey) here anyway.

>+    var before = selected.nextSibling.nextSibling;
>+    if (before)
>+      gActiveLanguages.insertBefore(selected, before);
>+    else
>+      gActiveLanguages.appendChild(selected);
Nit: insertBefore is null-safe (appendChild calls it!)

Comment 45

[Stefan [:stefanh]]

Comment on attachment 338589 [details] [diff] [review]
pref-languages patch v0.7a

+        gAvailableLanguages.appendItem(gLanguageNames[i][0],
+                                       gLanguageNames[i][1]);
.
.
.

+  arrayOfPrefs.forEach(
+    function(aKey)
+    {
+      if (aKey)
+      {
+        let langTitle = (aKey in gLanguageTitles) ? gLanguageTitles[aKey] :
+                        "[" + aKey + "]";
+        gActiveLanguages.appendItem(langTitle, aKey);
       }
     }

I haven't tested the patch, but the reason I didn't used appendItem was bug 250123. In other words, you might end up with no values on listitems that are not visible (check the add languages listbox by loading the subdialog and then scroll down the listbox). I don't remember exactly what happened, but I recall that it didn't felt safe using appendItem.

Comment 46

[Stefan [:stefanh]]

(In reply to comment #44)

> >+          gLanguageNames[i] = [title, str, curItem.value];
> if (curItem.value == "true")
>   gLanguageNames.push([title, str]);
> 

If only "true" languages are in gLanguageNames, a user-defined language that is in the .properties file (but is "false") won't display properly.

Comment 47

[Ian Neal]

Attachment: pref-languages patch v0.7b

Changes since v0.7a:
* gLanguageTitles now initialised as an object.
* Only put languages with curItem.value = true into gLanguageNames array.
* Insert language code / title into gLanguageTitles regardless of value of curItem.value.
* Removed case insensitive flag from regexpressions.
* Use while instead of for loop in tag validity function.
* Switched to suggested replace expression.
* Switched gSelectedLanguages to be an array.
* Gave child window an id to fix error console messages.
* Use hasOwnProperty in langTitle creation.
* Stop null checking before in MoveDown function.
* Switch to using appendChild from appendItem per comment 45.

Comment 48

[Karsten Düsterloh]

Comment on attachment 338718 [details] [diff] [review]
pref-languages patch v0.7b

>+++ b/suite/common/pref/pref-languages.js
>+var gLanguageTitles = new Object();

Please use 
  gLanguageTitles = {};
and I'll fix the test bustage over in 451952.

Comment 49

[Karsten Düsterloh]

Oh, and make DefaultCharsetList become defaultCharsetList, for consistency with the character_encoding_pane.

Comment 50

[[e-mail]]

(In reply to comment #46)
> If only "true" languages are in gLanguageNames, a user-defined language that is
> in the .properties file (but is "false") won't display properly.
We have gLanguageTitles for that.

appendItem is no more or less safe than appendChild, although I think there may be an instance where the patch will fail (using either version).

Comment 51

[[e-mail]]

Comment on attachment 338718 [details] [diff] [review]
pref-languages patch v0.7b

>+      {
>+        var item = document.createElement("listitem");
>+        item.setAttribute("label", gLanguageNames[i][0]);
>+        item.setAttribute("value", gLanguageNames[i][1]);
>+        gAvailableLanguages.appendChild(item);
>+      }
Please revert to appendItem in both places. (See below.)

>+  while (checkedTags < tags.length)
>+  {
>+    if (!/^[a-z0-9]{1,8}$/.test(tags[i]))
tags[checkedTags]

>+  var languages = aListbox.selectedItems;
(Since we never preselect any items, this is safe because in order to select the item the user has to scroll it into view, and thus e.value must work.)

>+var gLanguageTitles = new Object();
Yes, {} is better.

>+    function(aElement, aIndex)
>+    {
>+      return aElement == gLanguages[aIndex].value;
>     }
>+  );
>+}
>+
>+// Called on onsynctopreference.
>+function WriteActiveLanguages()
>+{
>+  return Array.map(gLanguages, function(e) { return e.value; }).join(",");
>+}
(Here the theory is that .value might not work on elements that were never displayed, although I was unable to reproduce the issue in this case.)

Comment 52

[Ian Neal]

Attachment: pref-languages patch v0.7c (Checkin: Comment 55)

Changes since v0.7b:
* Switched back to using appendItem.
* Used {} instead of new Object() for gLanguageTitles.
* Fixed error in tags while loop.

Carrying forward r/sr

Comment 53

[Ian Neal]

Comment on attachment 338762 [details] [diff] [review]
pref-languages patch v0.7c (Checkin: Comment 55)

* Also changed id from DefaultCharsetList to defaultCharsetList

Comment 54

[Ian Neal]

Comment on attachment 335939 [details] [diff] [review]
WIP5, quite close (Checkin: Comment 54)

http://hg.mozilla.org/comm-central/rev/83c3bdc8df86

Comment 55

[Ian Neal]

Comment on attachment 338762 [details] [diff] [review]
pref-languages patch v0.7c (Checkin: Comment 55)

http://hg.mozilla.org/comm-central/rev/5048efa447ab