1679587 - Close tab button changes size of Thunderbird window

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Bob Coleman]

Attachment: Greenshot-2020-11-27 18-02-34.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Double clicked tab close button.

Actual results:

Size of thunderbird window changes (toggles between maximized and not maximized).

Expected results:

Tab should close without window size changing. (Actually I'd prefer single clicking close the tab, but I could accept double clicking if it didn't change window size.

I don't know if I have the right terminology. I'm attaching a screenshot of my Thunderbird window. I'm talking about the "x" to the left of the buttons that open the calendar and tasks tabs.

Comment 1

[Richard Marti (:Paenglab)]

Alessandro, maybe this doesn't work correctly since bug 1554630. To test this you need to set the pref mail.tabs.closeButtons to 3. Then the close button will be on the end of the tab bar. I can fix the aligning when the button works again.

When clicking on the button nothing works.

Another thing I found is that https://searchfox.org/comm-central/source/mail/base/content/tabmail-tabs.js#434-477 is the same code as https://searchfox.org/comm-central/source/mail/base/content/tabbrowser-tab.js#115-158. Is this needed in both files?

Comment 2

[Alessandro Castellani [:aleca]]

I need to further investigate this, but yeah, I think the de'xbl work caused this regression.

Comment 3

[Alessandro Castellani [:aleca]]

I'm investigating this and I have a lot of questions.

What's the purpose of that closing button when only 1 tab is left?
Since we don't show the close tab for the primary mail tab (which makes sense), wouldn't be consistent to not show that button if only 1 tab is left?
The closing of the app should happen from the OS window buttons (minimize, enlarge, close).

Another thing I found is that https://searchfox.org/comm-central/source/mail/base/content/tabmail-tabs.js#434-477 is the same code as https://searchfox.org/comm-central/source/mail/base/content/tabbrowser-tab.js#115-158. Is this needed in both files?

This is a good question.
I guess is necessary since tabmail-tab is a CE inside the tabmail-tabs, and those 2 elements might handle clicks differently, even if it doesn't seem the case right now.
I think also that file name should be renamed to reflect the actual CE name: tabbrowser-tab.js > tabmail-tab.js

Wouldn't also make more sense to have those CE defined in a single file since both are called and only used once in the messenger.xhtml?
Unless we need them separate for add-on developers, for some reason.

Pinging Magnus to bring some clarity.

Comment 4

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #3)

I'm investigating this and I have a lot of questions.

What's the purpose of that closing button when only 1 tab is left?
Since we don't show the close tab for the primary mail tab (which makes sense), wouldn't be consistent to not show that button if only 1 tab is left?

Tested on TB 68 and the button does nothing when only one tab, the main tab, is open. Hide the button would be better.

Comment 5

[Magnus Melin [:mkmelin]]

Well first of all the mail.tabs.closeButtons seems very pointless to have. I don't see such an option even for Firefox anymore, though they have some other: https://searchfox.org/comm-central/source/mozilla/browser/app/profile/firefox.js#511

Agreed better not to show at all when there's only one tab. And +1 for moving to tabmail-tab.js

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1679587-tab-close-button.diff

This patch implements the following changes:

• Prevent toggling full size window when double clicking the close tab button.
• Hide/Show the arrow scroll box close button based on the pref and the amount of opened tabs, hooked to the "TabOpen" and "TabClose" events.
• Rename tabbrowser-tab.js to tabmail-tab.js

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9201010 [details] [diff] [review]
1679587-tab-close-button.diff

I need to double-click the close button to close a tab. This should be like the other close buttons a single click.
When I change the pref to 3 no close button is shown until I open a new tab or restart TB. The button should be shown immediately as I have multiple tabs open.
When i change the pref from 3 to another value the close button stays until I close a tab or restart TB.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9201010 [details] [diff] [review]
1679587-tab-close-button.diff

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

I'll let Richard handle this review

Comment 9

[Alessandro Castellani [:aleca]]

I need to double-click the close button to close a tab. This should be like the other close buttons a single click.

Do we know why it was originally intended to be a double click?
I didn't touch that behavior, but I'm okay with making it a simple regular click.

When i change the pref from 3 to another value the close button stays until I close a tab or restart TB.

Argh, good catch!
I wrongly removed the handling of the button from the pref:change listener.
Silly me.

Comment 10

[Richard Marti (:Paenglab)]

On TB 68 I need only a single click. So something must happened in the translation.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1679587-tab-close-button.diff

This fixes the pref switch problem, but the double click issue remains.
It seems that the double click is only necessary on Windows, as on Linux and macOS a single click works.

I'm pinging Thomas for some feedback, hoping he can find why on Windows a double click is required.
Thomas, this issue only happens if the pref mail.tabs.closeButtons is set to 3.

Comment 12

[Alessandro Castellani [:aleca]]

Gentle ping for Thomas to see if he has time to debug this on Windows.

Comment 13

[Thomas D. (:thomas8)]

I had a long look all around trying to figure this out, but no luck.

I put two console.log():
click tabmail-tabs.js:425:9
dblclick tabmail-tabs.js:484:9

This is weird:
With mail.tabs.closeButtons=3:

• single-left-click on close button at end of tabbar is not even firing (no console.log)
• for double-left-click, both our single-left-click AND the double-left-click fire.

I conclude: something else (maybe base tabs custom element) annihilates the single-click before it reaches here, but allows the single-click event to happen ONLY IF it's part of the double-click. For Firefox, this might makes sense as there's nothing happening on a single-click outside a tab header. Right-click and middle-click on the close-button at the end sail through and our click event fires, as seen from console.log.

Otherwise, I noticed this line which is still seen in Suite:
https://searchfox.org/comm-central/rev/9aa3bfea493b0032710c6a12a5ea933c3aff75a5/suite/mailnews/content/tabmail.xml#1119
this.setAttribute("closebuttons", "closeatend");
Alas, that doesn't fix it, as it's only for CSS, and no code attached.

Comment 14

[Thomas D. (:thomas8)]

Comment on attachment 9201218 [details] [diff] [review]
1679587-tab-close-button.diff

^^ see my comment 13. Something cancels the single-left-click before it reaches here.

Comment 15

[Alessandro Castellani [:aleca]]

Comment on attachment 9201218 [details] [diff] [review]
1679587-tab-close-button.diff

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

I'm gonna ask a review for this.
Even if it doesn't entirely solve the problem on Windows, it doesn't make things worst and improves a lot the handling of this button and its UI.

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 9201218 [details] [diff] [review]
1679587-tab-close-button.diff

I'm giving only the ui-r+ because it's an improvement to the actual state.

On Windows, a double-click is still needed but does no more switch the window mode.

This still needs a fix for Windows but maybe in a follow-up bug.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9201218 [details] [diff] [review]
1679587-tab-close-button.diff

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

I filed bug 1694926 for getting rid of the closeButtons 3.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a2bf0f78ec9
Fix several issues with tabmail tabs. r=mkmelin, ui-r=Paenglab