:Paolo Amadini Reporter
	Description • 6 years ago

In bug 1472555 we are removing the "listbox" element and the related platform code in favor of "richlistbox".

The "richlistbox" binding interface is mostly the same as "listbox", and for most simple lists the conversion only involves changing the tag name. Any use of the "rows" attribute can be changed to a "height" attribute or a CSS height. If the list uses checkboxes, these have to be added manually in the "richlistitem" elements.

	Richard Marti (:Paenglab) Assignee
	Comment 1 • 6 years ago

I'm following bug 1472555 and bug 1472556. And then I either follow their changes or restore the bindings in C-C.

	:Paolo Amadini Reporter
	Comment 2 • 6 years ago

In bug 1474258 I'm removing the "listheader" binding first, that is part of "listbox". While this can be restored in comm-central, I'm not sure it will continue to work as expected once we remove the rest of the layout code like ListboxObject and ListBoxBodyFrame in the next week or two, so a conversion to "richlistbox" might be necessary anyways.

	Richard Marti (:Paenglab) Assignee
	Comment 3 • 6 years ago

This patch restores the listbox binding in c-c. It can land also before it is removed in m-c. The bindings will be doubled in tree but the one from c-c will be used because it is loaded later than the m-c binding.

	Jorg K (CEST = GMT+2)
	Comment 4 • 6 years ago

Comment on attachment 8991104 [details] [diff] [review]
listbox.patch

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

I'm really not a great friend of forking all those bindings since that will increase our technical debt. It would be better to follow a replacement path here.

Untested, so rs=jorgk with the comment considered.

::: common/bindings/listbox.xml
@@ +10,5 @@
>  <bindings id="listboxBindings"
>            xmlns="http://www.mozilla.org/xbl"
>            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>            xmlns:xbl="http://www.mozilla.org/xbl">
> +#if 0

What is that?
#if 0 to the #endif is removed by the pre-processor.

@@ +1029,4 @@
>      <content>
>        <children>
>          <xul:listcell class="listcell-iconic" xbl:inherits="label,image,crop,disabled,flexlabel"/>
> +        <xul:listcell xbl:inherits="label,crop,disabled,flexlabel"/>

What is this doing?

::: mail/themes/linux/mail/listbox.css
@@ +43,5 @@
> +}
> +
> +/* ::::: listheader ::::: */
> +
> +listheader { 

Trailing space.

	Jorg K (CEST = GMT+2)
	Comment 5 • 6 years ago

(In reply to Richard Marti (:Paenglab) from comment #1)
> I'm following bug 1472555 and bug 1472556. And then I either follow their
> changes or restore the bindings in C-C.
Bug 1472556 is this bug here. You mean 1474258? Would it be possible to follow the M-C changes in a follow-up?

	Jorg K (CEST = GMT+2)
	Comment 6 • 6 years ago

Sorry, bug 1474258.

	Richard Marti (:Paenglab) Assignee
	Comment 7 • 6 years ago

(In reply to Jorg K (GMT+2) from comment #4)
> Comment on attachment 8991104 [details] [diff] [review]
> listbox.patch
> 
> Review of attachment 8991104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm really not a great friend of forking all those bindings since that will
> increase our technical debt. It would be better to follow a replacement path
> here.

I would second that. But this should do somebody which knows JS better than me.

> Untested, so rs=jorgk with the comment considered.
> 
> ::: common/bindings/listbox.xml
> @@ +10,5 @@
> >  <bindings id="listboxBindings"
> >            xmlns="http://www.mozilla.org/xbl"
> >            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >            xmlns:xbl="http://www.mozilla.org/xbl">
> > +#if 0
> 
> What is that?
> #if 0 to the #endif is removed by the pre-processor.

To remove the long explanation comment.

> @@ +1029,4 @@
> >      <content>
> >        <children>
> >          <xul:listcell class="listcell-iconic" xbl:inherits="label,image,crop,disabled,flexlabel"/>
> > +        <xul:listcell xbl:inherits="label,crop,disabled,flexlabel"/>
> 
> What is this doing?

Slipped in -> removed.

> ::: mail/themes/linux/mail/listbox.css
> @@ +43,5 @@
> > +}
> > +
> > +/* ::::: listheader ::::: */
> > +
> > +listheader { 
> 
> Trailing space.

This was a copy from m-c which I didn't changed.

	Richard Marti (:Paenglab) Assignee
	Comment 8 • 6 years ago

(In reply to Jorg K (GMT+2) from comment #5)
> (In reply to Richard Marti (:Paenglab) from comment #1)
> > I'm following bug 1472555 and bug 1472556. And then I either follow their
> > changes or restore the bindings in C-C.
> Bug 1472556 is this bug here. You mean 1474258? Would it be possible to
> follow the M-C changes in a follow-up?

It would be possible but from one that knows JS etc. better than me.

	Jorg K (CEST = GMT+2)
	Comment 9 • 6 years ago

Arshad, please take a look here. This is a typical bug we'd like you to work on. A binding is being removed in M-C and we're forking it because we don't have the skill set to follow M-C changes replacing it instead. In fact, now we do have the manpower and skill set: You ;-)

So please file a new bug to backout this changeset and do it better.

	Pulsebot
	Comment 10 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/be072a52c30e
Restore the listbox binding in C-C (will be removed in bug 1472555 and bug 1474258). r=jorgk

	Arshad Khan [:arshad]
	Comment 11 • 6 years ago

Hi Jorg, I ll take a look at it and file a new bug to backout this changeset as soon as I get comfortable with the overall picture of de-xbl. I am looking into some resources to get better understanding of it.

	Richard Marti (:Paenglab) Assignee
	Comment 12 • 6 years ago

Arshad, bug 1472554 has some dependencies which show how m-c converted the listbox.

	Richard Marti (:Paenglab) Assignee
	Comment 13 • 6 years ago

attachment 8991999 [details] from bug 1472555 removes also the listbox logic and not only the bindings. So we need this conversions really soon.

	Jorg K (CEST = GMT+2)
	Comment 14 • 6 years ago

So we are NOT done here, right? That conversion won't happen "really soon" since bug 1472555 is about to land.

Can't we switch to "richlistbox" like M-C does?

	Richard Marti (:Paenglab) Assignee
	Comment 15 • 6 years ago

We can but this needs a lot of JS changes too, which are above my JS knowledge. I'm actually on converting simple listboxes (like the tag listbox in prefs) to richlistboxes which don't need JS changes.

	Jorg K (CEST = GMT+2)
	Comment 16 • 6 years ago

We need to back out the listbox stuff once bug 1475817 is done. listbox also uses 'invertSelection' which will be removed in bug 1476659.

	Richard Marti (:Paenglab) Assignee
	Comment 17 • 6 years ago

I've seen that https://searchfox.org/comm-central/source/mail/components/im/content/chat-messenger.inc#172 is not converted.

	Richard Marti (:Paenglab) Assignee
	Comment 18 • 6 years ago

This is a back-out of the patch including the complete removal of listbox.xml.

With this patch applied, chat with the remaining <listbox> works better than without.

	Jorg K (CEST = GMT+2)
	Comment 19 • 6 years ago

Comment on attachment 8993214 [details] [diff] [review]
RemoveListboxBinding.patch

Sure, thanks. I'll see whether I can do the chat usage in a hurry.

	Richard Marti (:Paenglab) Assignee
	Comment 20 • 6 years ago

Thanks, seems not to be one of the easiest. It has also some CSS for the icons.

	Pulsebot
	Comment 21 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/64a4fe7e9949
Remove the listbox bindings after removal of supporting platform code in bug 1472555. r=jorgk DONTBUILD