Closed Bug 1553614 Opened 6 years ago Closed 6 years ago

[de-xbl] convert the tabmail-close-tab-button binding

Categories

(Thunderbird :: Mail Window Front End, task)

Product:
Thunderbird
Component:
Mail Window Front End
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 2 obsolete files)

tabmail-close-tab-button.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

Convert the tabmail-close-tab-button binding, which extends toolbarbutton.

https://searchfox.org/comm-central/rev/e649e2651e47fd724406b4b48d075e77e2ed9370/mail/base/content/tabmail.xml#2876

Might also be worth looking at what firefox dis for their button.

If I got it right, Firefox is using this method to deal with tabs and windows: https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#2562

We have a similar method here: https://searchfox.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#1107
I tried to leverage it but it doesn't work as expected, closing random tabs, or not responding at all.

I created a new method converting the XBL method to keep those conditions.

Attached patch tabmail-close-tab-button.patch (obsolete) (deleted) — — Splinter Review
Attachment #9067206 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9067206 [details] [diff] [review] tabmail-close-tab-button.patch Review of attachment 9067206 [details] [diff] [review]: ----------------------------------------------------------------- While this works, maybe we should take the opportunity to make it just an image, like firefox https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/browser/base/content/tabbrowser.xml#1955 ::: mail/base/content/mail3PaneWindowCommands.js @@ +1114,4 @@ > } > } > > +function CloseCurrentTab(event, button) { the name is slightly wrong, as it it's not necessarily current tab - it's the one whose button you clicked
Attachment #9067206 - Flags: review?(mkmelin+mozilla)

If I'm not wrong, FF uses only an image because it catches clicks with an XBL handler: https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/browser/base/content/tabbrowser.xml#2396

Do we want to emulate the same behaviour?
Or maybe we could use the occasion to de-xbl the entire tab?

Flags: needinfo?(mkmelin+mozilla)

de-xbl of the whole tab should probably wait for de-xbl of tab.
I think you could just add an oncommand="closeTab(event);" on the image?

Flags: needinfo?(mkmelin+mozilla)
Attached patch tabmail-close-tab-button.patch (obsolete) (deleted) — — Splinter Review

Sounds good.
The oncommand attribute doesn't work since it's not a button.
Using onclick works.

Attachment #9067206 - Attachment is obsolete: true
Attachment #9067414 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9067414 [details] [diff] [review] tabmail-close-tab-button.patch Review of attachment 9067414 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I think I gave the wrong advice :( I don't think we want to move this functionality to mail3PaneWIndowCommands.js - it should stay in tabmail.xml so that that functionality is confined. Indeed, you could add the function to the tabmail-tab instead (similar to firefox).
Attachment #9067414 - Flags: review?(mkmelin+mozilla)
Attached patch tabmail-close-tab-button.patch (deleted) — — Splinter Review
Attachment #9067414 - Attachment is obsolete: true
Attachment #9067478 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9067478 [details] [diff] [review] tabmail-close-tab-button.patch Review of attachment 9067478 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, do we ever use the tabs-closebutton button? Is it ever shown?
Attachment #9067478 - Flags: review?(mkmelin+mozilla) → review+

Yes, when you set mail.tabs.closeButtons to 3.

Thx RIchard! Doesn't seem Firefox has that anymore, and would seem pretty non-useful, so we could remove it. For another bug though.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/13000cbf0633
[de-xbl] convert tabmail-close-tab-button. r=mkmelin

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: