342560 - Need preference panel for new tag feature

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Karsten Düsterloh]

Now that labels have been replaced by tags, we need to replace the labels preferences panel with a respective tag configuration panel. 
(Since tags are on the 1.8 branch, this is a 1.1a blocker!)

Comment 1

[Robert Kaiser]

Do we really need this for the Alpha (note that Alpha is expected to happen with 1.8.1b2, due on Aug 15th) - or can we let this slip to Beta?

Note that we really need to have it for Beta, as that also marks the L10n freeze...

Karsten, how soon could you fix that?

Comment 2

[Justin Wood (:Callek)]

If this is simply a "copy Thunderbirds Text/Options" for tags, I can tackle this if Karsten Cannot; though I am not sure if I can make the 1.1a date.

Also imho I do think we can let this slip to beta.

Comment 3

[Karsten Düsterloh]

> Do we really need this for the Alpha (note that Alpha is expected to happen
> with 1.8.1b2, due on Aug 15th) - or can we let this slip to Beta?

With the current dialog, you can't neither remove, rename, reorder nor recolor tags (you can add tags via context menus in the main mail window).
We could temporarily use a dead-hacked version of the TB panel for Alpha, that shouldn't be too complicated and would allow us at least to remove tags
(but still no rename/reorder/recolor).
Actually, this bug depends upon bug 342065 (at least for SM).

> Karsten, how soon could you fix that?

Tuesday (11th) in the worst case for the TB dialog take-over.

Comment 4

[Karsten Düsterloh]

XRef: Bug 343875 (TB pref pane)

Comment 5

[Karsten Düsterloh]

Attachment: Band-aid patch: just copy TB's pref pane

This patch just copies over what TB has currently as its (also temporary) pref pane. This UI needs to be reowrked for beta/final!

Comment 6

[[e-mail]]

Comment on attachment 232343 [details] [diff] [review]
Band-aid patch: just copy TB's pref pane

