Bug 404263 already migrated the MailNews main pref pane, but there's more work to be done: Message Display Notifications Composition Send Format Addressing Junk Mail Tags Return Receipts Character Encoding Offline & Disk Space Bug 394522 is rather crowded by now, so spinning this off; we probably need to spin off other bugs from here.

(In reply to comment #0) > Addressing Already covered by bug 408613

Let's start with the tags panel, plus some minor additional ID cleanup.

Comment on attachment 311378 [details] [diff] [review] Tags, v1 >+#tags_pane .listheader-label It would be better to do #tagList > listhead >+#tags_pane listcell And #tagList > listitem > listcell >+#tags_pane textbox And #tagList > listitem > listcell > textbox Or you could just set .flex = 1 in the script. >+function BisectString(aPrev, aNext) I think I see what this is for - you add a tag in the middle of the list, then start typing its name, so you don't know its key yet and you need to make up an ordinal, but I don't see why you can't do things the old way in the non-instant apply case. I'm also worried that, in the instant apply case, if you could create the tag, start using it, then change its label, it would confuse the tag service, as the tag would be deleted and recreated. Also, did you consider swapping the ordinals when you use MoveTag?

(In reply to comment #3) > >+function BisectString(aPrev, aNext) > I think I see what this is for - you add a tag in the middle of the list, > then start typing its name, so you don't know its key yet and you need to > make up an ordinal, but I don't see why you can't do things the old way in > the non-instant apply case. Actually, the reason for this function is rather independent from instant vs. non-instant apply. I just need a way to generate ordinals between other ordinals. > I'm also worried that, in the instant apply case, if you could create the tag, > start using it, then change its label, it would confuse the tag service, as > the tag would be deleted and recreated. Why should it? One tag is deleted and another one is created, usually even with a different name. In fact I didn't notice any oddities while playing around with it. > Also, did you consider swapping the ordinals when you use MoveTag? Hm, that way I'd only need new ordinals when adding tags, true, but I'd still need BisectString then. And it would probably mean having ordinals in cases where actually none would be needed else. I'll update the CSS as soon as Ian has commented.

Comment on attachment 311378 [details] [diff] [review] Tags, v1 >Index: mailnews/base/prefs/resources/content/pref-labels.js >=================================================================== > function AppendTagEntry(aTagInfo, aRefChild) > { > // Creating a colorpicker dynamically in an onload handler is really sucky. > // You MUST first set its type attribute (to select the correct binding), then > // add the element to the DOM (to bind the binding) and finally set the color > // property(!) afterwards. Try in any other order and fail... :-( You have a "var key = aTagInfo.key;" after this line, is it still needed? >@@ -157,18 +125,19 @@ function AppendTagEntry(aTagInfo, aRefCh > > var colorCell = document.createElement('listcell'); > var colorpicker = document.createElement('colorpicker'); > colorpicker.setAttribute('type', 'button'); > colorCell.appendChild(colorpicker); > > var entry = document.createElement('listitem'); > entry.addEventListener('focus', OnFocus, true); >+ entry.addEventListener('change', OnChange, false) Nit: Missing ; >+function RecalculateOrdinal(aEntry) >+{ >+ // Calculate a new ordinal for the given entry, assuming that both its >+ // predecessor's and successor's are correct, i.e. ord(p) < ord(s)! >+ var tagInfo = aEntry.tagInfo; >+ // get neighbouring ordinals >+ var prevOrdinal = '', nextOrdinal = ''; >+ var prev = aEntry.previousSibling; >+ if (prev && prev.nodeName == 'listitem') // first.prev == listhead >+ prevOrdinal = GetTagOrdinal(prev.tagInfo); >+ var next = aEntry.nextSibling; >+ if (next) >+ { >+ nextOrdinal = GetTagOrdinal(next.tagInfo); >+ } >+ else >+ { >+ // ensure key < nextOrdinal if entry is the last/only entry >+ nextOrdinal = prevOrdinal || tagInfo.key; >+ nextOrdinal = String.fromCharCode(nextOrdinal.charCodeAt(0) + 2); >+ } >+ >+ var ordinal = tagInfo.key; If you move this line further up then only need to retrieve tagInfo.key the once. >+ if (prevOrdinal < ordinal && ordinal < nextOrdinal) >+ { >+ // no ordinal needed, just clear it >+ SetTagOrdinal(tagInfo, '') >+ return; >+ } > > function OnOK() > { > var i; Nit: This line is no longer needed. >Index: mailnews/base/prefs/resources/content/pref-labels.xul >=================================================================== >+<!-- > <?xul-overlay href="chrome://global/content/globalOverlay.xul"?> > <?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?> >- >+--> Nit: Remove these lines if they are no longer needed. As discussed on IRC there's also the possible renaming of these files too. r=me with those items addressed

I landed this on trunk. It contains addressed review comments and also includes changes for and the move from pref-labels.* to pref-tags.*.

I've just fixed the last remaining blocker to this bug, so this is now fixed :-)