Closed Bug 563261 (SM-lwtheme) Opened 14 years ago Closed 14 years ago

Make lightweight themes / personas work with SeaMonkey trunk in browser windows

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1a3
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: kairo, Assigned: kairo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

The Mozilla trunk provides a lightweight themes / personas mechanism, we should make sure SeaMonkey can work with that.

I suspect we need at least some changes in our default theme files to make them apply nicely.
As a note, bug 511104 is the general lightweight themes tracker. Bug 511771 seems particularly interesting there, bug 520346 has some additions, bug 511107 and others work of actually supporting the lightweight themes in the UI.
This is the patch that does the base work and gets personas to install, just go to getpersonas.com with this patch applied and enjoy!

Note that this will need some more work on the default theme to make personas / lightweight themes to look nicer - but the main thing is that it starts working.

Also, this for the moment only makes the browser window work, but getting other windows into that should easily be doable in followups.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #456653 - Flags: review?(iann_bugzilla)
I'd love to take a patch that makes this work; my caveat our default theme shouldn't suck with this working. So if this can make the beta I'd be happy, if it comes close to missing feature-freeze-beta I think we might want to let it slip.
I fully agree that we should end up with the default theme looking really nicely, but from my testing it's usable with the patch a lone, and further cleanup can be done in followup patches - I hope that once the basic thing works, other people are motivated to clean up the details. ;-)
OK, here's a slightly updated patch that also makes browser looks a bit more decent - at least on Linux and hopefully Windows. On Mac, we really need stefanh to make that look well.
Attachment #456653 - Attachment is obsolete: true
Attachment #456677 - Flags: review?(iann_bugzilla)
Attachment #456653 - Flags: review?(iann_bugzilla)
Version: unspecified → Trunk
My MQ says the patch needs fuzz 2 to apply on a clean trunk, as I developed it on top of my bookmarks patches.

Ian, do you want a patch for review that applies without fuzz or is it OK for review in this form?
Comment on attachment 456677 [details] [diff] [review]
browser patch, v1.1: make install work and browser look decently

This needs a sr from Neil, I guess, as it's affecting UI somewhat.
Attachment #456677 - Flags: superreview?(neil)
Ian informed me that the patch didn't apply cleanly to trunk - this version does, I stacked it below the places bookmarks set in my MQ now. ;-)
Attachment #456677 - Attachment is obsolete: true
Attachment #456773 - Flags: superreview?(neil)
Attachment #456773 - Flags: review?(iann_bugzilla)
Attachment #456677 - Flags: superreview?(neil)
Attachment #456677 - Flags: review?(iann_bugzilla)
Okay, doing some testing:
When you click on "Allow" in notification bar you get in the error console:
Error: this.close is not a function
Source File: chrome://global/content/bindings/notification.xml
Line: 459
When you click on "Manage Themes" in notification bar you get taken to the Extensions rather than Themes screen in Add-on Manager

