Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement Tabs Toolbar for Thunderbird and Lightning Compatibility.

RESOLVED FIXED in seamonkey2.10

Status

SeaMonkey
MailNews: General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.10

SeaMonkey Tracking Flags

(seamonkey2.8 fixed, seamonkey2.9 fixed, seamonkey2.10 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Lighting moved their tabbar buttons into the new Thunderbird tabbar-toolbar. We should somehow support this.
(Assignee)

Comment 1

6 years ago
Created attachment 591731 [details] [diff] [review]
Patch v1.0 Proposed fix.

> +++ b/suite/themes/modern/messenger/mailWindow1.css
> 
> +.tabbar-toolbar {
> +  border-bottom: 1px solid #000000;
> +  min-height: 0;
> +}
Couldn't find out why the bottom border disappeared so I added this hack.
Attachment #591731 - Flags: review?(neil)

Comment 2

6 years ago
(In reply to Philip Chee from comment #1)
> > +.tabbar-toolbar {
> > +  border-bottom: 1px solid #000000;
> > +  min-height: 0;
> > +}
> Couldn't find out why the bottom border disappeared so I added this hack.
It's because the toolbar isn't transparent. It probably needs to be. I don't know if toolbars are transparent under Aero; they do have -moz-appearance and it might or might not need to be turned off, as well as the border, for which I'd prefer separate bottom/top border styles please.
(Assignee)

Comment 3

6 years ago
Created attachment 596969 [details] [diff] [review]
Patch v1.1 Better fix.

>> Couldn't find out why the bottom border disappeared so I added this hack.
> It's because the toolbar isn't transparent. It probably needs to be. I don't

Aha. Setting |background-color: transparent;| works.

> know if toolbars are transparent under Aero; they do have -moz-appearance
> and it might or might not need to be turned off,
Yeah I needed to turn off -moz-appearance for aero as I did in the v1 patch.

> as well as the border, for
> which I'd prefer separate bottom/top border styles please.
Done!

----------------------------------------------------------

Stefanh I don't have a Mac so I'm totally guessing here. Please recommend if
any styles are needed at all.

> +++ b/suite/themes/classic/mac/messenger/mailWindow1.css

> +
> +.tabbar-toolbar {
> +  -moz-appearance: none;
> +}
Attachment #591731 - Attachment is obsolete: true
Attachment #591731 - Flags: review?(neil)
Attachment #596969 - Flags: ui-review?(stefanh)
(Assignee)

Comment 4

6 years ago
> but I have one question: why not use the id selector?

I might want to reuse the class? Also:

http://csswizardry.com/2011/09/writing-efficient-css-selectors/

It is important to note that, although an ID is technically faster and more performant, it is barely so. Using Steve Souders’ CSS Test Creator we can see that an ID selector and a class selector show very little difference in reflow speed.

In Firefox 6 on a Windows machine I get an average reflow figure of 10.9 for a simple class selector. An ID selector gave a mean of 12.5, so this actually reflowed slower than a class.

Comment 5

6 years ago
Comment on attachment 596969 [details] [diff] [review]
Patch v1.1 Better fix.

Looks fine to me on Mac. But you should add 'nowindowdrag="true"' to the toolbar in messenger.xul. I'm pretty sure we don't want the transparent toolbar to be draggable ;-)

Actually, not using the nowindowdrag attribute made me spot an error in toolbar.css - we give the 'xpfe="false"' toolbars a grippy under certain circumstances (I'll fix that in a separate bug).

Re the class usage, I don't think there's any perf issues with using a class (even though I'm sceptical to the reflow results you're refering to), but I think it's confusing using a class on a unique element with an id attribute. Also, using a class forces you to add extra code, but you have the id for free.
Attachment #596969 - Flags: ui-review?(stefanh) → ui-review+
(Assignee)

Comment 6

6 years ago
Created attachment 597812 [details] [diff] [review]
Patch v1.2 Use id selector in CSS. ui-r=stefanh.

> Looks fine to me on Mac. But you should add 'nowindowdrag="true"' to the
> toolbar in messenger.xul. I'm pretty sure we don't want the transparent
> toolbar to be draggable ;-)
Fixed.

> Re the class usage, I don't think there's any perf issues with using a class
> (even though I'm sceptical to the reflow results you're refering to), but I
> think it's confusing using a class on a unique element with an id attribute.
> Also, using a class forces you to add extra code, but you have the id for
> free.
Fixed.
Attachment #597812 - Flags: review?(neil)

Comment 7

6 years ago
Comment on attachment 597812 [details] [diff] [review]
Patch v1.2 Use id selector in CSS. ui-r=stefanh.

>+  min-height: 0;
Don't pinstripe/Modern have a min-height that you need to override too?

>+@media (-moz-windows-theme: aero) {
There's no @media block in toolbar.css so I'd remove it for consistency.

Comment 8

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #7)
> Comment on attachment 597812 [details] [diff] [review]
> Patch v1.2 Use id selector in CSS. f=stefanh.
> 
> >+  min-height: 0;
> Don't pinstripe/Modern have a min-height that you need to override too?

Ah, that's true.
(Assignee)

Updated

6 years ago
Attachment #597812 - Attachment description: Patch v1.2 Use id selector in CSS. f=stefanh. → Patch v1.2 Use id selector in CSS. ui-r=stefanh.
(Assignee)

Comment 9

6 years ago
Created attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

>>+  min-height: 0;
> Don't pinstripe/Modern have a min-height that you need to override too?
Fixed.

>>+@media (-moz-windows-theme: aero) {
> There's no @media block in toolbar.css so I'd remove it for consistency.
Fixed.
Attachment #596969 - Attachment is obsolete: true
Attachment #597812 - Attachment is obsolete: true
Attachment #597812 - Flags: review?(neil)
Attachment #598189 - Flags: ui-review+
Attachment #598189 - Flags: review?(neil)
Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

>+#tabbar-toolbar {
>+  min-height: 0;
>+  background-color: transparent;
>+}
One last nit, put the min-height last for consistency with the other themes.
Attachment #598189 - Flags: review?(neil) → review+
(Assignee)

Comment 11

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a35da4eb6892
status-seamonkey2.10: --- → fixed
Target Milestone: --- → seamonkey2.10
(Assignee)

Comment 12

6 years ago
Oops forgot to close this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

6 years ago
Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

[Approval Request Comment]
Regression caused by ): Lighting Bug 709572 - Add Calendar and Task toolbar for tabs on top 
User impact if declined: User will be unable to open a Calendar or Tasks tab even from the menu.
Testing completed (on m-c, etc.): Baked and tested on comm-central.
Risk to taking this patch (and alternatives if risky): low risk. Alternative is to write an addon but then we'd need to localize that addon too.
String changes made by this patch: Two additional strings.
 +<!ENTITY showTabsToolbarCmd.label "Tabs Toolbar">
 +<!ENTITY showTabsToolbarCmd.accesskey "T">
Attachment #598189 - Flags: approval-comm-beta?
Attachment #598189 - Flags: approval-comm-aurora?
Comment on attachment 598189 [details] [diff] [review]
Patch v1.3 Fix More Nits ui-r=stefanh.

Ratty seems to have a patch (from a convo in IRC) that would make this useable on beta/aurora without l10n changes.

Has a slight code-change to make it sane, but if that can pass a review I give it pre-emptive a+
Attachment #598189 - Flags: approval-comm-beta?
Attachment #598189 - Flags: approval-comm-beta-
Attachment #598189 - Flags: approval-comm-aurora?
Attachment #598189 - Flags: approval-comm-aurora-
(Assignee)

Comment 15

6 years ago
To summarize a IRC conversation with Callek a L10n safe patch for branches.

1. Remove the toolbarname and accesskey, thus removing the need for L10n entities.

2. Then modify utilityOverlay->onViewToolbarsPopupShowing() to take into account external toolbars without the "toolbarname" attribute.

replace:
  var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*"));
  toolbars = toolbars.concat(toolbox.externalToolbars);

with:
  var externalToolbars = Array.filter(toolbox.externalToolbars,
                                      function(toolbar) {
                                        return toolbar.hasAttribute("toolbarname")});
  var toolbars = Array.slice(toolbox.getElementsByAttribute("toolbarname", "*"));
  toolbars = toolbars.concat(externalToolbars);
(Assignee)

Comment 16

6 years ago
Created attachment 600988 [details] [diff] [review]
Patch v2.1 L10n safe Branch Patch.

Not tested on Aurora/Beta. But works on trunk.
Comment on attachment 600988 [details] [diff] [review]
Patch v2.1 L10n safe Branch Patch.

Neil can you do a fast turnaround here, its just a branch version of what you reviewed already, but has js changes not in original.
Attachment #600988 - Flags: review?(neil)
(Assignee)

Comment 18

6 years ago
I should add that the js changes are needed on trunk as well to take into account third party external toolbars (from addons) that don't bother to have toolbarname attributes.
Comment on attachment 600988 [details] [diff] [review]
Patch v2.1 L10n safe Branch Patch.

I don't see why we need to make the toolbar customisable on branch. I'm not even sure why we need it on trunk; the calendar buttons aren't removable, and it doesn't make sense to change the settings for the toolbar. At least on trunk the l10n changes allow us to hide the toolbar completely...
Attachment #600988 - Flags: review?(neil) → review-
(Assignee)

Comment 20

6 years ago
Created attachment 602288 [details] [diff] [review]
Patch v2.2 Branch patch with non-customizable toolbar.

> I don't see why we need to make the toolbar customisable on branch. I'm not
> even sure why we need it on trunk; the calendar buttons aren't removable,
> and it doesn't make sense to change the settings for the toolbar. At least
> on trunk the l10n changes allow us to hide the toolbar completely...

The Thunderbird tabbar-toolbar which Lightning inserts it's button into is
customizable and is meant to be a replacement for the non-customizable
<box id="tabmail-buttons">.

The intention of TB is to get extensions to migrate any UI they insert in the
latter to the toolbar.

IIRC The lightning toolbar buttons are deliberately non-customizable.
Attachment #602288 - Flags: review?(neil)

Updated

6 years ago
Attachment #602288 - Flags: review?(neil) → review+
Comment on attachment 602288 [details] [diff] [review]
Patch v2.2 Branch patch with non-customizable toolbar.

[Triage Comment]
Attachment #602288 - Flags: approval-comm-beta+
Attachment #602288 - Flags: approval-comm-aurora+
(Assignee)

Comment 22

6 years ago
Pushed to branches.
http://hg.mozilla.org/releases/comm-aurora/rev/80ddee7da026
http://hg.mozilla.org/releases/comm-beta/rev/18c5e81531db
status-seamonkey2.8: --- → fixed
status-seamonkey2.9: --- → fixed
You need to log in before you can comment on or make changes to this bug.