1538547 - [de-xbl] remove badgebutton binding

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Description

[Magnus Melin [:mkmelin]]

The badgebutton binding looks to me like it's redundant.
It is mapped to one toobarbutton #button-chat

https://searchfox.org/comm-central/source/mail/components/im/content/badgebutton.xml#13

I think all the styling can be just done on the toolbar button.

The functionality of updating the label is only called in (maybe) on place, https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/chat-messenger.js#325

Comment 1

[Khushil Mistry [:khushil324]]

I think we should convert it to Custom Element, it will be fairly simple. What do you suggest?

Comment 2

[Magnus Melin [:mkmelin]]

I think in this case it's not needed. None of the other toolbar buttons are custom elements. Since it's so little functionality, I'd check if we can't just handle this from the outside.

It should be just removing the binding, adjusting the css slightly, and then the setting of the count. Essentially.

Comment 3

[Khushil Mistry [:khushil324]]

Okay, Sure. I will do that and make a patch.

Comment 4

[Magnus Melin [:mkmelin]]

We should wait for bug 1538983 which will make it easier and more clear what to do.

Comment 5

[Magnus Melin [:mkmelin]]

Bug 1519577 is the new thing to track. Bug 1538983 only converted normal buttons.

Comment 6

[Jorg K (CEST = GMT+2)]

I'll have to disable mozmill/im/test-toolbar-buttons.js | test-toolbar-buttons.js::test_toolbar_and_placeholder

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ee712009254
temporarily disable test-toolbar-buttons.js::test_toolbar_and_placeholder. rs=bustage-fix

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538547_de-xbl_chat-badgebutton.patch

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538547_de-xbl_chat-badgebutton.patch

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9067579 [details] [diff] [review]
Bug-1538547_de-xbl_chat-badgebutton.patch

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

This doesn't atm use very similar structure as the MozToolbarbutton. 
Would it be possible to make it that? I mean, possibly even just let it add the fragment, and override render() to add our label?

Maybe also just badgeCount as an observed attribute?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9067579 [details] [diff] [review]
Bug-1538547_de-xbl_chat-badgebutton.patch

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

::: mail/components/im/content/toolbarbutton-badge-button.js
@@ +20,5 @@
> +   */
> +  class MozBadgebutton extends customElements.get("toolbarbutton") {
> +    static get inheritedAttributes() {
> +      return {
> +        ...super.inheritedAttributes,

for instance, these would not be working properly since there is too much changed structure compared to the parent

Comment 12

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #10)

This doesn't atm use very similar structure as the MozToolbarbutton.
Would it be possible to make it that? I mean, possibly even just let it add
the fragment, and override render() to add our label?

Maybe also just badgeCount as an observed attribute?

I don't think that it is possible like toolbarbutton-menu-button because we want a badge over the image. In toolbarbutton-menu-button, we are adding a dropmarker besides the toolbarbutton. We can have a badge over the whole toolbarbutton but it will not look good in terms of UI/UX.

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538547_de-xbl_chat-badgebutton.patch

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069648 [details] [diff] [review]
Bug-1538547_de-xbl_chat-badgebutton.patch

Sorry for stealing this. As far as I can see, this works and it's also like a cook-cutting exercise. It also fixes bug 1555136 which seems to come from some "refresh" issue due to a mix of XBL code removed here and the already de-XBL'ed other parts.

The test passes, too, so I'll enable it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f6f0c189c55a
[de-xbl] convert badgebutton binding to custom element. r=jorgk