>-/* Function to restore pref values to application defaults */
>+/* Function to restore pref values to application defaults * /
> function restoreColorAndDescriptionToDefaults()

>+    tagService.deleteKey(itemToRemove.getAttribute("value"));
Nit: itemToRemove.value should work.

Do we still need this?
>+    <hbox flex="1">
>+      <listbox id="tagList" flex="1" rows="10"/>
rows doesn't make sense here, because the list is stretched to the box.

I notice that a white tag doesn't work very well ;-)

I'm torn between two ideas for UI for this:
1. use a tree, so you can easily reuse the thread pane styles
2. use a listbox of textboxes and pickers, so you can easily edit inline

Comment 7

[Karsten Düsterloh]

> >-/* Function to restore pref values to application defaults */
> >+/* Function to restore pref values to application defaults * /
> > function restoreColorAndDescriptionToDefaults()

Oops. Yes, this function should go away.

> >+    tagService.deleteKey(itemToRemove.getAttribute("value"));
> Nit: itemToRemove.value should work.

It does.

> rows doesn't make sense here, because the list is stretched to the box.

Oops. Yes.

> I notice that a white tag doesn't work very well ;-)

Heh. :)

> I'm torn between two ideas for UI for this:
> 1. use a tree, so you can easily reuse the thread pane styles
> 2. use a listbox of textboxes and pickers, so you can easily edit inline

With (part of) my patch in bug 342576, the thread pane styles are accessible as CSS classes also, so that would be less of an issue.

Comment 8

[Karsten Düsterloh]

Attachment: Band-aid patch2

Fixed Neil's comments.

Comment 9

[[e-mail]]

(In reply to comment #7)
>>I'm torn between two ideas for UI for this:
>>1. use a tree, so you can easily reuse the thread pane styles
>>2. use a listbox of textboxes and pickers, so you can easily edit inline
>With (part of) my patch in bug 342576, the thread pane styles are accessible as
>CSS classes also, so that would be less of an issue.
Nice, but don't forget to take care that you will always be able to see which tag is selected - you wouldn't want to delete the wrong tag by mistake ;-)

Comment 10

[Ian Neal]

Comment on attachment 232606 [details] [diff] [review]
Band-aid patch2

>Index: resources/content/pref-labels.xul
>===================================================================
>+    <description>&displayText.label;</description>
>+    <hbox flex="1">
>+      <listbox id="tagList" flex="1"/>
>+      <vbox>
>+        <button label="&addTagButton.label;" 
>+                accesskey="&addTagButton.accesskey;"
>+                oncommand="AddTag();"/>
>+        <button label="&removeTagButton.label;" 
>+                accesskey="&removeTagButton.accesskey;" 
>+                oncommand="RemoveTag();"/>
>+      </vbox>
>+    </hbox>
The entities should be deleteTag* rather than removeTag* similarly for the function. Would be good to have a "Restore Defaults" button still here, if not for 1.1a then for the non-band aid solution.

>Index: resources/locale/en-US/mailPrefsOverlay.dtd
>===================================================================

>-<!ENTITY labels.label                     "Labels">
>+<!ENTITY labels.label                     "Tags">
Entity should be really be tags.label so that anyone localising 1.1a picks the change up more easily.

>Index: resources/locale/en-US/pref-labels.dtd
>===================================================================

>+<!ENTITY pref.labels.title          "Tags">
>+<!ENTITY labelsSettings.caption     "Customize Tags">
>+<!ENTITY displayText.label          "Tags can be used to categorize and prioritize your messages.">
Again entities should be changed to help anyone localising 1.1a.

>+<!ENTITY addTagButton.label         "Add">
>+<!ENTITY addTagButton.accesskey     "A">
>+<!ENTITY removeTagButton.label      "Delete">
>+<!ENTITY removeTagButton.accesskey  "D">
deleteTag* instead of removeTag*

In any non-band aid solution should also have the ability to disable the Add/Delete/Restore buttons by pref setting - see http://lxr.mozilla.org/seamonkey/search?string=disable_button

Comment 11

[Karsten Düsterloh]

Attachment: Band-aid patch 3

Addressed Ian's comments.

Comment 12

[Robert Kaiser]

Comment on attachment 232621 [details] [diff] [review]
Band-aid patch 3

>-<!ENTITY pref.labels.title          "Labels">

Note that this might be referenced in help, so be sure to change it to pref.tags.title there as well if that's the case.

a=me for 1.1a given its gets r+

Comment 13

[Karsten Düsterloh]

> >-<!ENTITY pref.labels.title          "Labels">
> 
> Note that this might be referenced in help,

I don't see why it would and lxr claims it isn't. We do need help to reflect the UI changes, though; I'll do that tonight.

Comment 14

[Karsten Düsterloh]

Comment on attachment 232621 [details] [diff] [review]
Band-aid patch 3

Restoring the default tags via addTag does not create the correct keys, I need to create the pref.js entries directly. IanN, please suppose that instead of the "tagService.addTag(tag, color);" call in RestoreDefaults.

++

Comment 15

[Robert Kaiser]

(In reply to comment #13)
> > >-<!ENTITY pref.labels.title          "Labels">
> > 
> > Note that this might be referenced in help,
> 
> I don't see why it would and lxr claims it isn't. We do need help to reflect
> the UI changes, though; I'll do that tonight.

I was thinking about the original intent behind bug 289444 (referencing the pref panel titles from help docs), but it seems that has never been actually done, so no need to worry, that's true.
Thanks for looking at the help docs as well, though :)

Comment 16

[Karsten Düsterloh]

Attachment: Band-aid patch 4

Restore default tags with correct keys. Would be taking over a+ if I could, so imagine it's there. ;-)

Comment 17

[Karsten Düsterloh]

Attachment: help patch

Help patch for tags in general, thus slightly ahead of the ban-aid patch.

Comment 18

[Robert Kaiser]

Comment on attachment 232801 [details] [diff] [review]
help patch

a=me for landing the appropriate changes in 1.1a code (the suite/locales/ part doesn't apply there, and pref-help.js probably still lives in xpfe/ there)

Comment 19

[Ian Neal]

Comment on attachment 232800 [details] [diff] [review]
Band-aid patch 4

r/a=me

Comment 20

[Ian Neal]

Comment on attachment 232801 [details] [diff] [review]
help patch

help-index1.rdf files are listed alphabetically so you need to move "Tags" into the correct place in the file rather than just replacing in current position for both /suite and /xpfe versions.

>Index: extensions/help/resources/locale/en-US/mail_help.xhtml
>===================================================================
There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree?

>Index: suite/common/pref/pref-help.js
>===================================================================
As Kairo said need the xpfe version for 1.1a

>Index: suite/locales/en-US/chrome/common/help/mail_help.xhtml
>===================================================================
There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree?

>Index: suite/locales/en-US/chrome/common/help/suite-toc.rdf
>===================================================================
There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree?

Comment 21

[Karsten Düsterloh]

Attachment: help patch, v2trunk

Corrected index order, against trunk.

Comment 22

[Karsten Düsterloh]

Attachment: help patch, v2branch

Same textual content as attachment 233149 [details] [diff] [review], but against 1.8 branch (i.e. different pref-help.js location).

Comment 23

[Robert Kaiser]

Comment on attachment 233150 [details] [diff] [review]
help patch, v2branch

a=me for 1.1a given it gets r+

Comment 24

[Karsten Düsterloh]

Comment on attachment 232800 [details] [diff] [review]
Band-aid patch 4

Checked in into MOZILLA_1_8_BRANCH for SM 1.1a and trunk (to keep it in sync).

Comment 25

[Ian Neal]

Comment on attachment 233149 [details] [diff] [review]
help patch, v2trunk

>Index: extensions/help/resources/locale/en-US/mail_help.xhtml
>===================================================================
>@@ -4749,37 +4753,40 @@ to filter unwanted mail.</p>

>+  <li><strong>Customize Tags</strong>: Specifies the tag text and the color
>+    for each tag. You can edit or replace the default tag text with your
>+    own text (up to 32 characters). To change the tag color, click the color
>+    chip next to that tag and select a new color. Use the Move Up and Move Down
>+    buttons to reorder your tags. Top tags have higher priority when coloring
>+    tagged messages.</li>
This last bit needs rewording to make the meaning clearer.

>Index: suite/locales/en-US/chrome/common/help/mail_help.xhtml
>===================================================================
>@@ -4749,37 +4753,40 @@ to filter unwanted mail.</p>

>+  <li><strong>Customize Tags</strong>: Specifies the tag text and the color
>+    for each tag. You can edit or replace the default tag text with your
>+    own text (up to 32 characters). To change the tag color, click the color
>+    chip next to that tag and select a new color. Use the Move Up and Move Down
>+    buttons to reorder your tags. Top tags have higher priority when coloring
>+    tagged messages.</li>
As above.
r=me with that change.

Comment 26

[Ian Neal]

Comment on attachment 233150 [details] [diff] [review]
help patch, v2branch

>Index: extensions/help/resources/locale/en-US/mail_help.xhtml
>===================================================================
>@@ -4749,37 +4754,39 @@ to filter unwanted mail.</p>

>+  <li><strong>Customize Tags</strong>: Specifies the tag text and the color
>+    for each tag. You can edit or replace the default tag text with your
>+    own text (up to 32 characters). To change the tag color, click the color
>+    chip next to that tag and select a new color. Use the Move Up and Move Down
>+    buttons to reorder your tags. Top tags have higher priority when coloring
>+    tagged messages.</li>
As per trunk patch, r=me with that change.

Comment 27

[Karsten Düsterloh]

Attachment: help patch, v3trunk (checked-in version)

Comment 28

[Karsten Düsterloh]

Attachment: help patch, v3branch (checked-in version)

Comment 29

[Karsten Düsterloh]

Checked in help changes into trunk and MOZILLA_1_8_BRANCH. We may need to change some wording when the real pref panel is there, though; e.g. Neil suggested using "more important"/"less important" for the tag order buttons...

Comment 30

[Alexander Kluss]

If it's possible to add a option in the prefs panel to change the text decoration (bold, underline, cross out) of the tags?
IMHO it's to little that I could only change the text color of tags.

Comment 31

[Mike Cowperthwaite]

*** Bug 343875 has been marked as a duplicate of this bug. ***

Comment 32

[Mike Cowperthwaite]

*** Bug 352332 has been marked as a duplicate of this bug. ***

Comment 33

[Karsten Düsterloh]

Attachment: Tag pref panel, v1

New tag pref panel for SM, including tag ordering.
Further changes:
- call pref panel from tag context menu
- help update

Comment 34

[[e-mail]]

Comment on attachment 238834 [details] [diff] [review]
Tag pref panel, v1

>-        <treeitem>
>+        <treeitem id="mailtagspref">
Hmm?

>+// <listbox> have drawbacks which make them seem inappropriate here.
What's the drawback of a listbox, the fact that you need allowevents="true" to be able to embed arbitrary xul or something else?

>+// init global stuff before the wsm is used
Any particular reason why; is SetFields called before Startup or something?

>+  // Creating a colorpicker dynamically in an onload handler is really sucky.
>+  // You MUST first set its type attribute, then add the element to the DOM
>+  // and finally set the color property(!) afterwards. Try in any other order
>+  // and fail... :-(
Would it help to make the colorpicker constructor read the color attribute?

>+function OnFocus(aTarget)
>+{
>+  // alas, the textbox doesn't catch this event, but its html:input element
If this is still applicable with a listbox conversion, you could add a capturing focus event listener to the box, then just walk up until you get to the right element.

> function AddTag()
I don't like this. I'd prefer for it to immediately add a row to the list.

Comment 35

[Karsten Düsterloh]

(In reply to comment #34)
> (From update of attachment 238834 [details] [diff] [review] [edit])
> >-        <treeitem>
> >+        <treeitem id="mailtagspref">
> Hmm?

nsPrefWindow needs this when opening the tag pref panel from a context menu.

> >+// <listbox> have drawbacks which make them seem inappropriate here.
> What's the drawback of a listbox

The overflow-y scrollbars on listboxes overlap the content. And if the rightmost column consists of eg. colorpickers, these (on Win2k at least) are drawn incomplete! (This seems to be fixed on trunk, but not on the 1.8 branch.)

> , the fact that you need allowevents="true" to
> be able to embed arbitrary xul or something else?
 
Okay, that'd work with trunk and 1.8 branch.
Nevertheless, I actually don't see the point of a listbox here...

> >+// init global stuff before the wsm is used
> Any particular reason why;
> is SetFields called before Startup 

Yes, it is.

>>+  // Creating a colorpicker dynamically in an onload handler is really sucky.
>>+  // You MUST first set its type attribute, then add the element to the DOM
>>+  // and finally set the color property(!) afterwards. Try in any other order
>>+  // and fail... :-(
> Would it help to make the colorpicker constructor read the color attribute?

Yes, adding 
    this.color = this.getAttribute("color");
to the colorpicker-button constructor seems to fix this problem.

> > function AddTag()
> I don't like this. I'd prefer for it to immediately add a row to the list.

Good idea. 

Comment 36

[[e-mail]]

(In reply to comment #35)
>>What's the drawback of a listbox
>The overflow-y scrollbars on listboxes overlap the content. And if the
>rightmost column consists of eg. colorpickers, these (on Win2k at least) are
>drawn incomplete! (This seems to be fixed on trunk, but not on the 1.8 branch.)
Ah yes, there was a similar problem with the filter/search editor, it seems that the workaround there was an amount of padding.
Putting the colourpickers on the left also works.

>Nevertheless, I actually don't see the point of a listbox here...
I thought that a listbox looked more obviously like a list, not only from a visual point of view but it might also improve accessibility.

>>Would it help to make the colorpicker constructor read the color attribute?
>Yes, adding 
>     this.color = this.getAttribute("color");
>to the colorpicker-button constructor seems to fix this problem.
Feel free to file that in a separate bug if you think it would help.

Comment 37

[Karsten Düsterloh]

Attachment: Tag pref panel, v2

Listbox-based version, including dropping of the newTag dialog.

Comment 38

[[e-mail]]

Comment on attachment 239402 [details] [diff] [review]
Tag pref panel, v2

This didn't work on a new profile which had no tags. In such a case I suggest you "Restore Defaults".

>-      <listbox id="tagList" flex="1"/>
>+      <listbox id="tagList">
>+        <listcols>
>+          <listcol/>
This didn't work properly for me. In one build, the listbox was narrow, and could have been wider, while in the other it was too wide. I suggest flex="1" on the listbox, the first listcol and each textbox that you create.

>-</page>
>+</page>
>\ No newline at end of file
Oops :-P

>+  // Listitems we append to the "end" of the listbox and would be rendered
>+  // outside the clipping area don't get their text and color set!
>+  // So we stuff them in bottom-up... :-|
For textboxes you can fix that by setting attributes on the items. (Filed that bug on colourpickers yet?)

>+  textbox.setAttribute('onfocus', 'OnFocus(this);');
I still don't like this being done through attributes.

>+    while (aTag + '#' + suffix in aTagList)
>+      ++suffix;
>+    aTag += '#' + suffix;
By comparison the Filter Editor uses "Untitled Filter 2" etc.

>+<!ENTITY raiseTagButton.label             "Raise Importance">
>+<!ENTITY raiseTagButton.accesskey         "+">
>+<!ENTITY lowerTagButton.label             "Lower Importance">
>+<!ENTITY lowerTagButton.accesskey         "-">
I'm not convinced of these strings: I was thinking of "&More Important" and "&Less Important".

>+/* ::::: Tags ::::: */
>+
>+#tagList listcell
>+{
>+  padding-left:  16px;
>+  padding-right: 16px;
>+}
>+
>+#tagList .listheader-label
>+{
>+  text-align: center;
>+}
Descendent selectors considered harmful. See http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Avoid_the_descendant_selector.21

