Convert simple <listbox> to <richlistbox>

RESOLVED FIXED in Thunderbird 63.0

Status

defect
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

(Depends on 1 bug)

unspecified
Thunderbird 63.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(24 attachments, 39 obsolete attachments)

26.57 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.07 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.86 KB, patch
jorgk
: review+
aceman
: feedback+
Details | Diff | Splinter Review
11.51 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
32.08 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
2.63 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
9.07 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
6.40 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
18.03 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
9.20 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
7.24 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
6.79 KB, image/png
Details
8.36 KB, patch
Details | Diff | Splinter Review
3.22 KB, patch
Paenglab
: review+
aceman
: feedback+
Details | Diff | Splinter Review
9.61 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
869 bytes, patch
jorgk
: review+
Details | Diff | Splinter Review
13.14 KB, patch
jorgk
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
1.17 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.03 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.48 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
1.71 KB, patch
aceman
: review+
Details | Diff | Splinter Review
6.10 KB, image/png
Details
1.90 KB, patch
aceman
: review+
Details | Diff | Splinter Review
12.47 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
This bug is to convert "simple" <listbox> to <richlistbox>. Simple means, they don't need much JS logic for the conversion.
Posted patch listboxConvert.patch (obsolete) — Splinter Review
This patch convert already a lot of <listbox>.

I ended in converting more than I wanted at the beginning. Now I have a problem with the Junk listbox in am-junk.*.

