Closed Bug 1472556 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: Paolo, Assigned: Paenglab)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Flags: needinfo?(richard.marti)
Summary: The "listbox" element will be removed → Port bug 1472555 : Replace use of "listbox" element which will be removed
I'm following bug 1472555 and bug 1472556. And then I either follow their changes or restore the bindings in C-C.
Flags: needinfo?(richard.marti)
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.
Depends on: 1474258
Attached patch listbox.patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8991104 - Flags: review?(jorgk)
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.
Attachment #8991104 - Flags: review?(jorgk) → review+
(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?
Attached patch listbox.patchSplinter Review
(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.
Attachment #8991104 - Attachment is obsolete: true
Attachment #8991111 - Flags: review+
(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.
Keywords: checkin-needed
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.
Flags: needinfo?(arshdkhn1)
Target Milestone: --- → Thunderbird 63.0
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Flags: needinfo?(arshdkhn1)
Arshad, bug 1472554 has some dependencies which show how m-c converted the listbox.
attachment 8991999 [details] from bug 1472555 removes also the listbox logic and not only the bindings. So we need this conversions really soon.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Depends on: 1475817
We need to back out the listbox stuff once bug 1475817 is done. listbox also uses 'invertSelection' which will be removed in bug 1476659.
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.
Attachment #8993214 - Flags: review?(jorgk)
Comment on attachment 8993214 [details] [diff] [review]
RemoveListboxBinding.patch

Sure, thanks. I'll see whether I can do the chat usage in a hurry.
Attachment #8993214 - Flags: review?(jorgk) → review+
Thanks, seems not to be one of the easiest. It has also some CSS for the icons.
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
No longer blocks: tb-war-on-xbl
You need to log in before you can comment on or make changes to this bug.