Another option which you may want to consider is to set flex="2" on the first listcol and flex="1" on the second listcol, and then sprinkle pack="center" on listcells and listheads.

Anyway, nearly there, I think.

Comment 39

[Ian Neal]

Comment on attachment 239402 [details] [diff] [review]
Tag pref panel, v2

>Index: mailnews/base/prefs/resources/content/pref-labels.js
>===================================================================
>+function GetFields(aPageData)
> {
>+  // collect the tag definitions from the UI and store them in the wsm
>+  var tags = [];
>+  for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling)
You could use here (and is more readable):
for each (var entry in gTagList.childNodes) 

>+function SetFields(aPageData)
> {

>+    var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]
>+                               .getService(Components.interfaces.nsIMsgTagService);
>+    var tagArray = tagService.getAllTags({});
>+    tags = aPageData[ACTIVE_TAGS_ID] = [];
>+    for (i = 0; i < tagArray.length; ++i)
You could use here:
for each (var t in tagArray)

> function AddTag()
>+{
>+  // Add a new tag to the UI here. It will be only be written to the
>+  // preference system only if the OKHandler is executed!
>+
>+  // create unique tag name
>+  var dupeList = {}; // indexed by tag
>+  for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling)
As before, could use (and more readable):
for each (var entry in gTagList.childNodes)