Aceman, please can you help me that clicking the entries changes the checked/unchecked stste? I don't know how to implement this.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8992176 - Flags: feedback?(acelists)
For the reference this is a list of all <listbox> in the tree https://dxr.mozilla.org/comm-central/search?q=%3Clistbox+-path%3Aobj-x86
Some comments:
- Aceman won't be able to do much right now, he has machine trouble.
- I'm a little confused here:
  + So you've converted some <listbox> to <richlistbox>, but not all?
  + What's the story about bug 1472556 comment #15. Where are those JS changes required?
  + Should we close bug 1472556 and continue here?
  + Should we backout most of bug 1472556 (https://hg.mozilla.org/comm-central/rev/be072a52c30e),
    that is, forking of the listbox binding to C-C.
Flags: needinfo?(richard.marti)
(In reply to Jorg K (GMT+2) from comment #3)
> Some comments:
> - Aceman won't be able to do much right now, he has machine trouble.
> - I'm a little confused here:
>   + So you've converted some <listbox> to <richlistbox>, but not all?

Yes, the simple ones like the tag list in display.inc.xul or in systemIntegrationDialog.xul where the listbox isn't needed. But I touched also some which needs some JS changes because richlists aren't 1:1 convertible. This are the others in this patch.

There are harder ones like the Attachment list, see bug 1435688 patch Part 2. Or bug 1472744 for the sanitizer. And also like you see with the junk part in my patch, I stumbled already on the checkbox logic.

>   + What's the story about bug 1472556 comment #15. Where are those JS
> changes required?

See my patch. I tink, we can use bug 1472556 to backout the listitem bindings. There are also some we use, like listcell-iconic, which we then can remove too.

>   + Should we close bug 1472556 and continue here?

No, for the back-out/removal of the listbox bindings.

>   + Should we backout most of bug 1472556
> (https://hg.mozilla.org/comm-central/rev/be072a52c30e),
>     that is, forking of the listbox binding to C-C.

I think, not yet. When all listboxes are converted, we can remove the whole listbox.xml and it's bindings. Or when m-c is faster with removed the listbox display logic, we can remove all because it would probably no more work.
Flags: needinfo?(richard.marti)
Posted patch listboxConvert.patch - JK (obsolete) — Splinter Review
This follows
https://hg.mozilla.org/mozilla-central/rev/72c622b851f3#l2.28 and
https://hg.mozilla.org/mozilla-central/rev/72c622b851f3#l2.71
more closely. Clicking the items works, however, pressing space only updates the box then it is next hovered :-( - I tried a few things, but without success :-(

Paolo, you're the author of the patch we're following here, can you please lend a hand. Why does this not work?

+    case "keypress":
+      if (event.key == " ") {
+        let item = event.target.currentItem;
+        if (item) {
+          // This doesn't work, it only refreshes when the checkbox gets focus again :-(
+          this.toggleItemCheckbox(item);
+          
+          // Tried all this without success:
+          // 1) item.click();
+          // 2) item.blur(); item.firstChild.focus();
+        }
+      }
+      break;
+     }
+  },
+
+  toggleItemCheckbox(item) {
+    if (!item.hasAttribute("checked")) {
+      item.setAttribute("checked", "true");
+    } else {
+      item.removeAttribute("checked");
+    }
+  }
Attachment #8992176 - Attachment is obsolete: true
Attachment #8992176 - Flags: feedback?(acelists)
Flags: needinfo?(paolo.mozmail)
Attachment #8992219 - Flags: feedback?(acelists)
Comment on attachment 8992176 [details] [diff] [review]
listboxConvert.patch

Review of attachment 8992176 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this conversion.
Why only some of the new 'richlistbox' elements get the class="theme-listbox" and not all?

::: mail/base/content/systemIntegrationDialog.xul
@@ +56,5 @@
> +              label="&feeds.label;"
> +              tooltiptext="&unsetDefault.tooltip;"/>
> +  </vbox>
> +
> +<separator class="groove"/>

Bad indent?

::: mail/components/preferences/attachmentReminder.xul
@@ +28,5 @@
>  
>      <groupbox>
>        <label control="keywordList">&attachKeywordText.label;</label>
>        <hbox>
> +        <richlistbox id="keywordList" flex="1" height="250px"

It is sad if richlistbox has no 'rows' equivalent. But then, does height need to be specified in px? Maybe em or other relative vertical measure could be used.

::: mail/extensions/mailviews/content/mailViewList.xul
@@ -30,5 @@
> -        <listcols>
> -          <listcol flex="1" width="0"/>
> -        </listcols>
> -        <listhead>
> -          <listheader label="&viewName.label;"/>

What is this replaced with? Are we going to lose the header label?

::: mailnews/base/prefs/content/am-junk.js
@@ +86,4 @@
>  
>      // Due to bug 448582, we have to use setAttribute to set the
>      // checked value of the listitem.
>      item.setAttribute("checked", currentArray.includes(abItem.URI));

So if you have to mimic checkboxes using images, is there code to manage this 'checked' attribute on click? You may need to add onclick= function to the listbox with a function that toggles the value. Or can you put <checkbox> elements inside the richlistbox? It seems to take any elements inside richlistitem. In any case you may need to touch the code in onSaveWhiteList() that reads the 'checked' attribute as it may be on a different element now.

::: mailnews/import/content/importDialog.js
@@ +401,5 @@
>  function AddModuleToList(moduleName, index)
>  {
>    var body = document.getElementById("moduleList");
>  
> +  let item = document.createElement('richlistitem');

Double quotes please (while touching this).

@@ +417,5 @@
>  
>    // Add item to allow for new account creation.
> +  let item = document.createElement("richlistitem");
> +  let label = document.createElement("label");
> +  label.setAttribute("value", gFeedsBundle.getString('ImportFeedsCreateNewListItem'));

Double quotes please (while touching this).
Attachment #8992176 - Attachment is obsolete: false
(In reply to :aceman from comment #6)
> ::: mailnews/base/prefs/content/am-junk.js
> @@ +86,4 @@
> >  
> >      // Due to bug 448582, we have to use setAttribute to set the
> >      // checked value of the listitem.
> >      item.setAttribute("checked", currentArray.includes(abItem.URI));
> 
> So if you have to mimic checkboxes using images, is there code to manage
> this 'checked' attribute on click? You may need to add onclick= function to
> the listbox with a function that toggles the value. Or can you put
> <checkbox> elements inside the richlistbox? It seems to take any elements
> inside richlistitem. In any case you may need to touch the code in
> onSaveWhiteList() that reads the 'checked' attribute as it may be on a
> different element now.

Try this here:
for (let abItem of abItems) {
  let item = document.createElement("richlistitem");
  let checkbox = document.createElement("checkbox");
  checkbox.setAttribute("label", abItem.label);
  checkbox.setAttribute("checked", currentArray.includes(abItem.URI));
  item.setAttribute("value", abItem.URI);
  item.appendChild(checkbox);
  wList.appendChild(item);
}
Posted patch listboxConvert.patch - JK v2 (obsolete) — Splinter Review
I did what Aceman suggested. That works.

The
  if (wlNode.firstChild.getAttribute("checked") == "true") {
is untested.

Can we get the rest done soon?
Attachment #8992176 - Attachment is obsolete: true
Attachment #8992219 - Attachment is obsolete: true
Attachment #8992219 - Flags: feedback?(acelists)
Flags: needinfo?(paolo.mozmail)
Many thanks for helping.

(In reply to :aceman from comment #6)
> Comment on attachment 8992176 [details] [diff] [review]
> listboxConvert.patch
> 
> Review of attachment 8992176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on this conversion.
> Why only some of the new 'richlistbox' elements get the
> class="theme-listbox" and not all?

This is a special class for richlistboxes in normal XUL dialogs to look like listitems. Mostly for Windows where treeitems/listboxitems look differently. Richlistboxes in the prefs are styled by the prefs stylesheets and don't need this special styling.

> ::: mail/base/content/systemIntegrationDialog.xul
> @@ +56,5 @@
> > +              label="&feeds.label;"
> > +              tooltiptext="&unsetDefault.tooltip;"/>
> > +  </vbox>
> > +
> > +<separator class="groove"/>
> 
> Bad indent?

I'll check.

> ::: mail/components/preferences/attachmentReminder.xul
> @@ +28,5 @@
> >  
> >      <groupbox>
> >        <label control="keywordList">&attachKeywordText.label;</label>
> >        <hbox>
> > +        <richlistbox id="keywordList" flex="1" height="250px"
> 
> It is sad if richlistbox has no 'rows' equivalent. But then, does height
> need to be specified in px? Maybe em or other relative vertical measure
> could be used.

In prefs the height of the items is set by px, so we can follow with using px.

> ::: mail/extensions/mailviews/content/mailViewList.xul
> @@ -30,5 @@
> > -        <listcols>
> > -          <listcol flex="1" width="0"/>
> > -        </listcols>
> > -        <listhead>
> > -          <listheader label="&viewName.label;"/>
> 
> What is this replaced with? Are we going to lose the header label?

When you check the actual mailViews listbox, no header are shown, so not needed. This is also why I remove the string from locale.

> ::: mailnews/import/content/importDialog.js
> @@ +401,5 @@
> >  function AddModuleToList(moduleName, index)
> >  {
> >    var body = document.getElementById("moduleList");
> >  
> > +  let item = document.createElement('richlistitem');
> 
> Double quotes please (while touching this).

Will do.

> @@ +417,5 @@
> >  
> >    // Add item to allow for new account creation.
> > +  let item = document.createElement("richlistitem");
> > +  let label = document.createElement("label");
> > +  label.setAttribute("value", gFeedsBundle.getString('ImportFeedsCreateNewListItem'));
> 
> Double quotes please (while touching this).

Will do.
Geoff, can you please give us a hand replacing listbox in Calendar. We're already having a bad time doing it for the rest of C-C and landing of bug 1472555 is imminent with only one review missing.
Flags: needinfo?(geoff)
Posted patch listboxConvert.patch (obsolete) — Splinter Review
This only needs one review for the one that is faster.

This is the patch based on Jörg's version with the comments addressed.

Additionally I converted the sanitizer to a vbox with <checkbox> instead the <listitem>. There are only three elements in this box and a richlistbox makes no sense because this never overflows, it would also need a lot of JS changes and my solution needs only a XUL change and some CSS to look like a list. Jörg, the sanitizer height does increase when collapsing/uncollapsing the the list. This isn't a but introduced by this patch. Aceman already tried to fix this but until now without success.
Attachment #8992222 - Attachment is obsolete: true
Attachment #8992364 - Flags: review?(jorgk)
Attachment #8992364 - Flags: review?(acelists)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0d91ea96bfcaadbd010c6fba5fd50384f654373a

Sadly 25 left still:

Mailnews:
.\base\prefs\content\am-identities-list.xul: <listbox id="identitiesList"
.\base\search\content\CustomHeaders.xul: <listbox id="headerList" flex="1" onselect="updateRemoveButton();" />
.\base\search\content\FilterEditor.xul: <listbox id="filterActionList" flex="1" rows="4" minheight="35%"
.\base\search\content\searchTermOverlay.xul: <listbox flex="1" id="searchTermList" rows="4" minheight="35%">
.\extensions\smime\content\msgCompSecurityInfo.xul: <listbox id="infolist" flex="1"
.\import\content\fieldMapImport.xul: <listbox id="fieldList" flex="1" onselect="disableMoveButtons();">

Mail:
.\base\content\FilterListDialog.xul: <listbox id="filterList" flex="1" onselect="updateButtons();"
.\components\addrbook\content\abEditListDialog.xul: <listbox id="addressingWidget" style="height: 15em;" 
.\components\addrbook\content\abMailListDialog.xul: <listbox id="addressingWidget" style="height: 15em;" 
.\components\compose\content\messengercompose.xul: <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
.\components\im\content\imAccountWizard.xul: <listbox flex="1" id="protolist"
.\components\preferences\applicationManager.xul: <listbox id="appList" onselect="gAppManagerDialog.onSelect();" flex="1"/>
.\components\preferences\sendoptions.xul: <listbox id="html_domains" flex="1" seltype="multiple" rows="5"
.\components\preferences\sendoptions.xul: <listbox id="plaintext_domains" flex="1" seltype="multiple" rows="5"

Calendar:
.\base\content\dialogs\calendar-event-dialog-reminder.xul: <listbox id="reminder-listbox"
.\base\content\dialogs\calendar-migration-dialog.xul: <listbox id="datasource-list" flex="1">
.\base\content\dialogs\chooseCalendarDialog.xul: <listbox id="calendar-list" rows="5" flex="1" seltype="single">
.\base\content\preferences\categories.xul: <listbox id="categorieslist"
.\lightning\content\lightning-item-iframe.xul: <listbox id="attachment-link"
.\providers\gdata\content\gdata-migration-wizard.xul: <listbox id="calendars-listbox" flex="1"/>

Editor:
.\ui\dialogs\content\EdDictionary.xul: <listbox rows="8" id="DictionaryList" flex="1"/>
.\ui\dialogs\content\EditorPublishProgress.xul: <listbox id="FileList" rows="1"/>
.\ui\dialogs\content\EditorPublishSettings.xul: <listbox rows="4" id="SiteList" flex="1"  onselect="SelectSiteList();"/>
.\ui\dialogs\content\EdLinkChecker.xul: <listbox rows="8" id="LinksList" class="MinWidth20" flex="1"/>
.\ui\dialogs\content\EdSpellCheck.xul: <listbox  rows="6" id="SuggestedList" onselect="SelectSuggestedWord()"
Keywords: leave-open
(In reply to Jorg K (GMT+2) from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=0d91ea96bfcaadbd010c6fba5fd50384f654373a
> 
> Sadly 25 left still:
> .\base\search\content\searchTermOverlay.xul: <listbox flex="1"
> id="searchTermList" rows="4" minheight="35%">

This is SM only. It's also still an overlay.

> Editor:
> .\ui\dialogs\content\EdDictionary.xul: <listbox rows="8" id="DictionaryList"
> flex="1"/>
> .\ui\dialogs\content\EditorPublishProgress.xul: <listbox id="FileList"
> rows="1"/>
> .\ui\dialogs\content\EditorPublishSettings.xul: <listbox rows="4"
> id="SiteList" flex="1"  onselect="SelectSiteList();"/>
> .\ui\dialogs\content\EdLinkChecker.xul: <listbox rows="8" id="LinksList"
> class="MinWidth20" flex="1"/>
> .\ui\dialogs\content\EdSpellCheck.xul: <listbox  rows="6" id="SuggestedList"
> onselect="SelectSuggestedWord()"

If I'm not wrong this are only used in SM.
Comment on attachment 8992364 [details] [diff] [review]
listboxConvert.patch

Interdiff to my last version looks good, so let's see what the try says.
Attachment #8992364 - Flags: review?(jorgk) → review+
Hmm, not so successful:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/account/test-ab-whitelist.js | test-ab-whitelist.js::test_address_book_whitelist

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_openComposeFromMailToLink
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_checkInsertImage
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_closeComposeWindowAndTab

The first test is for the junk list in account settings, so something still wrong there, the second test failure is unexpected.

I'll look into these.

BTW, EdDictionary.xul and EdSpellCheck.xul are used in TB's spellcheck, no?
Yes, they are. These seems to be simple ones. I'll try to convert them.
This fixes the test. It was as simple at adding '.firstChild'. test-compose-mailto.js doesn't fail locally.
Attachment #8992364 - Attachment is obsolete: true
Attachment #8992364 - Flags: review?(acelists)
Attachment #8992416 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e6fa0c597846
Convert simple <listbox> to <richlistbox>. r=jorgk
Attachment #8992416 - Attachment description: listboxConvert.patch - JK v3 → listboxConvert.patch - JK v3 [landed in comment #18]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1d4e4734c0a0
Follow-up to part 1: remove unneeded CSS code. r=me DONTBUILD
Posted patch listboxConvert-Part2.patch (obsolete) — Splinter Review
This are some more listboxes.

EdDictionary.xul, am-identities-list.xul and CustomHeaders.xul should be okay.

EdSpellCheck.xul needs the JS fixed.

I'm looking now for more simple listboxes to convert.
(In reply to Richard Marti (:Paenglab) from comment #21)
> EdDictionary.xul
Hmm, the Replace button doesn't work. Also, if you first click replace, then Add, you get a replace action. Something wrong there.
Posted patch imAccountWizardConvert.patch (obsolete) — Splinter Review
This is the conversion for imAccountWizard. Maybe not super clean, but it works.
Attachment #8992445 - Flags: review?(jorgk)
Posted patch listboxConvert-Part2.patch (obsolete) — Splinter Review
Fighting with the edit dictionary function. Whatever I tried,
  selItem.firstChild.setAttribute("label", gWordToAdd);
does not update the display in the list :-( - Why now?
Attachment #8992426 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
Posted patch imAccountWizardConvert.patch (obsolete) — Splinter Review
Hopefully cleaner approach.
Attachment #8992445 - Attachment is obsolete: true
Attachment #8992445 - Flags: review?(jorgk)
Attachment #8992462 - Flags: review?(jorgk)
Posted patch listboxConvert-Part3.patch (obsolete) — Splinter Review
Silly me, the label's attribute is called "value" and not "label" :-(
Attachment #8992458 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
Thanks, that works, I reshuffled the element creation for greater clarity.
Attachment #8992462 - Attachment is obsolete: true
Attachment #8992462 - Flags: review?(jorgk)
Attachment #8992466 - Flags: review+
Posted patch listboxConvert-Part3.patch (obsolete) — Splinter Review
CustomHeaders.js needed changes.
Attachment #8992463 - Attachment is obsolete: true
Comment on attachment 8992466 [details] [diff] [review]
imAccountWizardConvert.patch [landed in comment 31]

Review of attachment 8992466 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but needs some testing.

::: mail/components/im/content/imAccountWizard.js
@@ +17,5 @@
>      var protoList = document.getElementById("protolist");
>      var protos = [];
>      for (let proto of this.getProtocols())
>        protos.push(proto);
>      protos.sort((a, b) => a.name < b.name ? -1 : a.name > b.name ? 1 : 0);

Oh the readability :) Also there surely is a function for plain string comparison nowadays.

@@ -20,5 @@
>        protos.push(proto);
>      protos.sort((a, b) => a.name < b.name ? -1 : a.name > b.name ? 1 : 0);
>      protos.forEach(function(proto) {
> -      var id = proto.id;
> -      var item = protoList.appendItem(proto.name, id, id);

A three argument version of appendItem? Is that a typo or is the function overloaded somewhere?
Attachment #8992466 - Flags: feedback+
That's a typo, but it's gone now. BTW, I tested it ;-)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/79f02a922940
Part 2: Convert the imAccountWizard to richlistbox. r=jorgk
Comment on attachment 8992473 [details] [diff] [review]
listboxConvert-Part3.patch

Review of attachment 8992473 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/dialogs/content/EdDictionary.js
@@ +91,2 @@
>        try {
>          gSpellChecker.RemoveWordFromDictionary(selItem.label);

There are multiple places in this file which may need to be changed to item.firstChild.label.value now.

::: editor/ui/dialogs/content/EdSpellCheck.xul
@@ +60,5 @@
>          <!-- BUG! setting class="MinWidth20em" on tree doesn't work (width=0) -->
> +        <richlistbox id="SuggestedList"
> +                     class="theme-listbox"
> +                     onselect="SelectSuggestedWord()"
> +                     ondblclick="if (gAllowSelectWord) Replace(event.target.getAttribute('label'));"/>

No height here? Also, does height attribute actually work, or should it be style="height: ..." ?

::: mailnews/base/prefs/content/am-identities-list.xul
@@ +46,5 @@
> +               ondblclick="onEdit(event);"
> +               onselect="updateButtons();"
> +               seltype="single"
> +               flex="1"
> +               style="height: 150px;"/>

Why 150px now? The 0px was propabbly intentional, maybe the flex won and filled the element into all the available height.

::: mailnews/base/search/content/CustomHeaders.js
@@ +170,5 @@
>  }
>  
>  function addRow(newHdr)
>  {
> +  return gHdrsList.appendItem(newHdr);

This needs testing what happens if you omit the second argument (value). Whether you get value="" or value="undefined" or something.
Attachment #8992473 - Flags: feedback+
(In reply to :aceman from comment #32)
> There are multiple places in this file which may need to be changed to
> item.firstChild.label.value now.
You mean item.firstChild.value? No, if you read the documentation, richlistitem has a .label attribute (which is readonly).

> > +  return gHdrsList.appendItem(newHdr);
> This needs testing what happens if you omit the second argument (value).
> Whether you get value="" or value="undefined" or something.
This uses "" explicitly:
https://dxr.mozilla.org/comm-central/rev/e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/editor/ui/dialogs/content/EdDictionary.js#153
(In reply to Jorg K (GMT+2) from comment #33)
> (In reply to :aceman from comment #32)
> > There are multiple places in this file which may need to be changed to
> > item.firstChild.label.value now.
> You mean item.firstChild.value? No, if you read the documentation,
> richlistitem has a .label attribute (which is readonly).

Not very nice, but ok.

> > > +  return gHdrsList.appendItem(newHdr);
> > This needs testing what happens if you omit the second argument (value).
> > Whether you get value="" or value="undefined" or something.
> This uses "" explicitly:
> https://dxr.mozilla.org/comm-central/rev/
> e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/editor/ui/dialogs/content/
> EdDictionary.js#153

That's better, but it also probably handles those empty values.
I got this working with the help of Aceman.

Richard, if you can do the XUL replacements, I'm happy to work on the JS part. Hopefully Aceman fixes his machine soon.
Attachment #8992473 - Attachment is obsolete: true
Attachment #8992484 - Flags: review+
Attachment #8992466 - Attachment description: imAccountWizardConvert.patch → imAccountWizardConvert.patch [landed in comment 31]
So remaining to do:

Editor: 3 not used by TB, the others are done in part 3.

Calendar: 6 to be covered by Geoff

Mailnews:
.\base\prefs\content\am-identities-list.xul: Part 3
.\base\search\content\CustomHeaders.xul: Part 3
open:
.\base\search\content\FilterEditor.xul: <listbox id="filterActionList" flex="1" rows="4" minheight="35%"
.\extensions\smime\content\msgCompSecurityInfo.xul: <listbox id="infolist" flex="1"
.\import\content\fieldMapImport.xul: <listbox id="fieldList" flex="1" onselect="disableMoveButtons();">

Mail:
.\base\content\FilterListDialog.xul: <listbox id="filterList" flex="1" onselect="updateButtons();"
.\components\addrbook\content\abEditListDialog.xul: <listbox id="addressingWidget" style="height: 15em;" 
.\components\addrbook\content\abMailListDialog.xul: <listbox id="addressingWidget" style="height: 15em;" 
.\components\compose\content\messengercompose.xul: <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
.\components\preferences\applicationManager.xul: <listbox id="appList" onselect="gAppManagerDialog.onSelect();" flex="1"/>
.\components\preferences\sendoptions.xul: <listbox id="html_domains" flex="1" seltype="multiple" rows="5"
.\components\preferences\sendoptions.xul: <listbox id="plaintext_domains" flex="1" seltype="multiple" rows="5"

Try with part 3:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=538180e60a1214b8307d9391e0800d3e4bd422e1

As I said, Richard, if you can do the XUL, that would be a help.
Richard, can you please address comment #32 so I can land part 3:

::: editor/ui/dialogs/content/EdSpellCheck.xul
No height here? Also, does height attribute actually work, or should it be style="height: ..." ?

::: mailnews/base/prefs/content/am-identities-list.xul
Why 150px now? The 0px was probably intentional, maybe the flex won and filled the element into all the available height.

BTW, I tested "Manage Identities" and "Customize Headers" (in filters), try is green, so this is also good to go.
(In reply to Jorg K (GMT+2) from comment #37)
> Richard, can you please address comment #32 so I can land part 3:
> 
> ::: editor/ui/dialogs/content/EdSpellCheck.xul
> No height here? Also, does height attribute actually work, or should it be
> style="height: ..." ?

It's not needed. The box gets its height automatically. Or have you seen a strange height with the patch?

> ::: mailnews/base/prefs/content/am-identities-list.xul
> Why 150px now? The 0px was probably intentional, maybe the flex won and
> filled the element into all the available height.

Before the 0px was overruled by the flex. You can try with removing the height and check it. Not sure but I think I've done this.
This clears five of the six listboxes in calendar. The sixth is in the GData migration code, which I think might be obsolete and worth removing. Am I wrong, Philipp?
Flags: needinfo?(geoff)
Attachment #8992560 - Flags: review?(philipp)
(In reply to Richard Marti (:Paenglab) from comment #38)
> Before the 0px was overruled by the flex. You can try with removing the
> height and check it.
Works without the height. Landing this now.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d0dc345d7a37
Part 3: Convert simple <listbox> to <richlistbox>. r=jorgk DONTBUILD
Comment on attachment 8992484 [details] [diff] [review]
listboxConvert-Part3.patch [landed in comment #41]

Landed with height removed in am-identities-list.xul.
Attachment #8992484 - Attachment description: listboxConvert-Part3.patch → listboxConvert-Part3.patch [landed in comment #41]
This does the migration. The JS code is copied from FF.

However, something doesn't work here, for "Application Details" I always get a list that is one entry long. Same for FF. Geoff, you made this work for TB in bug 1472615, so you may have some insights as to how this is meant to work.
Attachment #8992566 - Flags: review?(geoff)
Ignore that, the list isn't high enough, so one doesn't see the other entries :-(
Hmm, I can't seem to increase the height, I tried:
  <hbox flex="1" style="height: 500px;">
    <richlistbox id="appList"
                 onselect="gAppManagerDialog.onSelect();"
                 flex="1"
                 style="height: 500px;"/>
but the height doesn't help, neither on the hbox nor on the richlistbox. Sigh. So much time spent on those details :-(
Attachment #8992572 - Flags: review?(jorgk)
"min-height" works. One review will do.
Attachment #8992566 - Attachment is obsolete: true
Attachment #8992566 - Flags: review?(geoff)
Attachment #8992574 - Flags: review?(richard.marti)
Attachment #8992574 - Flags: review?(geoff)
Attachment #8992574 - Flags: review?(acelists)
Comment on attachment 8992574 [details] [diff] [review]
1475817-applicationManager.patch (v2) [landed in comment #50]

Review of attachment 8992574 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand the point of that bit about XBL, but if you stole it from mozilla-central, it must be good, right? ;-)
Attachment #8992574 - Flags: review?(richard.marti)
Attachment #8992574 - Flags: review?(geoff)
Attachment #8992574 - Flags: review?(acelists)
Attachment #8992574 - Flags: review+
Comment on attachment 8992572 [details] [diff] [review]
1475817-listbox-filters-1.diff

Review of attachment 8992572 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really not the best reviewer here for that XUL/CSS stiff. Richard, please check the CSS.

::: mail/base/content/FilterListDialog.js
@@ +305,4 @@
>  
>    // Now update the checkbox
> +  if (aForceValue === undefined) {
> +    aFilterItem.childNodes[1].setAttribute("checked", filter.enabled);

If you force it, you don't want to update the check box? Why?

Can you use .firstChild.nextSibling (or so)?
Attachment #8992572 - Flags: review?(richard.marti)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4e763866d182
Part 4: Replace listbox in applicationManager.xul/js. r=darktrojan DONTBUILD
Attachment #8992574 - Attachment description: 1475817-applicationManager.patch (v2) → 1475817-applicationManager.patch (v2) [landed in comment #50]
(In reply to Jorg K (GMT+2) from comment #49)
> If you force it, you don't want to update the check box? Why?

If we get here, we're in the middle of an event handler that's going to update the checkbox when we're done. Perhaps force is too strong a word, but it's what came to mind.

> Can you use .firstChild.nextSibling (or so)?

Yes, but I'm just using the code that was already there.
Comment on attachment 8992560 [details] [diff] [review]
1475817-listbox-calendar-1.diff [landed in comment #59]

Review of attachment 8992560 [details] [diff] [review]:
-----------------------------------------------------------------

The gdata migration code is still a thing, what it does is migrate ics calendars to calendars using the gdata provider. This is always going to be relevant, at least as long as Google provides ICS urls.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +2324,5 @@
>   */
>  function attachmentClick(aEvent) {
> +    let item = document.popupNode;
> +    while (item && item.localName != "richlistbox" && item.localName != "richlistitem") {
> +        item = item.parentNode;

You can use the shiny new document.popupNode.closest("richlistbox, richlistitem")
Attachment #8992560 - Flags: review?(philipp) → review+
Note that Geoff is working on addressing, see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b661bf0789e5b42ca87535c2d659224962afe209
https://hg.mozilla.org/try-comm-central/rev/bb7dbbbc6d4c4ee43a1386c060c73a55a0c9a54c

With that those patches applied we're left with:

Mail:
.\components\compose\content\messengercompose.xul: <listbox id="addressingWidget" flex="1" seltype="multiple" rows="1"
.\components\preferences\sendoptions.xul: <listbox id="html_domains" flex="1" seltype="multiple" rows="5"
.\components\preferences\sendoptions.xul: <listbox id="plaintext_domains" flex="1" seltype="multiple" rows="5"
Geoff mentioned that messengercompose.xul needs to go with the addressing changes.

Mailnews:
.\base\search\content\FilterEditor.xul:    <listbox id="filterActionList" flex="1" rows="4" minheight="35%"
(dead overlay) .\base\search\content\searchTermOverlay.xul:
.\extensions\smime\content\msgCompSecurityInfo.xul:  <listbox id="infolist" flex="1"
.\import\content\fieldMapImport.xul:    <listbox id="fieldList" flex="1" onselect="disableMoveButtons();">

Let's hope bug 1472555 doesn't land for the next 48 hours.
This works. I've done a few tweaks. The checkboxes don't line up under the headings, so perhaps Richard can fix that.

BTW, what's that change:
-      listitem = gFilterListbox.children[listitemIndex + 1];
+      listitem = gFilterListbox.children[listitemIndex];
Was that wrong to start with?
Attachment #8992572 - Attachment is obsolete: true
Attachment #8992572 - Flags: review?(richard.marti)
Attachment #8992572 - Flags: review?(jorgk)
Attachment #8992618 - Flags: review?(richard.marti)
Attachment #8992618 - Flags: review?(acelists)
Attachment #8992618 - Flags: feedback+
The CSS changes for Mac and Windows where missing -> added. I haven't found a solution to center the treecol. Instead of centering the checkmark, I gave a padding to center it a bit under the treecol. With other locales where the string is longer it's no more centered but it would look still better than when it would be left aligned. Geoff gave to this column a width of 100px to have enough space for locales with longer strings because the richlistbox is no more a grid and it doesn't fit to the length of the strings.

I also added the class="theme-listbox" to look better under Windows.
Attachment #8992618 - Attachment is obsolete: true
Attachment #8992618 - Flags: review?(richard.marti)
Attachment #8992618 - Flags: review?(acelists)
Attachment #8992701 - Flags: review?(jorgk)
We see movement in bug 1472555, so that will land soon. Here's a try run without Geoff's ambitious patch for addressing:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=53248cbf0f0c6fd77d6ba16cbca9eb5509252f17
Comment on attachment 8992701 [details] [diff] [review]
1475817-listbox-filters-1.diff [landed in comment #59]

It was never centred, it's always been left aligned like the filter name. Can you please do that.

Time's running out, so we can worry about details later.
Attachment #8992701 - Flags: review?(jorgk) → review+
Posted patch sendoptions.patch (obsolete) — Splinter Review
This changes the sendoptions to richlistbox. Adding a new domain works, also the duplicated domain works. Deleting a domain blocks TB. And also the domains aren't stored. Jörg, please can you take a look?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1e7bb39d56f3
Part 5: Convert simple <listbox> to <richlistbox> in calendar/. r=philipp
https://hg.mozilla.org/comm-central/rev/c038f663b745
Part 6: Convert simple <listbox> to <richlistbox>, message filters dialog. r=jorgk DONTBUILD
Attachment #8992560 - Attachment description: 1475817-listbox-calendar-1.diff → 1475817-listbox-calendar-1.diff [landed in comment #59]
Attachment #8992701 - Attachment description: 1475817-listbox-filters-1.diff → 1475817-listbox-filters-1.diff [landed in comment #59]
Thanks, that works for me. Let's see whether any tests need updating:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=208f5677532ef352a18bfd3c8a12bc0b86e5a234
Attachment #8992724 - Attachment is obsolete: true
Attachment #8992737 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2501e7eabc9f
Part 7: Convert simple <listbox> to <richlistbox>, sendoptions.xul/js. r=jorgk DONTBUILD
Attachment #8992737 - Attachment description: sendoptions.patch → sendoptions.patch [landed in comment #61]
Posted patch listboxConvert-Part4.patch (obsolete) — Splinter Review
This converts the FilterEditor and msgCompSecurityInfo.

msgCompSecurityInfo is complete and works. Only the columns don't fit to the text of the richlistitem. You can try to fix this or I'll do this later.

The FilterEditor is XUL only. The XUL has the treecols in it only as reference. When the JS works they need to be removed.

On the different childs of the richlistitem the flex/class needs to be taken from the corresponding treecol similar how I've done it in msgCompSecurityInfo.
Posted patch listboxConvert-SMIME.patch (obsolete) — Splinter Review
I removed the filter editor hunk.
Attachment #8992747 - Attachment is obsolete: true
Comment on attachment 8992759 [details] [diff] [review]
listboxConvert-SMIME.patch

Review of attachment 8992759 [details] [diff] [review]:
-----------------------------------------------------------------

I reshuffled the element creating a bit. Sadly I can't even work out how to trigger the UI for this. I thought it's the "Message Security" panel, but I can't see a list there.

::: mailnews/extensions/smime/content/msgCompSecurityInfo.xul
@@ -56,5 @@
> -    <listhead>
> -      <listheader label="&tree.recipient;"/>
> -      <listheader label="&tree.status;"/>
> -      <listheader label="&tree.issuedDate;"/>
> -      <listheader label="&tree.expiresDate;"/>

Did you forget to move the labels to the new scheme?
I tinkered with this a bit, but it goes into searchWidgets.xml and bindings :-( - I'll have to call it a day. Maybe Aceman can fix this one.
Attachment #8992761 - Attachment is obsolete: true
Posted patch listboxConvert-SMIME.patch (obsolete) — Splinter Review
Somehow my last changes didn't go into the patch. Now the treecols should be there. But still with the aligning error.
Attachment #8992759 - Attachment is obsolete: true
Attachment #8992772 - Flags: review?(jorgk)
Hmm, but what do I do to see this in the UI?
Write a message, sign it and then click on the Security button.
Comment on attachment 8992772 [details] [diff] [review]
listboxConvert-SMIME.patch

Yes, this works, but as mentioned, the alignment isn't right. If I get desperate, I'll land it, but I'll wait for now.
Attachment #8992772 - Flags: feedback+
This at least restores the filter rule display.
But there is still:
JavaScript error: chrome://messenger/content/searchWidgets.xml, line 554: TypeError: this.mListBox.getItemAtIndex(...) is null

The listbox thinks there are 0 items, when though the items are properly displayed and are not zero. This is the action list.
Attachment #8992767 - Attachment is obsolete: true
Comment on attachment 8992785 [details] [diff] [review]
listboxConvert-FilterEd.patch - WIP 2

Review of attachment 8992785 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/content/searchTermOverlay.xul
@@ +26,5 @@
>      </radiogroup>
>  
>      <hbox flex="1">
>        <hbox id="searchterms"/>
> +      <richlistbox flex="1" id="searchTermList" rows="4" minheight="35%">

Changes in this file may not be required since it's an overlay not used in TB any more. Maybe we should remove the entire file.
This is the new/edit mailing list dialogs and the address fields in the compose window.
Attachment #8992829 - Flags: review?(jorgk)
This is the remaining listbox in calendar.
Attachment #8992830 - Flags: review?(philipp)
Attachment #8992830 - Flags: review?(philipp) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d70436bc01de
Part 8: Convert simple <listbox> to <richlistbox> in calendar/providers/gdata. r=philipp
Attachment #8992830 - Attachment description: 1475817-listbox-gdata-1.diff → 1475817-listbox-gdata-1.diff [landed in comment #75]
Posted image 1475817.png - addressing (obsolete) —
Thanks a lot, Geoff, this includes messengercompose.xul :-)

I didn't see a try run (and the last one wasn't so successful), so here's one now:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=00d3a935a8e32dae3b1261faefc22efeee741313

Looking at the patch locally, I see a graphic anomaly in the compose window.
Posted patch filterEdRich.patch - WIP 2b (obsolete) — Splinter Review
Removed changes in searchTermOverlay.xul as per comment #72. Unchanged:
JavaScript error: chrome://messenger/content/searchWidgets.xml, line 553: TypeError: this.mListBox.getItemAtIndex(...) is null

In summary:
Addressing and messengercompose.xul - covered by Geoff's patch, graphical nits remaining.
SMIME - covered by Richard's patch, graphical nits remaining.
Filter edit: WIP
Import list (mailnews/import/content/fieldMapImport.xul): Hopefully Richard can start with the XUL.
Attachment #8992785 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #77)
> Addressing and messengercompose.xul - covered by Geoff's patch, graphical
> nits remaining.
Try is green, so this works. Richard, could you see where the graphics go wrong? Geoff is on Linux, so it's harder for him to do in a VM.
It's okay, I know what's wrong. I forgot to copy the hidden="true" attribute into the tree version of messengercompose.xul.
Fixed, and so is another thing I missed because the file is preprocessed and I fixed the copy in objdir/dist/bin then copied back.
Attachment #8992829 - Attachment is obsolete: true
Attachment #8992829 - Flags: review?(jorgk)
Attachment #8992877 - Flags: review?(jorgk)
Posted patch 1475817-listbox-import-1.diff (obsolete) — Splinter Review
This patch is for the field-matching part of the import process. Anybody, feel free to take this over and finish it. I think I have a good approach here to keeping columns lined up. (Yes, I know .css files shouldn't be where I put it, but it's unfinished.)
Comment on attachment 8992877 [details] [diff] [review]
1475817-listbox-addressing-2.diff [landed in comment #83]

Many thanks. This works now and fixes bug 1457085 as well. As commented there, do we need to backport this entire patch or can it only be the messengercompose.xul part?
Attachment #8992877 - Flags: review?(jorgk) → review+
Attachment #8992861 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7962d601822d
Part 9: Convert simple <listbox> to <richlistbox>, address list dialogs. r=jorgk DONTBUILD
Attachment #8992877 - Attachment description: 1475817-listbox-addressing-2.diff → 1475817-listbox-addressing-2.diff [landed in comment #83]
If it causes no problems, and I'm not totally convinced of that, it could be backported, but the whole lot would be needed, as for some weird reason this code is shared with the mailing list UI code.
(In reply to Geoff Lankow (:darktrojan) from comment #81)
> This patch is for the field-matching part of the import process. Anybody,
> feel free to take this over and finish it.
Can you please add fieldMapImport.css to your patch :-(
Posted patch 1475817-listbox-import-2.diff (obsolete) — Splinter Review
That would be helpful, wouldn't it? :(
Attachment #8992880 - Attachment is obsolete: true
(In reply to Geoff Lankow (:darktrojan) from comment #84)
> If it causes no problems, and I'm not totally convinced of that, it could be
> backported, but the whole lot would be needed, as for some weird reason this
> code is shared with the mailing list UI code.
The patch applies to TB 60, so I started a try, see bug 1457085 comment #18. It's the only chance to fix that annoying regression.
Only XUL conversion of fieldMapImport. Needs JS changes and somewhere a CSS rule for 

#fieldList treecolpicker {
  display: none;
}

Not yet tested where we can put it. Maybe in messenger.css.
This is Geoff's patch with changed CSS indentation from tab to spaces. I removed one rule which made the aligning bad. Now all is aligned. Tested on all platforms.
Attachment #8992882 - Attachment is obsolete: true
Attachment #8993035 - Flags: review?(jorgk)
Thanks to Geoff's patch I could follow with aligning the columns. What would I do without him.

Only one specialty of Windows: the richlistitems have a border and a margin. This makes, when using the width of the treecols, the item wider than the box, which makes show a horizontal scrollbar. I fixed this by making the last label 5px smaller. The fieldMapImport doesn't wave this issue, probably because on Windows this dialog isn't resizeable.
Attachment #8992772 - Attachment is obsolete: true
Attachment #8992772 - Flags: review?(jorgk)
Attachment #8993042 - Flags: review?(jorgk)
Attachment #8992920 - Attachment is obsolete: true
Comment on attachment 8993042 [details] [diff] [review]
listboxConvert-SMIME.patch [landed in comment #94]

Thanks, this mostly works. There are still columns that don't line up, I'll attach a screenshot. We might need to follow up since the M-C changes will merge soon.
Attachment #8993042 - Flags: review?(jorgk) → review+
Comment on attachment 8993035 [details] [diff] [review]
1475817-listbox-import-2.diff [landed in comment #94]

This works nicely. I'm only wondering how Geoff managed to fix that while the mapping panel was actually broken, see bug 1476520.
Attachment #8993035 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/51798cb0d062
Part 10: Convert simple <listbox> to <richlistbox>, msgCompSecurityInfo. r=jorgk
https://hg.mozilla.org/comm-central/rev/a7d0257d9621
Part 11: Convert simple <listbox> to <richlistbox> in mailnews/import. r=Paenglab,jorgk
Attachment #8993035 - Attachment description: 1475817-listbox-import-2.diff → 1475817-listbox-import-2.diff [landed in comment #94]
Attachment #8993042 - Attachment description: listboxConvert-SMIME.patch → listboxConvert-SMIME.patch [landed in comment #94]
Posted patch filterEdRich.patch - WIP 3 (obsolete) — Splinter Review
This now looks better, columns align like the old editor.
But the errors about getRowCount() still exist.
Attachment #8992869 - Attachment is obsolete: true
With bug 1472555 now landed, we're seeing Mozmill test failures, at least:
search-window/test-search-window.js
folder-display/test-mail-views.js
folder-widget/test-message-filters.js

The patch actually fixes the first two, test-message-filters.js still fails as can be expected.
Comment on attachment 8993175 [details] [diff] [review]
filterEdRich.patch - WIP 3

Review of attachment 8993175 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/content/searchTermOverlay.js
@@ +433,3 @@
>      listitem.setAttribute("allowevents", "true");
>      for (let i = 0; i < aChildren.length; i++) {
>        let listcell = document.createElement("listcell");

This needs to change since 'listcell' is dead:
https://hg.mozilla.org/mozilla-central/rev/38ebcdae9bca#l39.13
(In reply to Jorg K (GMT+2) from comment #96)
> The patch actually fixes the first two, test-message-filters.js still fails
> as can be expected.
Spoke too early, they only passed without having applied bug 1472555 locally :-( - So no point landing the filterEdRich.patch since it buys us nothing.
Patch to disable failing tests in order to allow for efficient sheriffing.
Posted patch 1475817-chat.patch - WIP (obsolete) — Splinter Review
Doesn't work at all, but it documents that chat is also still missing.

Looking for #listitem, I found
mail\base\content\mailWidgets.xml: <binding id="attachmentitem" extends="chrome://global/content/bindings/listbox.xml#listitem">

mail\base\content\messenger.css: -moz-binding: url("chrome://messenger/content/searchWidgets.xml#listitem");

So I guess attachment lists are broken as well :-( - Richard?

Geoff, you had enough of listboxes, but perhaps you can help some more here.
Flags: needinfo?(geoff)
Posted patch 1475817-chat.patch (obsolete) — Splinter Review
I am a bit further. My problem is that the buddy name and icon aren't applied.
Attachment #8992830 - Attachment is obsolete: true
Oh, there is also
https://dxr.mozilla.org/comm-central/rev/e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/common/bindings/preferences.xml#486
aElement.localName == "listitem")

I was working on chat as well, but I'll stop now. Please use this: mail/components/im/content/imconversation.xml

          this.trackNick(name);
 
-         var item = document.createElement("listitem");
+         var label = document.createElement("label");
+         label.setAttribute("value", name);
+         var item = document.createElement("richlistitem");
+         item.appendChild(label);
          item.chatBuddy = aBuddy;
-         item.setAttribute("class", "listitem-iconic");
-         item.setAttribute("label", name);
+         // item.setAttribute("class", "listitem-iconic");  // XXX Todo.
          this.setBuddyAttributes(item);
(In reply to Jorg K (GMT+2) from comment #102)
> I was working on chat as well, but I'll stop now. Please use this:
> mail/components/im/content/imconversation.xml
Ignore that, you've already added it.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a34f9a5e167b
temporarily disable failing tests. rs=bustage-fix
Posted patch 1475817-chat.patch - WIP (obsolete) — Splinter Review
Image and label now working. 
Problems:
- The tooltip show only on empty space of richlistitem.
- Mouse cursor shows always background activity.
- The nicklist expands always to full height.
Attachment #8993287 - Attachment is obsolete: true
Attachment #8993288 - Attachment is obsolete: true
Attachment #8993290 - Flags: feedback?(geoff)
(In reply to Richard Marti (:Paenglab) from comment #105)
> - Mouse cursor shows always background activity.

This was something different. Shows no more.
Posted patch 1475817-chat.patch (obsolete) — Splinter Review
Works for me. I reshuffled some lines and used firstChild instead of children[0].

Is there anything not working or is that ready for review?

I don't want to sound like a broken record, but we still have:
mail\base\content\mailWidgets.xml: <binding id="attachmentitem" extends="chrome://global/content/bindings/listbox.xml#listitem">
mail\base\content\messenger.css: -moz-binding: url("chrome://messenger/content/searchWidgets.xml#listitem");
common\bindings\preferences.xml: 4 times.
Attachment #8993290 - Attachment is obsolete: true
Attachment #8993290 - Flags: feedback?(geoff)
Attachment #8993323 - Flags: feedback+
(In reply to Jorg K (GMT+2) from comment #107)
> Is there anything not working or is that ready for review?
Yes, problems listed in comment #105:
- The tooltip show only on empty space of richlistitem.
- The nicklist expands always to full height.
Posted patch 1475817-chat.patch (obsolete) — Splinter Review
Based on your reshuffling firstchild patch with a workaround for the previousConversations height. I've set a  style="min-height: 200px;" to make it show.

When you are okay with this, only the tooltip issue remains.
Attachment #8993323 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #107)
> mail\base\content\messenger.css: -moz-binding:
> url("chrome://messenger/content/searchWidgets.xml#listitem");

This should be no problem as it's a binding in searchWidgets.xml with the same name.

> common\bindings\preferences.xml: 4 times.

Three of them should be removable without problem.
This removes the "listitem" check in preferences.xml.
Attachment #8993328 - Flags: review?(jorgk)
(In reply to Richard Marti (:Paenglab) from comment #110)
> This should be no problem as it's a binding in searchWidgets.xml with the
> same name.
Yes, it will be a problem, see the searchWidgets.xml part of the "filterEdRich" patch. I'll add it there.

> Three of them should be removable without problem.
And the 4th one?
  <binding id="panebutton" role="xul:listitem"
           extends="chrome://global/content/bindings/radio.xml#radio">
And this:
mail\base\content\mailWidgets.xml: <binding id="attachmentitem" extends="chrome://global/content/bindings/listbox.xml#listitem">
Posted patch filterEdRich2.patch - WIP 3b (obsolete) — Splinter Review
One hunk more:
-#searchTermList > listitem {
-  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#listitem");
+#searchTermList > richlistitem {
+  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#richlistitem");
Attachment #8993175 - Attachment is obsolete: true
Changed the role from "xul:listitem" to "listitem".

This seems to still exist, see https://searchfox.org/comm-central/source/mozilla/accessible/tests/mochitest/attributes/test_obj_group.html#291
Attachment #8993328 - Attachment is obsolete: true
Attachment #8993328 - Flags: review?(jorgk)
Attachment #8993360 - Flags: review?(jorgk)
Comment on attachment 8993360 [details] [diff] [review]
preferences-remove-listitem.patch

Review of attachment 8993360 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Yes, 'role' is related to accessibility and it won't do any harm. I wonder why we're checking for listitem/listbox in this code when it appears to work without it.

I'm OK with the changes if you address the comment, but Aceman should take a look as well since he's an expert in the field.

::: common/bindings/preferences.xml
@@ -561,1 @@
>            case "listbox":

listbox can go, too.
Attachment #8993360 - Flags: review?(jorgk) → review+
listbox removed too.

Aceman, does this look reasonable?
Attachment #8993360 - Attachment is obsolete: true
Attachment #8993408 - Flags: review+
Attachment #8993408 - Flags: feedback?(acelists)
Comment on attachment 8993408 [details] [diff] [review]
preferences-remove-listitem.patch [landed in comment #119]

Review of attachment 8993408 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this looks ok, if we have no listitem elements in the preferences panes. And we can't as those are not supported anymore. And richlistitems do not support 'checked' attribute automatically so we would put it on a <checkbox> as done in the junk panel.

I'm no expert on the 'role'. You assume we can keep it at 'listitem' as accessibility apps know what that is and behave accordingly? And our element behaves as a listitem.
Attachment #8993408 - Flags: feedback?(acelists) → feedback+
Comment on attachment 8993408 [details] [diff] [review]
preferences-remove-listitem.patch [landed in comment #119]

I don't want to spoil the fun here, but the applications list (incoming attachments) looks different now. A single item is always selected, and margins and such have changed. Has that also changed in FF?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e5ce1a2de5ef
Part 12: Remove the "listitem"/"listbox" check in preferences.xml. r=jorgk
Fixed the tooltip issue.
Attachment #8993326 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #118)
> Comment on attachment 8993408 [details] [diff] [review]
> preferences-remove-listitem.patch
> 
> I don't want to spoil the fun here, but the applications list (incoming
> attachments) looks different now. A single item is always selected, and
> margins and such have changed. Has that also changed in FF?

I'll fix this.
Also, we might want to add these again, no?

--- a/common/bindings/preferences.xml
+++ b/common/bindings/preferences.xml
@@ -550,16 +550,18 @@
         <parameter name="aElement"/>
         <body>
         <![CDATA[
           switch (aElement.localName) {
           case "checkbox":
           case "colorpicker":
           case "radiogroup":
           case "textbox":
+          case "richlistitem":
+          case "richlistbox":

This is in method name="isElementEditable".
Attachment #8993408 - Attachment description: preferences-remove-listitem.patch → preferences-remove-listitem.patch [landed in comment #119]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5dbe1e828a03
Follow-up to part 12: Add richlistitem/richlistbox back to method isElementEditable. r=me
https://hg.mozilla.org/comm-central/rev/02051239a1bd
Part 13: Convert simple <listbox> to <richlistbox>, chat. r=jorgk DONTBUILD
Comment on attachment 8993457 [details] [diff] [review]
1475817-chat.patch [landed in comment #124]

Thanks, working for now. The 200px for the history is not ideal since it collapses to nothing in a jump.
Attachment #8993457 - Attachment description: 1475817-chat.patch → 1475817-chat.patch [landed in comment #124]
Attachment #8993457 - Flags: review+
Attachment #8993472 - Attachment description: 1475817-prefs-follow-up.patch → 1475817-prefs-follow-up.patch [landed in comment #124]
Attachment #8993472 - Flags: review+
This finally works for me without errors. The way we overrode the listitem binding to suppress the click/select/fireevent methods had to be changed due to the new composition of richlistitem and listitem (that still exists in m-c and richlistitem extends from it).

I also changed listcell to hbox. Now the + and - buttons a a bit bigger than they were and than the other menulists inside the row. That may need some padding to add.

Also, in action list the columns of the action/value/buttons are no strictly kept/aligned. That may need some playing with, but does not affect functionality.

Please run the tests with it.
Attachment #8993337 - Attachment is obsolete: true
Attachment #8993529 - Flags: ui-review?(richard.marti)
Attachment #8993529 - Flags: review?(jorgk)
After the conversion in the junk settings of account manager, even when the junk classification was disabled, you could still check the boxes before the addressbooks in the new richlistbox. This should fix it.
Attachment #8993532 - Flags: review?(jorgk)
Geoff, we're finally seeing the end of the tunnel here. Richard asked for your help regarding this:
https://hg.mozilla.org/comm-central/rev/02051239a1bd#l2.25
The min-height isn't very elegant. Is there a better way?
Comment on attachment 8993532 [details] [diff] [review]
fix junk settings checkboxes [landed in comment #131]

Works for me, thanks.
Attachment #8993532 - Flags: review?(jorgk) → review+
Comment on attachment 8993529 [details] [diff] [review]
filter editor.patch - 4 [landed in comment #131]

Yes, all works now. Richard will need to fix anything he doesn't like in a follow-up (as we will need for the S/MIME columns and the application list).
Attachment #8993529 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/aa5d645f1d72
Follow-up to part 1: fix disabling of checkboxes in junk settings pane of AM after conversion to richlistbox. r=jorgk
https://hg.mozilla.org/comm-central/rev/7398a3a9f02d
Backed out changeset a34f9a5e167b to re-enable tests. a=backout
https://hg.mozilla.org/comm-central/rev/a494924db574
Part 14: Convert simple <listbox> to <richlistbox>, search and filter editor. r=jorgk
Attachment #8993266 - Attachment description: 1475817-disable.patch → 1475817-disable.patch [landed in comment #104 and backed out in comment #131]
Attachment #8993529 - Attachment description: filter editor.patch - 4 → filter editor.patch - 4 [landed in comment #131]
Attachment #8993532 - Attachment description: fix junk settings checkboxes → fix junk settings checkboxes [landed in comment #131]
Summarising open issues:
- S/MIME columns not always lining up, comment #92.
- Something wrong with application list, comment #118.
- min-height in chat not optimal, comment #128.
- maybe Richard doesn't like the filter edit now, comment #126.

Finally, according to expert Aceman, this
mail\base\content\mailWidgets.xml: <binding id="attachmentitem" extends="chrome://global/content/bindings/listbox.xml#listitem">
is not an issue. M-C's listbox.xml still exists as it still is the base for richlistbox. I was also the base for listbox. listitem wasn't removed, only the stuff surrounding it:
https://hg.mozilla.org/mozilla-central/rev/501544bfc95b#l3.287
Yes, the min-height is ugly, but it does exactly what it says it does. I can't think of any way around it.
Flags: needinfo?(geoff)
See Also: → 1476947
Comment on attachment 8993529 [details] [diff] [review]
filter editor.patch - 4 [landed in comment #131]

Looks good. very similar to the previous listitem version. :)

The "+" and "-" buttons needs some padding and the new filter rule does no more flash. This can be done in follow-up bugs.
Attachment #8993529 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Richard Marti (:Paenglab) from comment #134)
> This can be done in follow-up bugs.
Or here since we still need to fix some minor issues anyway, see comment #132.
This lets crop the labels when the dialog is smaller. I don't crop the notFound because then the other three fields aren't used and it has more space. Also is it better to let the user always show that no certificate was found.
Attachment #8993767 - Flags: review?(jorgk)
Comment on attachment 8993767 [details] [diff] [review]
msgCompSecurityInfo-crop.patch [landed in comment #138]

Nice!
Attachment #8993767 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95d4e3a3652b
Follow-up to part 10: Crop the labels in msgCompSecurityInfo. r=jorgk
Posted patch FilterBox-CSS.patch (obsolete) — Splinter Review
This equalizes the height of the elements. Fixed the "new rule animation". Also the spinbuttons for the age and size textbox are working again.
Attachment #8993902 - Flags: review?(acelists)
Richard, can you please take a look at part 9: https://hg.mozilla.org/comm-central/rev/7962d601822d

Mostly listbox and listboxbody changed to richlistbox, but we still have CSS referring to listboxbody:
https://searchfox.org/comm-central/search?q=listboxbody&case=false&regexp=false&path=

I'm going to uplift part 9 to TB 60 ESR to fix bug 1457085, so I need to be sure that this is correct. I can't see any malfunction, but |#addressingWidget listboxbody {| seems wrong.
Flags: needinfo?(richard.marti)
Looking at this a bit more:
https://searchfox.org/comm-central/rev/71e50601fecb14eb2ae9fc8996ed2007d8df893f/calendar/base/themes/common/dialogs/calendar-event-dialog.css#576
.attendees-list-listbox > listboxbody {
seems to be a no-op since there is no attendees-list-listbox in the entire system.

However:
https://searchfox.org/comm-central/rev/71e50601fecb14eb2ae9fc8996ed2007d8df893f/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml#1101
(target.localName != "listboxbody" &&
would need to be converted, no?

I guess that's not working right now when you select attendees?
Flags: needinfo?(geoff)
I added two calendar reviewers. Richard, can you please check the CSS changes.
Flags: needinfo?(geoff)
Attachment #8993911 - Flags: review?(philipp)
Attachment #8993911 - Flags: review?(geoff)
Attachment #8993911 - Flags: feedback?(richard.marti)
(In reply to Jorg K (GMT+2) from comment #140)
> Richard, can you please take a look at part 9:
> https://hg.mozilla.org/comm-central/rev/7962d601822d
> 
> Mostly listbox and listboxbody changed to richlistbox, but we still have CSS
> referring to listboxbody:
> https://searchfox.org/comm-central/
> search?q=listboxbody&case=false&regexp=false&path=
> 
> I'm going to uplift part 9 to TB 60 ESR to fix bug 1457085, so I need to be
> sure that this is correct. I can't see any malfunction, but
> |#addressingWidget listboxbody {| seems wrong.

Yes, this is no more needed. My patch removes this and aligns the textboxes on the right (on the left they are). This patch should be uplifted together with part 9.
Flags: needinfo?(richard.marti)
Attachment #8993912 - Flags: review?(jorgk)
Comment on attachment 8993911 [details] [diff] [review]
1475817-follow-up-part-9.patch

The removal of .attendees-list-listbox > listboxbody is reasonable as no such element with this class exists. I can't check the disabled richlistbox as I see no attendees tab in the event dialog. testing this on a other richlistbox shows no difference, so it's maybe no more needed.
Attachment #8993911 - Flags: feedback?(richard.marti) → feedback+
Comment on attachment 8993911 [details] [diff] [review]
1475817-follow-up-part-9.patch

Review of attachment 8993911 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't have done this in a hurry. I need to come back to this. IIRC Geoff mentioned that attendees aren't working since they also use listbox. I just wanted to identify the part that relates to the addressing widget that changed in part 9 and would require uplift.
Attachment #8993911 - Flags: review?(philipp)
Attachment #8993911 - Flags: review?(geoff)
Yes, attendees don't work at all and Geoff will take care of that in bug 1476947. As far as I can see, attendees work with part 9 uplifted to TB 60 ESR. Richard, can you confirm:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3db82378948ee7bb1ce056117b4d81b40520143
That's how I found bug 1477503.

So the patch was basically nonsense apart from two hunks which I'll attach now.
Attachment #8993911 - Attachment is obsolete: true
Attachment #8993933 - Flags: review?(acelists)
Comment on attachment 8993912 [details] [diff] [review]
composerBoxAlign.patch [landed in comment #164]

Yes, the removals look good, "background-color: transparent" isn't needed any more? I haven't tried, but wouldn't it be hard to see a 1px difference for the "padding-inline-end: 1px"?
Attachment #8993912 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #149)
> Comment on attachment 8993912 [details] [diff] [review]
> composerBoxAlign.patch
> 
> Yes, the removals look good, "background-color: transparent" isn't needed
> any more? I haven't tried, but wouldn't it be hard to see a 1px difference
> for the "padding-inline-end: 1px"?

The "background-color: transparent" is set on #addressingWidget which has no child that has set a background colour.

The 1px is visible when one textbox is selected and the other, above or below the selected one, is hovered. And if we are here, why don't do it correct?
Comment on attachment 8993902 [details] [diff] [review]
FilterBox-CSS.patch

Review of attachment 8993902 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks much better now.
The rows of search terms and actions are of different heights. In the past they seemed the same to me.
Also if you wanted you could add the "new row" highlight to the actions too, here or in a new bug.
Attachment #8993902 - Flags: review?(acelists) → review+
Comment on attachment 8993933 [details] [diff] [review]
1475817-follow-up-part-9.patch (v2) [landed in comment #156]

Review of attachment 8993933 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, thanks.
Attachment #8993933 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #151)
> Comment on attachment 8993902 [details] [diff] [review]
> FilterBox-CSS.patch
> 
> Review of attachment 8993902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks much better now.
> The rows of search terms and actions are of different heights. In the past
> they seemed the same to me.
> Also if you wanted you could add the "new row" highlight to the actions too,
> here or in a new bug.

Hmm, here under Ubuntu they are the same height. It can be because different Linux themes use different height of the textbox.
This is about the composerBoxAlign patch.

I looked at it with the magnifier to be pixel-perfect. The screenshot shows the From field, a selected To field and an unselected one.

The line under the From field is two pixels too short. The patch makes it one pixel too short, and it's right when hovered.

The line under the unselected field is one pixel to short and OK when hovered.

If you look on the left, the lines are all long enough.
Looking at FilterBox-CSS.patch on Windows 10, it looks better than in TB 60. All the boxes are the same height now, and the [+] and [-] box are two pixels smaller, one at the top and one at the bottom. We could make them the exact same height, I suppose.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/501f617f5d6f
Follow-up to part 9 (addressing): Remove reference to richlistcell. r=aceman
See bug 1477478 for the attendees dialog.
Depends on: 1476947, 1477478
I've noticed a bad problem related to part 9: awClickEmptySpace() (two copies, one in mail one in mailnews for mailing lists) stopped working.

So you can get yourself into the situation where you have one recipient, and you can't add another one (unless you click onto the first one and hit enter).

STR:
Enter a recipient until an autocomplete suggestions comes up. Now click on the body.
Result: To: with one recipient. Now click onto the empty row below. Nothing happens.
What should happen is that a new row should open with a To:.
Same problem in mailing lists.
Attachment #8993933 - Attachment description: 1475817-follow-up-part-9.patch (v2) → 1475817-follow-up-part-9.patch (v2) [landed in comment #156]
Attachment #8993767 - Attachment description: msgCompSecurityInfo-crop.patch → msgCompSecurityInfo-crop.patch [landed in comment #138]
For adding members to a mailing list, the STR are similar.
Start typing, when autocomplete results show, click on list name field.
Then click on the empty entry below the one just entered.

The hbox obviously comes from this change in part 9:
+            <richlistitem class="addressingWidgetItem" allowevents="true">
+              <hbox class="addressingWidgetCell" align="center" style="&headersSpace.style;">
Attachment #8993998 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #154)
> Created attachment 8993960 [details]
> 1475817-addressing-lines.png
> 
> This is about the composerBoxAlign patch.
> 
> I looked at it with the magnifier to be pixel-perfect. The screenshot shows
> the From field, a selected To field and an unselected one.
> 
> The line under the From field is two pixels too short. The patch makes it
> one pixel too short, and it's right when hovered.
> 
> The line under the unselected field is one pixel to short and OK when
> hovered.

This is the expected behavior. When hovered/focused the right 1px border is shown, when not the border is hidden (this is how it is also on TB 60). And this is only fixable with border-image magic I could do, but then in a follow up bug.
(In reply to Richard Marti (:Paenglab) from comment #160)
> This is the expected behavior. When hovered/focused the right 1px border is
> shown, when not the border is hidden (this is how it is also on TB 60). And
> this is only fixable with border-image magic I could do, but then in a
> follow up bug.
Hmm. It's different on the left. There, when hovered, the border is shown, but it doesn't get wider.

So left:

Unhovered:
=======

=======

Hovered:
=======
|
=======

Right:

Unhovered:
=======

=======

Hovered:
=======|
       |
=======|

We can go with the simple fix here, I just wanted to point out that it's not consistent. And you were the one who wanted to do it correctly ;-)
I had an exchange with Richard via PM. He explained to me that what we see comes from the anti-clock-wise draw-order of the edges. So the lower border is drawn and then the right border is drawn erasing the right-most pixel of the lower border. The picture in comment #161 isn't correct since in the unhovered state the upper border isn't drawn.

So attachment 8993912 [details] [diff] [review] is good to go ;-).
Comment on attachment 8993998 [details] [diff] [review]
1475817-awClickEmptySpace.patch [landed in comment #164]

Review of attachment 8993998 [details] [diff] [review]:
-----------------------------------------------------------------

Yes ;)
Attachment #8993998 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/325241c16434
Follow-up to part 9: Align the composer header textboxes. r=jorgk
https://hg.mozilla.org/comm-central/rev/d1371b7019f3
Follow-up to part 9: Make awClickEmptySpace() work again. r=aceman
Attachment #8993998 - Attachment description: 1475817-awClickEmptySpace.patch → 1475817-awClickEmptySpace.patch [landed in comment #164]
Comment on attachment 8993912 [details] [diff] [review]
composerBoxAlign.patch [landed in comment #164]

One patch to go for the filters and we'll be done here :-)
Attachment #8993912 - Attachment description: composerBoxAlign.patch → composerBoxAlign.patch [landed in comment #164]
Keywords: leave-open
Posted patch FilterBox-CSS.patch (obsolete) — Splinter Review
This fixes the button height under Windows. Before the inspector showed the same height as the other elements but they are painted two pixels smaller. Now they are painted the same height, also under Win 7.
Attachment #8993902 - Attachment is obsolete: true
Attachment #8994045 - Flags: review?(jorgk)
Comment on attachment 8994045 [details] [diff] [review]
FilterBox-CSS.patch

You attached the same patch again :-(
Attachment #8994045 - Flags: review?(jorgk)
Sorry, this time the right one.
Attachment #8994045 - Attachment is obsolete: true
Attachment #8994048 - Flags: review?(jorgk)
Comment on attachment 8994048 [details] [diff] [review]
FilterBox-CSS.patch [landed in comment #170]

Pixel-perfect!
Attachment #8994048 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b3ebe0396e3d
Part 15: Fix the Search/Filter dialog CSS. r=aceman,jorgk DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Those who want to see more listbox action, please follow bug 1476947 or bug 1477478.
Target Milestone: --- → Thunderbird 63.0
Attachment #8994048 - Attachment description: FilterBox-CSS.patch → FilterBox-CSS.patch [landed in comment #171]
Attachment #8994048 - Attachment description: FilterBox-CSS.patch [landed in comment #171] → FilterBox-CSS.patch [landed in comment #170]
No longer blocks: tb-war-on-xbl
Depends on: 1526285
Depends on: 1538520
No longer depends on: 1538520
You need to log in before you can comment on or make changes to this bug.