579731 - Improve security button look in default theme

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Robert Kaiser]

The current default theme "security button" panel in the status bar only covers the area of the text and icon, but not a uniform square or the whole panel. Esp. in the EV case, this looks strange, as the shapes of the two don't fit too well.

Ideally, we'd color the whole panel, but -moz-appearance assigns a background color we can't change, so we need to refrain to that coloring of only the image and text parts by themselves.

In the lwtheme case, we remove the -moz-appearance, so we can color the panel background, but we should let the lwtheme image shine through.

Comment 1

[[e-mail]]

Actually we could colour the whole panel if we implemented our own binding that interposed an extra box purely for that purpose.

Comment 2

[Robert Kaiser]

That's probably not a bad idea at all...

Comment 3

[Robert Kaiser]

Attachment: v1: implement a binding with an extra box and use it

OK, following your suggestion, this is where I ended up: This patch implements that extra binding and uses it for the security button.

Comment 4

[[e-mail]]

Comment on attachment 459131 [details] [diff] [review]
v1: implement a binding with an extra box and use it

>diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml
Nit: statusbarpanel doesn't really belong in toolbar.xml

>+  -moz-box-pack: stretch;
The Error Console would have told you that there's no such thing as -moz-box-pack: stretch; as it happens in this case you don't need it but for maximum flexibility (pun intended) you would inherit the flex from the statusbarpanel to the hbox.

>+  padding: 0;
Nit: suite style is to use 0px instead of the default unit.

>+/* solid background color on normal theme, translucent one with it */
>+#security-button[level="high"] > .statusbarpanel-contentbox:not(:-moz-lwtheme),
>+#security-button[level="low"] > .statusbarpanel-contentbox:not(:-moz-lwtheme) {
>   background-color: #E8DB99;
...
>+#security-button[level="high"] > .statusbarpanel-contentbox:-moz-lwtheme,
>+#security-button[level="low"] > .statusbarpanel-contentbox:-moz-lwtheme {
>+  background-color: rgba(232, 219, 153, .8);
You don't actually need to provide separate rules with and without :-moz-lwtheme, the idea of CSS is that the more important, specific, or failing that, later rule wins.

>-#security-button > .statusbarpanel-icon {
>+#security-button {
If you're going to change this to use CSS inheritance then you may as well do it for the other styles you're changing too. But it would be slightly neater to inherit from the contentbox as that way you can set the image and background at the same time.

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> >diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml
> Nit: statusbarpanel doesn't really belong in toolbar.xml

Thene where does it belong? I really don't want to create yet another file for this simple binding (as every additional file we load costs perf and we are doing a lot of things wrong in that area anyhow) and I couldn't find any existing XBL file in common that would fit better.

> >+  -moz-box-pack: stretch;
> The Error Console would have told you that there's no such thing as
> -moz-box-pack: stretch; as it happens in this case you don't need it but for
> maximum flexibility (pun intended) you would inherit the flex from the
> statusbarpanel to the hbox.

Hmm, I guess the box extends to the full size anyhow, I guess both the -moz-box-pack and -moz-box-align rules there aren't really needed on the panel.

> >+  padding: 0;
> Nit: suite style is to use 0px instead of the default unit.

I thought 0 was faster than 0px, but sure, can do it either way.

> You don't actually need to provide separate rules with and without
> :-moz-lwtheme, the idea of CSS is that the more important, specific, or failing
> that, later rule wins.

Well, all the rules are needed, I just though that specifying the :not() would make it s bit clearer and would increase speed by not applying at all where not needed.

> >-#security-button > .statusbarpanel-icon {
> >+#security-button {
> If you're going to change this to use CSS inheritance then you may as well do
> it for the other styles you're changing too. But it would be slightly neater to
> inherit from the contentbox as that way you can set the image and background at
> the same time.

Does that work?
I used that actually to make things consistent between our different themes (classic, mac, and Modern) as before my patch we are inconsistent between them.

Comment 6

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #4)
> > >diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml
> I couldn't find any existing XBL file in common that would fit better.
There are no XBL files where it fits, except per-theme, where odds and ends get dumped into globalBindings.xml (Modern) or communicatorBindings.xml (Classic).

> > >+  -moz-box-pack: stretch;
> > The Error Console would have told you that there's no such thing as
> > -moz-box-pack: stretch; as it happens in this case you don't need it but for
> > maximum flexibility (pun intended) you would inherit the flex from the
> > statusbarpanel to the hbox.
> Hmm, I guess the box extends to the full size anyhow, I guess both the
> -moz-box-pack and -moz-box-align rules there aren't really needed on the panel.
The -moz-box-align rule makes the contentbox the full height of the panel, which you may or may not want. (I have a slight preference for not using it). Packing the panel only has an effect if the contentbox doesn't take up the full width. Since the panel doesn't flex, this is unlikely to happen.

