1472556 - Port bug 1472555 : Replace use of "listbox" element which will be removed

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[:Paolo Amadini]

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.

Comment 1

[Richard Marti (:Paenglab)]

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

Comment 2

[:Paolo Amadini]

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.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: listbox.patch

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.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

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

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry, bug 1474258.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: listbox.patch

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

Comment 8

[Richard Marti (:Paenglab)]

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

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Arshad Khan [:arshad]]

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.

Comment 12

[Richard Marti (:Paenglab)]

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

Comment 13

[Richard Marti (:Paenglab)]

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

Comment 14

[Jorg K (CEST = GMT+2)]

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?

Comment 15

[Richard Marti (:Paenglab)]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

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

Comment 17

[Richard Marti (:Paenglab)]

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

Comment 18

[Richard Marti (:Paenglab)]

Attachment: RemoveListboxBinding.patch

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.

Comment 19

[Jorg K (CEST = GMT+2)]

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

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

Comment 20

[Richard Marti (:Paenglab)]

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

Comment 21

[Pulsebot]

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