Last Comment Bug 563261 - (SM-lwtheme) Make lightweight themes / personas work with SeaMonkey trunk in browser windows
(SM-lwtheme)
: Make lightweight themes / personas work with SeaMonkey trunk in browser windows
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: seamonkey2.1a3
Assigned To: Robert Kaiser
:
Mentors:
Depends on: 581382
Blocks: 579734 579731 579732 579737 579738 579739 581050 608966 610408 721583
  Show dependency treegraph
 
Reported: 2010-05-02 15:34 PDT by Robert Kaiser
Modified: 2012-01-26 16:40 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
base patch, v1: make install work (11.15 KB, patch)
2010-07-09 18:04 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
browser patch, v1.1: make install work and browser look decently (13.84 KB, patch)
2010-07-10 07:55 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
browser patch, v1.2: applies cleanly to trunk (13.89 KB, patch)
2010-07-11 15:00 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
browser patch, v1.3: address comments (14.47 KB, patch)
2010-07-15 17:22 PDT, Robert Kaiser
neil: superreview-
Details | Diff | Splinter Review
v2: address comments, factor out other stuff to followups (12.48 KB, patch)
2010-07-18 09:08 PDT, Robert Kaiser
iann_bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
Fix toolbargrippies (391 bytes, patch)
2010-07-19 09:48 PDT, Ian Neal
kairo: review-
Details | Diff | Splinter Review

Description Robert Kaiser 2010-05-02 15:34:26 PDT
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.
Comment 1 Robert Kaiser 2010-06-11 16:10:00 PDT
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.
Comment 2 Robert Kaiser 2010-07-09 18:04:56 PDT
Created attachment 456653 [details] [diff] [review]
base patch, v1: make install work

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.
Comment 3 Justin Wood (:Callek) 2010-07-09 19:00:30 PDT
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.
Comment 4 Robert Kaiser 2010-07-09 19:16:47 PDT
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. ;-)
Comment 5 Robert Kaiser 2010-07-10 07:55:49 PDT
Created attachment 456677 [details] [diff] [review]
browser patch, v1.1: make install work and browser look decently

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.
Comment 6 Robert Kaiser 2010-07-10 18:11:46 PDT
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 7 Robert Kaiser 2010-07-11 13:38:07 PDT
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.
Comment 8 Robert Kaiser 2010-07-11 15:00:31 PDT
Created attachment 456773 [details] [diff] [review]
browser patch, v1.2: applies cleanly to trunk

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. ;-)
Comment 9 Ian Neal 2010-07-12 13:17:10 PDT
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.
Comment 10 Ian Neal 2010-07-12 13:21:11 PDT
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
Comment 11 Robert Kaiser 2010-07-12 13:32:26 PDT
(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?
Comment 12 Ian Neal 2010-07-12 14:23:29 PDT
(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.
Comment 13 Justin Wood (:Callek) 2010-07-12 20:14:08 PDT
(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.
Comment 14 Robert Kaiser 2010-07-13 04:15:06 PDT
(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.
Comment 15 Ian Neal 2010-07-13 14:05:39 PDT
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 16 neil@parkwaycc.co.uk 2010-07-13 16:38:41 PDT
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?
Comment 17 Robert Kaiser 2010-07-14 07:42:47 PDT
(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.
Comment 18 neil@parkwaycc.co.uk 2010-07-14 16:48:07 PDT
(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.
Comment 19 Robert Kaiser 2010-07-14 17:14:52 PDT
(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.
Comment 20 Justin Wood (:Callek) 2010-07-15 16:16:52 PDT
(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
Comment 21 Robert Kaiser 2010-07-15 17:17:18 PDT
(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.
Comment 22 Robert Kaiser 2010-07-15 17:22:14 PDT
Created attachment 457723 [details] [diff] [review]
browser patch, v1.3: address comments

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.
Comment 23 Karsten Düsterloh 2010-07-15 22:54:42 PDT
> > 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.
Comment 24 neil@parkwaycc.co.uk 2010-07-16 03:35:01 PDT
(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...]
Comment 25 Robert Kaiser 2010-07-16 05:02:38 PDT
(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 26 neil@parkwaycc.co.uk 2010-07-16 05:41:28 PDT
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?*/
Comment 27 Robert Kaiser 2010-07-16 06:11:50 PDT
(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.
Comment 28 neil@parkwaycc.co.uk 2010-07-17 15:04:18 PDT
(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 29 Stefan [:stefanh] 2010-07-17 18:31:28 PDT
Comment on attachment 457723 [details] [diff] [review]
browser patch, v1.3: address comments

You missed classic/mac :P
Comment 30 Stefan [:stefanh] 2010-07-17 18:32:44 PDT
(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...
Comment 31 neil@parkwaycc.co.uk 2010-07-18 03:29:29 PDT
(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.
Comment 32 Robert Kaiser 2010-07-18 06:08:54 PDT
(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.
Comment 33 Robert Kaiser 2010-07-18 08:18:42 PDT
(In reply to comment #26)
> Well, I can't wait for XPCOMUtils.defineLazyModuleImport to hide all this...

Is there a bug on that?
Comment 34 neil@parkwaycc.co.uk 2010-07-18 08:36:06 PDT
(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.
Comment 35 Robert Kaiser 2010-07-18 08:50:23 PDT
(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.
Comment 36 Robert Kaiser 2010-07-18 09:08:44 PDT
Created attachment 458185 [details] [diff] [review]
v2: address comments, factor out other stuff to followups

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.
Comment 37 neil@parkwaycc.co.uk 2010-07-18 09:45:19 PDT
(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?
Comment 38 Robert Kaiser 2010-07-18 10:17:56 PDT
(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.
Comment 39 Ian Neal 2010-07-19 09:48:38 PDT
Created attachment 458368 [details] [diff] [review]
Fix toolbargrippies

Not sure if the opacity is set to the correct level...
Comment 40 Robert Kaiser 2010-07-19 12:29:04 PDT
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.
Comment 41 Robert Kaiser 2010-07-20 06:55:32 PDT
Pushed the main patch as http://hg.mozilla.org/comm-central/rev/470af94dee08 - let's do everything else in followups.

Note You need to log in before you can comment on or make changes to this bug.