1417198 - Migrate statusbarpanel-menu-iconic, statusbarpanel-iconic, statusbarpanel-iconic-text bindings to TB

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Brian Grinstead [:bgrins]]

In Bug 1416368 I'm planning to remove statusbarpanel-* bindings and associated CSS from mozilla-central. These are still used in comm-central: https://searchfox.org/comm-central/search?q=statusbarpanel-&path=.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: statusbar.patch

For TB I have already a patch since I saw you want to remove this bindings.

Frg, what do you think for SM? Do you want to do something similar or do you want to implement the bindings in suite?

Comment 2

[Frank-Rainer Grahl (:frg)]

Looks good. I would prefer not to reimplenet anything.

Stefanh do you have probably some time to look into this for suite?

Comment 3

[Stefan [:stefanh]]

Brian, just a question - appreciate some advice from a de-xbl expert here :-)

It looks to me that statusbar and statusbarpanel also will be removed soon. When I look at the statusbar* bindings, wouldn't they be good candidates for converting to Custom Elements? I realize that there might be some work left on Custom Elements, but I'm thinking that the best strategy for comm-central is to just migrate the bindings and then convert them to Custom Elements at a later stage (like in a couple of months).

Comment 4

[Stefan [:stefanh]]

Actually, suite already has a binding in its general.xml that extends statusbarpanel-iconic-text. So just moving the m-c bindings there would be the best option anyway (doing it the TB way won't work out-of-the-box).

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Stefan [:stefanh] from comment #3)
> Brian, just a question - appreciate some advice from a de-xbl expert here :-)
> 
> It looks to me that statusbar and statusbarpanel also will be removed soon.
> When I look at the statusbar* bindings, wouldn't they be good candidates for
> converting to Custom Elements? I realize that there might be some work left
> on Custom Elements, but I'm thinking that the best strategy for comm-central
> is to just migrate the bindings and then convert them to Custom Elements at
> a later stage (like in a couple of months).

Yes, they would be good candidates for Custom Elements (they don't use [implements] or other complex features). This is definitely not final output, but I have a tool that converts XBL:

- https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/StatusbarpanelIconic.js
- https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/StatusbarpanelIconicText.js
- https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/StatusbarpanelMenuIconic.js

In particular, statusbarpanel-menu-iconic would need to use shadow DOM to keep parity (to match "<children />" behavior from XBL).

Custom Element in XUL work is still in progress in Bug 1404420, so in the mean time I've been looking for bindings that are unused.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Stefan [:stefanh] from comment #4)
> Actually, suite already has a binding in its general.xml that extends
> statusbarpanel-iconic-text. So just moving the m-c bindings there would be
> the best option anyway (doing it the TB way won't work out-of-the-box).

Makes sense to me. I'll plan on landing the m-c removal and expect that the XML/CSS would move over to c-c.

Comment 7

[Stefan [:stefanh]]

Thanks Brian. Filed bug 1418094 (have a patch there as well) for the suite part.

Comment 8

[Stefan [:stefanh]]

(In reply to Brian Grinstead [:bgrins] from comment #5)

> In particular, statusbarpanel-menu-iconic would need to use shadow DOM to
> keep parity (to match "<children />" behavior from XBL).

I can't find any usage of that binding in comm-central, so I guess that makes conversion easier.

Comment 9

[Stefan [:stefanh]]

Richard - I first had this idea that we could put the bindings in a shared directory, but if the flattening works for TB it makes of course more sense than to keep using xbl bindings.

Comment 10

[Richard Marti (:Paenglab)]

With the SM specific bug 1418094 I rename this one to TB only.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 8928283 [details] [diff] [review]
statusbar.patch

Aceman, this patch uses instead of the binding simple XUL plus copies the CSS from global.css to messenger.css.

Comment 12

[:aceman]

Comment on attachment 8928283 [details] [diff] [review]
statusbar.patch

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

I don't understand this patch. You do not just unroll/open-code the contents of the binding to the places where we used it. You only selectively add <image/> somewhere but not all places. Also you do not add class="statusbarpanel-icon" to the images, which the original binding had.

Comment 13

[:aceman]

So for the offline icon you use toolbarbutton instead of an image.

Comment 14

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #12)
> Comment on attachment 8928283 [details] [diff] [review]
> statusbar.patch
> 
> Review of attachment 8928283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand this patch. You do not just unroll/open-code the contents
> of the binding to the places where we used it. You only selectively add
> <image/> somewhere but not all places. 

Which places are missing?

> Also you do not add class="statusbarpanel-icon" to the images, which the
> original binding had.

This class makes no sense as where is no rule for it.

(In reply to :aceman from comment #13)
> So for the offline icon you use toolbarbutton instead of an image.

In reality it's a button (or an area) where you can set the online/offline state. With this the function is clearer.

Comment 15

[:aceman]

Comment on attachment 8928283 [details] [diff] [review]
statusbar.patch

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

::: mail/themes/linux/mail/messenger.css
@@ +582,5 @@
>  
>  /* Status panel */
>  
> +.statusbarpanel-iconic,
> +.statusbarpanel-iconic-text {

Where is statusbarpanel-iconic-text used?

Comment 16

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #14)
> > I don't understand this patch. You do not just unroll/open-code the contents
> > of the binding to the places where we used it. You only selectively add
> > <image/> somewhere but not all places. 
> 
> Which places are missing?

I meant the offline icon, but I then saw you just converted it to a button instead of image. OK.
 
> > Also you do not add class="statusbarpanel-icon" to the images, which the
> > original binding had.
> 
> This class makes no sense as where is no rule for it.

OK.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: statusbar.patch

(In reply to :aceman from comment #15)
> Comment on attachment 8928283 [details] [diff] [review]
> statusbar.patch
> 
> Review of attachment 8928283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/themes/linux/mail/messenger.css
> @@ +582,5 @@
> >  
> >  /* Status panel */
> >  
> > +.statusbarpanel-iconic,
> > +.statusbarpanel-iconic-text {
> 
> Where is statusbarpanel-iconic-text used?

Good catch, copy/paste from global.css. Removed now because not used.

Comment 18

[:aceman]

Comment on attachment 8929545 [details] [diff] [review]
statusbar.patch

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

Thanks.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d342491c066b
Fix TB after landing of bug 1416368: Remove unused statusbarpanel-* bindings. r=jorgk

Comment 20

[Jorg K (CEST = GMT+2)]

For the record: r=aceman.
Please check "hg out" carefully before pushing.