>+function RaiseTag()
> {
>+  // Move the selected tag one position up in the tagList's child order.
>+  // This reordering may require changing ordinal strings, which will happen
>+  // when we write tag data to the preferences system in the OKHandler.
>+  var entry = gTagList.selectedItem;
>+  entry.parentNode.insertBefore(entry, gTagList.getPreviousItem(entry, 1));
>+  UpdateTagEntry(entry.taginfo, entry);
>+  FocusTagEntry(entry);
>+}
>+
>+function LowerTag()
>+{
>+  // Move the selected tag one position down in the tagList's child order.
>+  // This reordering may require changing ordinal strings, which will happen
>+  // when we write tag data to the preferences system in the OKHandler.
>+  var entry = gTagList.selectedItem;
>+  entry.parentNode.insertBefore(entry, gTagList.getNextItem(entry, 2));
>+  UpdateTagEntry(entry.taginfo, entry);
>+  FocusTagEntry(entry);
> }
Is it worth looking at combining RaiseTag and LowerTag into one function with an argument?

> 
>+function OnOK()
>+{
>+  var i;
>+  var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]
>+                             .getService(Components.interfaces.nsIMsgTagService);
>+  // we may be called in another page's context, so get the stored data from the
>+  // wsm the hard way
>+  var pageData    = parent.hPrefWindow.wsm.dataManager.pageData[TAGPANEL_URI];
>+  var activeTags  = pageData[ACTIVE_TAGS_ID];
>+  var deletedTags = pageData[DELETED_TAGS_ID];
>+
>+  // remove all deleted tags from the preferences system
>+  for (var key in deletedTags)
>+    tagService.deleteKey(key);
>+
>+  // count dupes so that we can eliminate them later
>+  var dupeCounts = {}; // indexed by tag
>+  for (i = 0; i < activeTags.length; ++i)
>+  {
>+    var tag = activeTags[i].tag;
You could use:
for each (var tagInfo in activeTags)
{
  var tag = tagInfo.tag;

>+  // Now write tags to the preferences system, create keys and ordinal strings.
>+  // Manually set ordinal strings are NOT retained!
>+  var lastTagInfo = null;
>+  for (i = 0; i < activeTags.length; ++i)
>+  {
>+    var tagInfo = activeTags[i];
Again could use:
for each (var tagInfo in activeTags)

>Index: mailnews/base/prefs/resources/locale/en-US/pref-labels.dtd
>===================================================================
>@@ -31,17 +31,24 @@
>    - use your version of this file under the terms of the MPL, indicate your
>    - decision by deleting the provisions above and replace them with the notice
>    - and other provisions required by the LGPL or the GPL. If you do not delete
>    - the provisions above, a recipient may use your version of this file under
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
>-<!ENTITY pref.tags.title            "Tags">
>-<!ENTITY pref.tags.caption          "Customize Tags">
>-<!ENTITY pref.tags.description      "Tags can be used to categorize and prioritize your messages.">
>-<!ENTITY addTagButton.label         "Add">
>-<!ENTITY addTagButton.accesskey     "A">
>-<!ENTITY deleteTagButton.label      "Delete">
>-<!ENTITY deleteTagButton.accesskey  "D">
>-<!ENTITY restoreDefaults.label      "Restore Defaults">
>-<!ENTITY restoreDefaults.accesskey  "R">
>+<!ENTITY pref.tags.title                  "Tags">
>+<!ENTITY pref.tags.caption                "Customize Tags">
>+<!ENTITY pref.tags.description            "Tags can be used to categorize and prioritize your messages. Modify the appearance and importance of tags using the settings below. Tags near the top are more important than those further down.">
>+<!ENTITY tagColumn.label                  "Tag">
>+<!ENTITY colorColumn.label                "Color">
>+<!ENTITY defaultTagName.label             "New Tag">
>+<!ENTITY addTagButton.label               "Add">
>+<!ENTITY addTagButton.accesskey           "A">
>+<!ENTITY deleteTagButton.label            "Delete">
>+<!ENTITY deleteTagButton.accesskey        "D">
>+<!ENTITY raiseTagButton.label             "Raise Importance">
>+<!ENTITY raiseTagButton.accesskey         "+">
>+<!ENTITY lowerTagButton.label             "Lower Importance">
>+<!ENTITY lowerTagButton.accesskey         "-">
>+<!ENTITY restoreDefaultsButton.label      "Restore Defaults">
>+<!ENTITY restoreDefaultsButton.accesskey  "R">
Why the extra space between the entity name and entity value?

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
>@@ -610,41 +610,16 @@ function ToggleMessageTag(key, addKey)
>       prevHdrFolder = msgHdr.folder;
>     }
>     messages.AppendElement(msgHdr);
>   }
>   if (prevHdrFolder)
>     prevHdrFolder[toggler](messages, key);
> }
> 
>-function AddTag()
>-{
>-  var args = {result: "", okCallback: AddTagCallback};
>-  var dialog = window.openDialog("chrome://messenger/content/newTagDialog.xul",
>-                                 "",
>-                                 "chrome,titlebar,modal",
>-                                 args);
>-}
If we are no longer calling newTagDialog.xul it should no longer be packaged in mailnews/jar.mn

