1240894 - the new tab button in mailnews is set to display:none unless the close button is at the end

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[rexyrexy2]

Attachment: The rules in question

The button is set to display:none by default, and there is a style to set it to display:-moz-box if the close button is at the end. This makes no logical sense, as the button works the same either way, and this leaves no new-tab button when close buttons are set to display on the tabs.

*this is not a theme issue, as this rule is not set by the theme, but by messenger.css in omni.ja*

Comment 1

[Tony Mechelynck [:tonymec]]

(In reply to rexyrexy2 from comment #0)
[...]
> *this is not a theme issue, as this rule is not set by the theme, but by
> messenger.css in omni.ja*

IIUC, default CSS rules that themes "can override if they want to" are part of the Default theme (which is actually a kind of "empty" theme using all the defaults: in Safe Mode even the Default theme is displayed as "disabled" in about:addons, yet what we see is its characteristic look & feel). Any of these rules that Modern doesn't override are part of both themes.

But there are times when the border between Themes and UI Design becomes fuzzy and it isn't always obvious in which of them a given bug belongs. Ratty, this is more your area of expertise than it is mine: what do you think?

Comment 2

[Philip Chee]

(In reply to rexyrexy2 from comment #0)

> The button is set to display:none by default, and there is a style to set it
> to display:-moz-box if the close button is at the end. This makes no logical
> sense, as the button works the same either way, and this leaves no new-tab
> button when close buttons are set to display on the tabs.

> *this is not a theme issue, as this rule is not set by the theme, but by
> messenger.css in omni.ja*

I agree.

http://mxr.mozilla.org/comm-central/source/suite/mailnews/messenger.css?rev=6b0dd3834439&mark=193-200#193
> 193 .tabs-newbutton {
> 194   -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-new-tab-button");
> 195   display: none;
> 196 }
> 197 
> 198 .tabmail-tabs[closebuttons="closeatend"] .tabs-newbutton {
> 199   display: -moz-box;
> 200 }

These two rules should be replaced by just:

.tabs-newbutton {
  -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-new-tab-button");
}

Comment 3

[Philip Chee]

> These two rules should be replaced by just:
> 
> .tabs-newbutton {
>   -moz-binding:
> url("chrome://messenger/content/tabmail.xml#tabmail-new-tab-button");
> }

Anyone want to supply a patch?

Comment 4

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #3)
> > These two rules should be replaced by just:
> > 
> > .tabs-newbutton {
> >   -moz-binding:
> > url("chrome://messenger/content/tabmail.xml#tabmail-new-tab-button");
> > }
> 
> Anyone want to supply a patch?

Not much enthusiasm, it seems. OK, you convinced me to try. Patch coming soon.

Comment 5

[Tony Mechelynck [:tonymec]]

Attachment: Make new tab button always visible regardless of position of close tab button

Comment 6

[Philip Chee]

Comment on attachment 8714192 [details] [diff] [review]
Make new tab button always visible regardless of position of close tab button

nice! r=me

Comment 7

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #6)
> Comment on attachment 8714192 [details] [diff] [review]
> Make new tab button always visible regardless of position of close tab button
> 
> nice! r=me

Thanks, setting checkin-needed.

rexyrexy2: do you know (or can you find out) which earlier branches are affected? AFAIK there aren't any 2.43a2 or 2.42b yet but if 2.39 release is affected I suppose everything in between will be too. I'm asking, not in order to decide if this should be ported to release (it shouldn't: low severity affecting obviously neither severity nor stability) but to aurora and/or beta (low risk, no strings).

Comment 8

[Philip Chee]

IIRCmIt goes all the way back to when mail-tabs were introduced.

Comment 9

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/9be1d7c29bb3

Comment 10

[Tony Mechelynck [:tonymec]]

Comment on attachment 8714192 [details] [diff] [review]
Make new tab button always visible regardless of position of close tab button

Patch applies cleanly on aurora; I don't have beta or release clones.

[Approval Request Comment]
Regression caused by (bug #): unknown, maybe when mail tabs were added to SeaMonkey
User impact if declined: "new tab" button is absent unless "close tab" button is at far right
Testing completed (on m-c, etc.): patch landed on m-c, see comment #9
Risk to taking this patch (and alternatives if risky): none AFAICT (conditional "display: none" removed)
String changes made by this patch: none

Only approve for release if it can land before the release is built (2.41 I think?)

Comment 11

[Tony Mechelynck [:tonymec]]

Edmund: do you think this patch (no-risk but non-stability non-security) can land in time for the 2.41 release? If not, please set approval-comm-release-

Comment 12

[Edmund Wong (:ewong)]

(In reply to Tony Mechelynck [:tonymec] from comment #11)
> Edmund: do you think this patch (no-risk but non-stability non-security) can
> land in time for the 2.41 release? If not, please set approval-comm-release-

I don't see why not.

Comment 13

[Tony Mechelynck [:tonymec]]

(In reply to Edmund Wong (:ewong) from comment #12)
> (In reply to Tony Mechelynck [:tonymec] from comment #11)
> > Edmund: do you think this patch (no-risk but non-stability non-security) can
> > land in time for the 2.41 release? If not, please set approval-comm-release-
> 
> I don't see why not.

Thanks, this (and the previous a+) makes the patch ready to land on 2.43a2, 2.42b and 2.41. The problem is that 2.44a1 builds with the fix aren't easy to find: AFAIK there aren't any yet at ftp.m.o; but there are "unofficial" ones in several languages for L64, W32 and even W64 (but not L32 or Mac AFAICT) dated 2016-02-08, i.e. the day after comment #9, at https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/ These are from the official sources but not from official build machines, and they include Lightning as a cherry on the cake.

Testers, don't hesitate to avail yourselves of these builds (which admittedly lack "Socorro" symbols, so I advise you to download also the crashreporter-symbols.zip, it can help you add symbols to crash traces), and you Sheriffs (or whoever plays that role at SeaMonkey), feel free to land on branches as soon as you judge that the (albeit limited) baking this patch has seen from the above-mentioned 2.44a1 builds is enough (or that baking is not really necessary).

Comment 14

[Tony Mechelynck [:tonymec]]

Setting FIXED (on trunk) according to comment #9; the fix will hopefully "soon" land on the branches, see comment #13.

Comment 15

[Tony Mechelynck [:tonymec]]

Thanks Ratty!
http://hg.mozilla.org/releases/comm-aurora/rev/a97e880f9abe
http://hg.mozilla.org/releases/comm-beta/rev/f2e84bd12120
http://hg.mozilla.org/releases/comm-release/rev/b7f8063e48a9