Closed Bug 1240894 Opened 4 years ago Closed 4 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect, minor)

defect
Not set
minor

Tracking

(seamonkey2.41 fixed, seamonkey2.42 fixed, seamonkey2.43 fixed, seamonkey2.44 fixed)

RESOLVED FIXED
seamonkey2.44
Tracking Status
seamonkey2.41 --- fixed
seamonkey2.42 --- fixed
seamonkey2.43 --- fixed
seamonkey2.44 --- fixed

People

(Reporter: rexyrexy2, Assigned: tonymec)

Details

(Keywords: verifyme)

Attachments

(2 files)

Attached image 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*
Severity: normal → minor
(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?
Flags: needinfo?(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");
}
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(philip.chee)
OS: Unspecified → All
Hardware: Unspecified → All
> 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?
(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.
Assignee: nobody → antoine.mechelynck
Status: NEW → ASSIGNED
Comment on attachment 8714192 [details] [diff] [review]
Make new tab button always visible regardless of position of close tab button

nice! r=me
Attachment #8714192 - Flags: review?(philip.chee) → review+
(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).
Flags: needinfo?(rexyrexy2)
Keywords: checkin-needed
IIRCmIt goes all the way back to when mail-tabs were introduced.
Flags: needinfo?(rexyrexy2)
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?)
Attachment #8714192 - Flags: approval-comm-release?
Attachment #8714192 - Flags: approval-comm-beta?
Attachment #8714192 - Flags: approval-comm-aurora?
Attachment #8714192 - Flags: approval-comm-beta?
Attachment #8714192 - Flags: approval-comm-beta+
Attachment #8714192 - Flags: approval-comm-aurora?
Attachment #8714192 - Flags: approval-comm-aurora+
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-
Flags: needinfo?(ewong)
Keywords: checkin-needed
Whiteboard: [checkin see comment #10 and 11]
Whiteboard: [checkin see comment #10 and 11] → [checkin see comment #9 to 11]
(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.
Flags: needinfo?(ewong)
Attachment #8714192 - Flags: approval-comm-release? → approval-comm-release+
(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).
Setting FIXED (on trunk) according to comment #9; the fix will hopefully "soon" land on the branches, see comment #13.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: verifyme
Resolution: --- → FIXED
Whiteboard: [checkin see comment #9 to 11] → [checkin see comment #13]
You need to log in before you can comment on or make changes to this bug.