Comment 40

[[e-mail]]

Using for each on arrays is dubious because of the possibility of extensions polluting the array prototype. It could even happen for a NodeList too.

Comment 41

[Karsten Düsterloh]

Attachment: Tag pref panel, v3

Most comments addressed, some notes on those I didn't:

(In reply to comment #38)
> This didn't work on a new profile which had no tags. In such a case I suggest
> you "Restore Defaults".

This is bug 354381.

> >+  // Listitems we append to the "end" of the listbox and would be rendered
> >+  // outside the clipping area don't get their text and color set!
> >+  // So we stuff them in bottom-up... :-|
> For textboxes you can fix that by setting attributes on the items.
> (Filed that bug on colourpickers yet?)

Yes, I filed 354065 (against toolkit, since our respective syncing bug is open anyway). But without that colorpicker fix there's no point in setting the textbox' _attribute_ either.

> >+<!ENTITY raiseTagButton.label             "Raise Importance">
> >+<!ENTITY raiseTagButton.accesskey         "+">
> >+<!ENTITY lowerTagButton.label             "Lower Importance">
> >+<!ENTITY lowerTagButton.accesskey         "-">
> I'm not convinced of these strings: I was thinking of "&More Important" and
> "&Less Important".

Buttons should describe actions (i.e. use verbs) and all other buttons on this panel do.


(In reply to comment #39)
> >+  for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling)
> You could use here (and is more readable):
> for each (var entry in gTagList.childNodes) 

No, I can't.
ECMA-357 explicitly states that the order of element traversal is implementation-dependent, and I do need to rely on strict ordering here.


Carrying over sr+.

Comment 42

[Karsten Düsterloh]

Comment on attachment 240350 [details] [diff] [review]
Tag pref panel, v3

Checked in on trunk.

Comment 43

[Karsten Düsterloh]

Landed on MOZILLA_1_8_BRANCH. 
Mind that bug 354381 is not yet there!

Comment 44

[[e-mail]]

Attachment: Small optimization

Comment 45

[[e-mail]]

Comment on attachment 246536 [details] [diff] [review]
Small optimization

Or I could use event.currentTarget if you prefer.

Comment 46

[Karsten Düsterloh]

Comment on attachment 246536 [details] [diff] [review]
Small optimization

Nice. :)
We should also kill the misleading onfocus attributes in the comment in ll. 40 + 43.