> > You don't actually need to provide separate rules with and without
> > :-moz-lwtheme, the idea of CSS is that the more important, specific, or failing
> > that, later rule wins.
> Well, all the rules are needed, I just though that specifying the :not() would
> make it s bit clearer and would increase speed by not applying at all where not
> needed.
Ah, but they're not all needed, since by removing the :not() you get an existing rule, and you can therefore apply the style there.

> > >-#security-button > .statusbarpanel-icon {
> > >+#security-button {
> > If you're going to change this to use CSS inheritance then you may as well do
> > it for the other styles you're changing too. But it would be slightly neater to
> > inherit from the contentbox as that way you can set the image and background at
> > the same time.
> Does that work?
Sure, otherwise stefanh wouldn't have used it as a shortcut to set the icon ;-)

> I used that actually to make things consistent between our different themes
> (classic, mac, and Modern) as before my patch we are inconsistent between them.
Yeah, my fault for not spotting stefanh's change.

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > >diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml
> > I couldn't find any existing XBL file in common that would fit better.
> There are no XBL files where it fits, except per-theme, where odds and ends get
> dumped into globalBindings.xml (Modern) or communicatorBindings.xml (Classic).

Hmm, I feel better with having this in content, so I guess we don't care about perf penalties of that kind and I'll create yet another tiny XBL file (if we only could make statusbar be a toolbar in an odd location, then throwing it in toolbar would make sense).

> The -moz-box-align rule makes the contentbox the full height of the panel,
> which you may or may not want. (I have a slight preference for not using it).

Ah, OK. Well, I have a preference for the contentbox actually spanning the whole panel, so I guess we should leave that.

> Ah, but they're not all needed, since by removing the :not() you get an
> existing rule, and you can therefore apply the style there.

Ah, OK.

> > > >-#security-button > .statusbarpanel-icon {
> > > >+#security-button {
> > > If you're going to change this to use CSS inheritance then you may as well do
> > > it for the other styles you're changing too. But it would be slightly neater to
> > > inherit from the contentbox as that way you can set the image and background at
> > > the same time.
> > Does that work?
> Sure, otherwise stefanh wouldn't have used it as a shortcut to set the icon ;-)

I meant the "inherit from the contentbox" - which sounds like a good compromise as we can merge rules.

> > I used that actually to make things consistent between our different themes
> > (classic, mac, and Modern) as before my patch we are inconsistent between them.
> Yeah, my fault for not spotting stefanh's change.

Heh, OK.

Comment 8

[Robert Kaiser]

Attachment: v1.1: address comments

I hope this addresses all the comments.

Comment 9

[Robert Kaiser]

Attachment: v1.1: address comments

This is the correct v1.1 patch, the previous one missed the new XBL file.

Comment 10

[[e-mail]]

(In reply to comment #7)
> I meant the "inherit from the contentbox" - which sounds like a good compromise
> as we can merge rules.
Yeah, merging rules is the biggest win. Using :not is only a win if it doesn't get used much (if it's often not true, then you lose out by testing for it).

Comment 11

[[e-mail]]

Comment on attachment 459498 [details] [diff] [review]
v1.1: address comments

>+    <statusbarpanel class="statusbarpanel-backgroundbox"
>                     id="security-button" dir="reverse"
Should get rid of dir="reverse" and put the child nodes in the correct order in our shiny new binding?

>+++ b/suite/common/bindings/general.xml
Nice :-)

>diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml
>-
Oops ;-)

>+  -moz-box-pack: center;
We probably don't need these packs either.

>+#security-button[label] > .statusbarpanel-contentbox > .statusbarpanel-icon,
Don't need this any more, it was only there to set the background.

>+#security-button > .statusbarpanel-contentbox > .statusbarpanel-text {
>   margin: 0px;
>+  color: #FFFFFF;
>+}
>+#security-button[label] > .statusbarpanel-contentbox {
>   background-color: #62C441;
>-  color: #FFFFFF;
>+}
You forgot to leave a blank line between the rules. Also as I mentioned on IRC we could do with disabling text shadow. Finally it might also be slightly neater to swap these rules so that the background colours keep together.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> >+    <statusbarpanel class="statusbarpanel-backgroundbox"
> >                     id="security-button" dir="reverse"
> Should get rid of dir="reverse" and put the child nodes in the correct order in
> our shiny new binding?

I actually thought about that, but as I decided to implement this as a very generic panel which we could reuse anywhere else easily, I decided to leave the binding in the default statusbarpanel order. As you wrote this as a question, I took it that you were not sure yourself, so I left it as-is in the checked in patch. If you still want this, I can switch it in a followup checkin.

Pushed as http://hg.mozilla.org/comm-central/rev/f8b17dfbdd95