Still have to review the code itself.
More testing:
Clicking on remove for an installed persona in Add-on Manager, the persona is removed and you have an option to undo it. If click on undo, the persona returns but the undo option is left there and you get in the error console:
Error: Theme is not marked to be uninstalled
Source File: resource://gre/modules/LightweightThemeManager.jsm
Line: 467
(In reply to comment #9)
> When you click on "Allow" in notification bar you get in the error console:
> Error: this.close is not a function
> Source File: chrome://global/content/bindings/notification.xml
> Line: 459

Need to look into that.

> When you click on "Manage Themes" in notification bar you get taken to the
> Extensions rather than Themes screen in Add-on Manager

That's expected, we don't implement the proper stuff in toEM() right now, but that belongs into a different bug.

(In reply to comment #10)
> Clicking on remove for an installed persona in Add-on Manager, the persona is
> removed and you have an option to undo it. If click on undo, the persona
> returns but the undo option is left there and you get in the error console:
> Error: Theme is not marked to be uninstalled
> Source File: resource://gre/modules/LightweightThemeManager.jsm
> Line: 467

That sounds like a toolkit bug, does it happen in Minefield as well?
(In reply to comment #11)
> (In reply to comment #9)
> > When you click on "Allow" in notification bar you get in the error console:
> > Error: this.close is not a function
> > Source File: chrome://global/content/bindings/notification.xml
> > Line: 459
> 
> Need to look into that.

You only get the "Allow" button if getpersonas.com is not in the exception list, but even with it removed from firefox's list it doesn't happen.

> (In reply to comment #10)
> > Clicking on remove for an installed persona in Add-on Manager, the persona is
> > removed and you have an option to undo it. If click on undo, the persona
> > returns but the undo option is left there and you get in the error console:
> > Error: Theme is not marked to be uninstalled
> > Source File: resource://gre/modules/LightweightThemeManager.jsm
> > Line: 467
> 
> That sounds like a toolkit bug, does it happen in Minefield as well?
Yeah, that's a toolkit bug, happens in Minefield too - created Bug 578170 for that issue.
(In reply to comment #9)
> Okay, doing some testing:
> When you click on "Manage Themes" in notification bar you get taken to the
> Extensions rather than Themes screen in Add-on Manager

Maybe a toolkit bug, but I have to port their ability to load a specific pane of the AddonManager when it open/on-demand anyway, so this part could also be under my wing.
(In reply to comment #13)
> (In reply to comment #9)
> > Okay, doing some testing:
> > When you click on "Manage Themes" in notification bar you get taken to the
> > Extensions rather than Themes screen in Add-on Manager
> 
> Maybe a toolkit bug, but I have to port their ability to load a specific pane
> of the AddonManager when it open/on-demand anyway, so this part could also be
> under my wing.

Toolkit can do it, as Firefox does it correctly. I also have everything in the patch to hand over the same param to toEM() as FF does for their function. Just the code on our side to use the param correctly to call up the right list is missing, and that needs to be part of the remaining add-ons manager work, yes.
According to Mossop the xpinstall.whitelist.add prefs are only checked when the app version changes e.g. 2.1a2pre to 2.13pre so should be okay for users who only take releases but nightly users will need to manually add the exception.
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/AddonManager.jsm#183
Comment on attachment 456773 [details] [diff] [review]
browser patch, v1.2: applies cleanly to trunk

>-pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
>+pref("xpinstall.whitelist.add", "addons.mozilla.org");
Why this change?

>+#main-window[lwtheme="true"] {
>+  background-repeat: no-repeat;
>+  background-position: top right;
>+}
>+
>+#status-bar[lwthemefooter="true"] {
>+  background-repeat: no-repeat;
>+  background-position: bottom left;
>+}
This looks like something that belongs in communicator.css with tag rules instead of id rules.

>+  get _manager () {
>+    var temp = {};
>+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
>+    delete this._manager;
>+    return this._manager = temp.LightweightThemeManager;
>+  },
get LightweightThemeManager() {
  delete this.LightweightThemeManager;
  Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", this);
  return this.LightweightThemeManager;
}
Or just import it at the top level.

>+        LightWeightThemeWebInstaller._manager.forgetUsedTheme(newTheme.id);
>+        LightWeightThemeWebInstaller._manager.currentTheme = previousTheme;
Which would make this simpler.

>+  _getThemeFromNode: function (node) {
>+    return this._manager.parseTheme(node.getAttribute("data-browsertheme"),
>+                                    node.baseURI);
Or you could add a runtime check here, since this is always called first:
if (!LightweightThemeManager)
  Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm");

>+lwthemePostInstallNotification.undoButton=Undo
>+lwthemePostInstallNotification.undoButton.accesskey=U
Alt+U activates the Debug menu :-(

>+#urlbar:-moz-lwtheme:not([focused="true"]),
>+.tabbrowser-tab:-moz-lwtheme:not([selected="true"]) {
>+  opacity: .8;
Search and Go buttons need opacity too.

>+.tabbrowser-tab:-moz-lwtheme {
>+  text-shadow: none;
I assume this is overriding some toolkit shadow rule?

>+.tabbrowser-strip:-moz-lwtheme {
>+  padding-bottom: 0px;
What does this achieve? (I notice that the sizing of the toolbar and statusbar elements changes, at least on Windows.)

>+#security-button[level="high"]:-moz-lwtheme {
>+  background-color: #E8DB99;
>+  opacity: .8;
Why does this need to be on the button itself?
(In reply to comment #16)
> >-pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
> >+pref("xpinstall.whitelist.add", "addons.mozilla.org");
> Why this change?

Because it matches what Firefox has and it looks nicer.

> >+#main-window[lwtheme="true"] {
> >+  background-repeat: no-repeat;
> >+  background-position: top right;
> >+}
> >+
> >+#status-bar[lwthemefooter="true"] {
> >+  background-repeat: no-repeat;
> >+  background-position: bottom left;
> >+}
> This looks like something that belongs in communicator.css with tag rules
> instead of id rules.

That would make it apply to all SeaMonkey windows, even those that don't support lwthemes right now. I'd rather keep such rules confined to where we actually support it right now.

> >+  get _manager () {
> >+    var temp = {};
> >+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
> >+    delete this._manager;
> >+    return this._manager = temp.LightweightThemeManager;
> >+  },
> get LightweightThemeManager() {
>   delete this.LightweightThemeManager;
>   Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm",
> this);
>   return this.LightweightThemeManager;
> }
> Or just import it at the top level.

And would of course make us have a useless deviation from Firefox and make porting harder. But I'm tired of arguing that, so I might as well swallow my frustration about such review comments and do it.

> >+lwthemePostInstallNotification.undoButton=Undo
> >+lwthemePostInstallNotification.undoButton.accesskey=U
> Alt+U activates the Debug menu :-(

yay :(
Do we have some key we can use there that doesn't make us run into that? Or can we finally get rid of debugQA?

> >+#urlbar:-moz-lwtheme:not([focused="true"]),
> >+.tabbrowser-tab:-moz-lwtheme:not([selected="true"]) {
> >+  opacity: .8;
> Search and Go buttons need opacity too.

Makes sense, but such details might warrant a followup to beautify things further.

> >+.tabbrowser-tab:-moz-lwtheme {
> >+  text-shadow: none;
> I assume this is overriding some toolkit shadow rule?

Yes, one general to lwthemes. Firefox has the same for tabbrowser tabs, that's where I copied it from.

> >+.tabbrowser-strip:-moz-lwtheme {
> >+  padding-bottom: 0px;
> What does this achieve? (I notice that the sizing of the toolbar and statusbar
> elements changes, at least on Windows.)

I added that because I noticed that the tabs with the opacity had a stripe of pure background image between them and the web content and could track it down to a padding on the tabbrowser strip.

> >+#security-button[level="high"]:-moz-lwtheme {
> >+  background-color: #E8DB99;
> >+  opacity: .8;
> Why does this need to be on the button itself?

As you know, the "button" is a statusbar-panel and we only did not put the color on that itself because the -moz-appearance applies a background color that would override any theme setting. As lwtheme switches off native theming for those panels, we can do what we should do anyhow and apply the background to the panel itself.
(In reply to comment #17)
>>>+#main-window[lwtheme="true"] {
>>>+  background-repeat: no-repeat;
>>>+  background-position: top right;
>>>+}
>>>+
>>>+#status-bar[lwthemefooter="true"] {
>>>+  background-repeat: no-repeat;
>>>+  background-position: bottom left;
>>>+}
>>This looks like something that belongs in communicator.css with tag rules
>>instead of id rules.
>That would make it apply to all SeaMonkey windows, even those that don't
>support lwthemes right now. I'd rather keep such rules confined to where we
>actually support it right now.
Aren't the attributes you're using specific to the browser window?

>>>+lwthemePostInstallNotification.undoButton=Undo
>>>+lwthemePostInstallNotification.undoButton.accesskey=U
>>Alt+U activates the Debug menu :-(
> yay :(
> Do we have some key we can use there that doesn't make us run into that?
Alt+D?

> Or can we finally get rid of debugQA?
I guess we could put the bits of Debug that still work under QA.

>>>+.tabbrowser-strip:-moz-lwtheme {
>>>+  padding-bottom: 0px;
>>What does this achieve? (I notice that the sizing of the toolbar and statusbar
>>elements changes, at least on Windows.)
>I added that because I noticed that the tabs with the opacity had a stripe of
>pure background image between them and the web content and could track it down
>to a padding on the tabbrowser strip.
So the problem here (and elsewhere) is that hovering over a Persona has to reflow everything because the heights of many of the elements changes :-(
(And I wasn't even testing with the tabstrip visible...)

> > >+#security-button[level="high"]:-moz-lwtheme {
> > >+  background-color: #E8DB99;
> > >+  opacity: .8;
> > Why does this need to be on the button itself?
> As you know, the "button" is a statusbar-panel and we only did not put the
> color on that itself because the -moz-appearance applies a background color
> that would override any theme setting. As lwtheme switches off native theming
> for those panels, we can do what we should do anyhow and apply the background
> to the panel itself.
Not only does that makes the CSS harder to read than the lazy importing code, it means that the coloured area size changes.
(In reply to comment #18)
> Aren't the attributes you're using specific to the browser window?

Ah, right, they only apply when lwthemes are set anyhow. Might really be good to move on that account.

> >>>+lwthemePostInstallNotification.undoButton=Undo
> >>>+lwthemePostInstallNotification.undoButton.accesskey=U
> >>Alt+U activates the Debug menu :-(
> > yay :(
> > Do we have some key we can use there that doesn't make us run into that?
> Alt+D?

In my build, that activates the location bar... Alt+N seems not used here, though.

> > Or can we finally get rid of debugQA?
> I guess we could put the bits of Debug that still work under QA.

That might be a good idea - but I guess that should be a different bug anyhow. And IMHO the debugQA menus need a major overhaul anyhow.

> So the problem here (and elsewhere) is that hovering over a Persona has to
> reflow everything because the heights of many of the elements changes :-(

*Many* of the elements? Hrm, is that due to the native/non-native switch? I guess we should see if the same happens for Firefox. Anyhow, I'm very much in favor of having the main patch land and clear things up in theme-specific followup bugs/patches as much as possible.

> Not only does that makes the CSS harder to read than the lazy importing code,
> it means that the coloured area size changes.

Actually, I find the CSS easier to read for the lwtheme case. The changing area is less concern for me than the pure ugliness we are forced to have on the panel in the actual default theme.

Also, I'd guess that most actual users of lwtheme won't switch much between pure default theme and an lwtheme, but more between different lwthemes.
(In reply to comment #16)
> Comment on attachment 456773 [details] [diff] [review]
> browser patch, v1.2: applies cleanly to trunk
> 
> >-pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
> >+pref("xpinstall.whitelist.add", "addons.mozilla.org");
> Why this change?

Well imo not needed in the slightest for this patch, but I'm happy to take it as a ridealong anyway. In fact I had that as a TODO that I forgot about the manager work anyway.

> >+  get _manager () {
> >+    var temp = {};
> >+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
> >+    delete this._manager;
> >+    return this._manager = temp.LightweightThemeManager;
> >+  },
> get LightweightThemeManager() {
>   delete this.LightweightThemeManager;
>   Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm",
> this);
>   return this.LightweightThemeManager;
> }
> Or just import it at the top level.

And doing a toplevel import imo will be a leak of global variables (not great) and potentially a perf hit earlier than we really need. That and a divergence from the Firefox code that will make future backports MUCH harder to do.

> >+        LightWeightThemeWebInstaller._manager.forgetUsedTheme(newTheme.id);
> >+        LightWeightThemeWebInstaller._manager.currentTheme = previousTheme;
> Which would make this simpler.

I don't see how your change above makes this simpler. Different, yes. but simpler?

> >+#urlbar:-moz-lwtheme:not([focused="true"]),
> >+.tabbrowser-tab:-moz-lwtheme:not([selected="true"]) {
> >+  opacity: .8;
> Search and Go buttons need opacity too.

Better for these theme nits to a followup. imo
(In reply to comment #9)
> When you click on "Allow" in notification bar you get in the error console:
> Error: this.close is not a function
> Source File: chrome://global/content/bindings/notification.xml
> Line: 459

Hmm, I can trigger this, but installing works anyhow, and I completely fail to understand the error message. I have no clue how to move forward on that.
This addresses almost all comments so far, with the exception of this "Allow" button thing I have no clue about and the _manager change which I'm not very fond of. I think this really should be a private (and marked as such by the _ prefix) member of that object, and I also want to keep things in sync with Firefox so we can easily port over future updates.
Attachment #456773 - Attachment is obsolete: true
Attachment #457723 - Flags: superreview?(neil)
Attachment #457723 - Flags: review?(iann_bugzilla)
Attachment #456773 - Flags: superreview?(neil)
Attachment #456773 - Flags: review?(iann_bugzilla)
> > Error: this.close is not a function
> > Source File: chrome://global/content/bindings/notification.xml
> > Line: 459
> 
> Hmm, I can trigger this, but installing works anyhow, and I completely fail to
> understand the error message. I have no clue how to move forward on that.

IIRC from making the addon work in SM, this is fallout from trying to close the Persona dialog in the wrong context.
(In reply to comment #19)
>(In reply to comment #18)
>>>>>+lwthemePostInstallNotification.undoButton=Undo
>>>>>+lwthemePostInstallNotification.undoButton.accesskey=U
>>>>Alt+U activates the Debug menu :-(
>>> yay :(
>>> Do we have some key we can use there that doesn't make us run into that?
>>Alt+D?
>In my build, that activates the location bar...
Oops, I forgot about that.

> > So the problem here (and elsewhere) is that hovering over a Persona has to
> > reflow everything because the heights of many of the elements changes :-(
> *Many* of the elements? Hrm, is that due to the native/non-native switch? I
> guess we should see if the same happens for Firefox. Anyhow, I'm very much in
> favor of having the main patch land and clear things up in theme-specific
> followup bugs/patches as much as possible.
Yeah, it's probably a Toolkit bug anyway.

(In reply to comment #20)
>>>+        LightWeightThemeWebInstaller._manager.forgetUsedTheme(newTheme.id);
>>>+        LightWeightThemeWebInstaller._manager.currentTheme = previousTheme;
>>Which would make this simpler.
> I don't see how your change above makes this simpler. Different, yes. but
> simpler?
It would simply say LightweightThemeManager instead of LightWeightThemeWebInstaller._manager (so much for private variables...)
[and yes, Firefox also managed to screw up Lightweight/LightWeight...]
(In reply to comment #23)
> IIRC from making the addon work in SM, this is fallout from trying to close the
> Persona dialog in the wrong context.

Hmm, but this is a notification bar, not a dialog, and I'm not sure how clicking the button on the bar can be in the wrong context.
Comment on attachment 457723 [details] [diff] [review]
browser patch, v1.3: address comments

>+
>+
Nit: double blank line

>+  get _manager () {
>+    var temp = {};
>+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
>+    delete this._manager;
>+    return this._manager = temp.LightweightThemeManager;
Well, I can't wait for XPCOMUtils.defineLazyModuleImport to hide all this...

>+        LightWeightThemeWebInstaller._install(data);
>+      }
>+    }];
>+
>+    this._removePreviousNotifications();
>+
>+    var notificationBox = gBrowser.getNotificationBox();
>+    var notificationBar =
>+      notificationBox.appendNotification(message, "lwtheme-install-request", "",
>+                                         notificationBox.PRIORITY_INFO_MEDIUM,
>+                                         buttons);
>+    notificationBar.persistence = 1;
>+  },
>+
>+  _install: function (newTheme) {
>+    var previousTheme = this._manager.currentTheme;
>+    this._manager.currentTheme = newTheme;
>+    if (this._manager.currentTheme &&
>+        this._manager.currentTheme.id == newTheme.id)
>+      this._postInstallNotification(newTheme, previousTheme);
This is the cause of the close error; posting the install notification destroys the permission notification, which means it can no longer close itself. One workaround is for _install to return a flag indicating whether the notification was already closed (if the callback returns true then the notification does not try to close itself).

> #go-button {
>   margin-top: 0px;
>   margin-bottom: 0px;
>   -moz-margin-start: 0px;
>   -moz-margin-end: 4px;
>   min-height: 25px;
>   font: message-box;
>   font-weight: bold;
>+  opacity: .8;
Should be in a :moz-lwtheme rule, no?

>+#security-button[level="high"]:-moz-lwtheme {
>+  background-color: #E8DB99;
>+  opacity: .8;
>+}
>+
>+#security-button[level="broken"]:-moz-lwtheme {
>+  background-color: #E83404;
>+  opacity: .8;
>+}
>+
>+#security-button[label]:-moz-lwtheme {
>+  background-color: #62C441;
>+  opacity: .8;
>+}
So this is weird, because the button only has opacity when the level is broken, high or EV.
Please go back to the old way of setting the colour (which incidentally matches the case when it used to be an image background rather than a background colour) and then possibly give the whole button opacity (although I'm not convinced on this).

> .tabbrowser-strip {
>-  padding-bottom: 3px;
>   border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
So, the fix here is to change this to:
border-bottom: 5px solid;
-moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow ThreeDFace; /*-moz-dialog?*/
Attachment #457723 - Flags: superreview?(neil) → superreview-
(In reply to comment #26)
> So this is weird, because the button only has opacity when the level is broken,
> high or EV.

well, in any other case it doesn't have a background at all, so it appears fully opaque anyhow!

> Please go back to the old way of setting the colour (which incidentally matches
> the case when it used to be an image background rather than a background
> colour) and then possibly give the whole button opacity (although I'm not
> convinced on this).

Well, if you want it to look like crap, sure, can do that. SeaMonkey style is to appear crappy anyhow, so probably that's the best idea to keep that image.

> So, the fix here is to change this to:
> border-bottom: 5px solid;
> -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow ThreeDFace;
> /*-moz-dialog?*/

Well, if we go that way, then I'll file another bug because the tabbar looks so crappy, and remove that complete superfluous 4px and make it the same nice-looking 1py border as Firefox has in that new bug.
(In reply to comment #27)
> (In reply to comment #26)
> > So this is weird, because the button only has opacity when the level is broken,
> > high or EV.
> well, in any other case it doesn't have a background at all, so it appears
> fully opaque anyhow!
Actually a) there is a case where it does have a background b) the lock's opacity also changes when it's locked, which is the point.

> Well, if you want it to look like crap, sure, can do that. SeaMonkey style is
> to appear crappy anyhow, so probably that's the best idea to keep that image.
> Well, if we go that way, then I'll file another bug because the tabbar looks so
> crappy
Then why haven't you filed bugs before?
Comment on attachment 457723 [details] [diff] [review]
browser patch, v1.3: address comments

You missed classic/mac :P
(In reply to comment #29)
> Comment on attachment 457723 [details] [diff] [review]
> browser patch, v1.3: address comments
> 
> You missed classic/mac :P

Oh, I saw your comment about me fixing it now...
(In reply to comment #27)
> (In reply to comment #26)
> > Please go back to the old way of setting the colour (which incidentally matches
> > the case when it used to be an image background rather than a background
> > colour) and then possibly give the whole button opacity (although I'm not
> > convinced on this).
> Well, if you want it to look like crap, sure, can do that. SeaMonkey style is
> to appear crappy anyhow, so probably that's the best idea to keep that image.
This dates back to the original Modern icon from 2001-06-28:
http://mxr.mozilla.org/mozilla1.7/source/themes/modern/communicator/icons/lock-secure.gif
The Classic theme used to use the Netscape icon, but as part of our theme update we migrated it to use the Modern icon, keeping the same size background. The switch to EV simply moved the background into CSS on the image element.

> > So, the fix here is to change this to:
> > border-bottom: 5px solid;
> > -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow ThreeDFace;
> > /*-moz-dialog?*/
> Well, if we go that way, then I'll file another bug because the tabbar looks so
> crappy
And that dates back to 2001-12-19 so that's almost as old.
(In reply to comment #31)
> (In reply to comment #27)
> The Classic theme used to use the Netscape icon, but as part of our theme
> update we migrated it to use the Modern icon, keeping the same size background.
> The switch to EV simply moved the background into CSS on the image element.

Well, yes, because in the EV bug we already discussed the problem that we can't set the background on the whole panel, or else we would have changed that - like we did for Modern, btw.
Anyhow, I'll just cut the #security-button part from this patch and file another bug, I'm tired of fighting over those silly little things.

> > Well, if we go that way, then I'll file another bug because the tabbar looks so
> > crappy
> And that dates back to 2001-12-19 so that's almost as old.

That doesn't change that it looks bad.
Blocks: 579731
Blocks: 579732
Blocks: 579734
(In reply to comment #26)
> Well, I can't wait for XPCOMUtils.defineLazyModuleImport to hide all this...

Is there a bug on that?
(In reply to comment #32)
> Well, yes, because in the EV bug we already discussed the problem that we can't
> set the background on the whole panel, or else we would have changed that -
> like we did for Modern, btw.
No we didn't.

> That doesn't change that it looks bad.
No but you had 8 years in which to file a bug...

(In reply to comment #26)
> possibly give the whole button opacity
Actually that's wrong too, we should use an RGBA background colour.
Blocks: 579737
Blocks: 579738
Blocks: 579739
(In reply to comment #34)
> (In reply to comment #32)
> > Well, yes, because in the EV bug we already discussed the problem that we can't
> > set the background on the whole panel, or else we would have changed that -
> > like we did for Modern, btw.
> No we didn't.

Whatever you think, you won't believe me anyhow.

> > That doesn't change that it looks bad.
> No but you had 8 years in which to file a bug...

Well, I don't use the default theme, and I more and more see why.
In any case, I'll just leave it unchanged and move this into bug 579732 instead. Thanks for making things more complicated but I'm tired of fighting about issues that are not the core of a bug.

> (In reply to comment #26)
> > possibly give the whole button opacity
> Actually that's wrong too, we should use an RGBA background colour.

Good idea, I started to think that as well, but I've put all that off into its own bug 579731.
Summary: Make lightweight themes / personas work with SeaMonkey trunk → Make lightweight themes / personas work with SeaMonkey trunk in browser windows
Alias: SM-lwtheme
OK, so I fixed this callback issue and factored out the tabbrowser and security-button stuff to followups to avoid more fighting over UI here.
Attachment #457723 - Attachment is obsolete: true
Attachment #458185 - Flags: superreview?(neil)
Attachment #458185 - Flags: review?(iann_bugzilla)
Attachment #457723 - Flags: review?(iann_bugzilla)
(In reply to comment #35)
> Whatever you think, you won't believe me anyhow.
It's not a case of what I think, all I did was look up Modern's navigator.css (what I do think was I just copied it from one to the other when I implemented the changes, although I couldn't be bothered to check, so I didn't mention it.)

> > > That doesn't change that it looks bad.
> > No but you had 8 years in which to file a bug...
> Well, I don't use the default theme
Same 3px padding in Modern (Hewitt patched both themes simultaneously). Perhaps you've been using LCARStrek all this time instead? What padding does it use?
Attachment #458185 - Flags: superreview?(neil) → superreview+
(In reply to comment #37)
> (In reply to comment #35)
> > Well, I don't use the default theme
> Same 3px padding in Modern (Hewitt patched both themes simultaneously). Perhaps
> you've been using LCARStrek all this time instead? What padding does it use?

I've been using EarlyBlue and then LCARStrek, and both don't use any padding there. Also, Firefox has none, only our ancient-styled themes do.
Attachment #458185 - Flags: review?(iann_bugzilla) → review+
Not sure if the opacity is set to the correct level...
Attachment #458368 - Flags: review?(kairo)
Comment on attachment 458368 [details] [diff] [review]
Fix toolbargrippies

1) If you want to fix anything my patch here covers, please do it in a followup bugs, we already have enough pointless side case discussion here and we need those follup fixups anyhow if we want to ship anything looking halfway pleasant.

2) I'm not sure if we should change any hover styles at all, we currently are not applying any opacity changes to any hover effect on lwtheme anywhere.

3) Even without testing, I'm very sure that .5 is way too low an opacity value.
Attachment #458368 - Flags: review?(kairo) → review-
Pushed the main patch as http://hg.mozilla.org/comm-central/rev/470af94dee08 - let's do everything else in followups.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 581382
Target Milestone: --- → seamonkey2.1a3
Blocks: 610408
You need to log in before you can comment on or make changes to this bug.