This bug is to convert "simple" <listbox> to <richlistbox>. Simple means, they don't need much JS logic for the conversion.

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.

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.

(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.

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"); + } + }

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).

(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); }

I did what Aceman suggested. That works. The if (wlNode.firstChild.getAttribute("checked") == "true") { is untested. Can we get the rest done soon?

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.

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.

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()"

(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.

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.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/e6fa0c597846 Convert simple <listbox> to <richlistbox>. r=jorgk

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

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.

This is the conversion for imAccountWizard. Maybe not super clean, but it works.

Fighting with the edit dictionary function. Whatever I tried, selItem.firstChild.setAttribute("label", gWordToAdd); does not update the display in the list :-( - Why now?

Hopefully cleaner approach.

Silly me, the label's attribute is called "value" and not "label" :-(

Thanks, that works, I reshuffled the element creation for greater clarity.

CustomHeaders.js needed changes.

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?

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.

(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.

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?

(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.

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.

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 :-(

"min-height" works. One review will do.

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? ;-)

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)?

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/4e763866d182 Part 4: Replace listbox in applicationManager.xul/js. r=darktrojan DONTBUILD

(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")

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?

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.

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.

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

Thanks, that works for me. Let's see whether any tests need updating: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=208f5677532ef352a18bfd3c8a12bc0b86e5a234

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

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.

I removed the filter editor hunk.

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.

Somehow my last changes didn't go into the patch. Now the treecols should be there. But still with the aligning error.

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.

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.

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.

This is the remaining listbox in calendar.

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

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.

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.

(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.

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?

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

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 :-(

That would be helpful, wouldn't it? :(

(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.

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.

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.

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.

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

This now looks better, columns align like the old editor. But the errors about getRowCount() still exist.

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.

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.

I am a bit further. My problem is that the buddy name and icon aren't applied.

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

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.

(In reply to Richard Marti (:Paenglab) from comment #105) > - Mouse cursor shows always background activity. This was something different. Shows no more.

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.

(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.

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.

(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.

(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">

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");

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

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.

listbox removed too. Aceman, does this look reasonable?

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.

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.

(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".

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.

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.

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.

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.

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).

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

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.

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.

(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.

Comment on attachment 8993767 [details] [diff] [review] msgCompSecurityInfo-crop.patch [landed in comment #138] Nice!

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

This equalizes the height of the elements. Fixed the "new rule animation". Also the spinbuttons for the age and size textbox are working again.

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.

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?

And, BTW, there is no such thing as a richlistcell, so please remove it here: https://hg.mozilla.org/comm-central/rev/7962d601822d#l3.105 https://hg.mozilla.org/comm-central/rev/7962d601822d#l5.38

I added two calendar reviewers. Richard, can you please check the CSS changes.

(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.

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.

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.

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.

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"?

(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.

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.

(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.

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.

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;">

(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 ;)

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

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 :-)

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.