Closed
Bug 1472556
Opened 6 years ago
Closed 6 years ago
Port bug 1472555 : Replace use of "listbox" element which will be removed
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 63.0
People
(Reporter: Paolo, Assigned: Paenglab)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•6 years ago
|
Flags: needinfo?(richard.marti)
Summary: The "listbox" element will be removed → Port bug 1472555 : Replace use of "listbox" element which will be removed
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.
Flags: needinfo?(richard.marti)
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.
Depends on: 1474258
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.
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.
Attachment #8991104 -
Flags: review?(jorgk) → review+
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?
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.
Attachment #8991104 -
Attachment is obsolete: true
Attachment #8991111 -
Flags: review+
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.
Keywords: checkin-needed
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.
Flags: needinfo?(arshdkhn1)
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 63.0
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
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.
Flags: needinfo?(arshdkhn1)
Assignee | ||
Comment 12•6 years ago
|
||
Arshad, bug 1472554 has some dependencies which show how m-c converted the listbox.
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.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
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.
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.
Attachment #8993214 -
Flags: review?(jorgk)
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.
Attachment #8993214 -
Flags: review?(jorgk) → review+
Assignee | ||
Comment 20•6 years ago
|
||
Thanks, seems not to be one of the easiest. It has also some CSS for the icons.
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Updated•6 years ago
|
No longer blocks: tb-war-on-xbl
Updated•6 years ago
|
Blocks: tb-war-on-xbl
You need to log in
before you can comment on or make changes to this bug.
Description
•