1162635 - Remove outset border from notification bars

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Notification Bar.png

Notification bars on Windows (all versions) have a weird beveled border that makes them look like a button.

Let's remove that!

The solution seems to be to remove the .outset class from hbox.notification-inner and add border-bottom: 1px solid #999 instead.

Comment 1

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #0)
> Created attachment 8602852 [details]
> Notification Bar.png
> 
> Notification bars on Windows (all versions) have a weird beveled border that
> makes them look like a button.
> 
> Let's remove that!
> 
> The solution seems to be to remove the .outset class from
> hbox.notification-inner and add border-bottom: 1px solid #999 instead.

Right, except that will affect toolkit...

Dão, do you think we can just change this in toolkit? If not, should we just override all of this in shared browser css code?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I think we should make this change in toolkit. This is a good instance where we can make toolkit styling more "modern" without much cost, rather than keeping an outdated look around and overriding it all over the place.

Comment 3

[Dão Gottwald [:dao]]

We can't do this exactly as suggested. Notification bars can be at the bottom of the content area (even ignoring other toolkit applications), so we can't just set border-bottom for all of them.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> We can't do this exactly as suggested. Notification bars can be at the
> bottom of the content area (even ignoring other toolkit applications), so we
> can't just set border-bottom for all of them.

Seems like we should keep both border-bottom and border-top (but as a flat 1px border instead of the inset/outset) and override one of the two depending on where the notifications are. Note that right now we also show the outset border-bottom on the bottom notificationbar on Windows, and that looks even worse.

Comment 5

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

This uses ThreeDShadow, which is a tiny bit lighter than #999 for me on win8, but also still looks dark-ish in comparison with the bottom border of the navbar. In any case, I think this is an improvement over the current state of things... Is this what you had in mind?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8665918 [details] [diff] [review]
flatten notification styles on Windows and Linux,

I think we should probably support an attribute on the notificationbox element that makes the toolkit stylesheets take care of removing the top or bottom border.

Comment 7

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

Comment 8

[:Gijs (he/him)]

I'll take suggestions for the name of the attribute, but this made sense to me.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8665954 [details] [diff] [review]
flatten notification styles on Windows and Linux,

>--- a/toolkit/themes/linux/global/notification.css
>+++ b/toolkit/themes/linux/global/notification.css
>@@ -20,16 +20,26 @@ notification[type="info"] {
> notification[type="critical"] {
>   color: white;
>   background-image: linear-gradient(rgb(212,0,0), rgb(152,0,0));
> }
> 
> .notification-inner {
>   padding-top: 1px;
>   padding-bottom: 1px;
>+  border-top: 1px solid ThreeDShadow;
>+  border-bottom: 1px solid ThreeDShadow;
>+}

On Linux we use -moz-appearance: -moz-gtk-info-bar for warning level notifications, which already renders a border, resulting in a double border. If you set the CSS border on the notification element rather than .notification-inner, this should "just work", because that border will be ignored when -moz-appearance is used.

Maybe we can remove .notification-inner altogether? Looks might it might be useless at this point.

>+notificationbox[notificationside="top"] .notification-inner {
>+  border-top-style: none;
>+}

Please use the child selector.

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #9)
> Maybe we can remove .notification-inner altogether? Looks might it might be
> useless at this point.

That seems like a bigger can of worms with add-on and much more toolkit consumer impact, though? I can look into it if you feel strongly, but it seems likely to be more complex and break more stuff. I'd prefer to not scope-creep this bug that far...

Comment 11

[Dão Gottwald [:dao]]

I wouldn't expect a large number of extensions to use this thing and I don't think we should keep it around for maybe one or two obscure ones. Themes might style it like we do, but they should fail gracefully, i.e. they'll likely just lose the border.

However, this can be spun off to a separate bug.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #11)
> Themes
> might style it like we do, but they should fail gracefully, i.e. they'll
> likely just lose the border.

(Most will probably depend on the outset class and lose the border anyway.)

Comment 13

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

So, like this?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8666790 [details] [diff] [review]
flatten notification styles on Windows and Linux,

Review of attachment 8666790 [details] [diff] [review]:
-----------------------------------------------------------------

d'oh.

Comment 15

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #9)
> On Linux we use -moz-appearance: -moz-gtk-info-bar for warning level
> notifications, which already renders a border

Actually, testing this, I can't reproduce this... presumably this is dependent on the GTK theme or something?

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > On Linux we use -moz-appearance: -moz-gtk-info-bar for warning level
> > notifications, which already renders a border
> 
> Actually, testing this, I can't reproduce this... presumably this is
> dependent on the GTK theme or something?

... and in fact, I can't seem to override the border on that style (for <notification>), so now our own custom borders aren't showing up (ie the moz-appearance border none is overriding mine, even when I use !important). :-\

How is this working right now, considering that notification-inner already has a border (the outset one), and that border isn't causing a double border for you (apparently) ? Because it kind of sounds like keeping the border on notification-inner wouldn't actually change anything here, and trying to put it on <notification> would break something.

Comment 17

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > (In reply to Dão Gottwald [:dao] from comment #9)
> > > On Linux we use -moz-appearance: -moz-gtk-info-bar for warning level
> > > notifications, which already renders a border
> > 
> > Actually, testing this, I can't reproduce this... presumably this is
> > dependent on the GTK theme or something?

What GTK theme do you use? I'm using the default theme on Ubuntu.

> ... and in fact, I can't seem to override the border on that style (for
> <notification>), so now our own custom borders aren't showing up (ie the
> moz-appearance border none is overriding mine, even when I use !important).
> :-\

Yes, that's how -moz-appearance works.

> How is this working right now, considering that notification-inner already
> has a border (the outset one), and that border isn't causing a double border
> for you (apparently) ?

It already causes a double border.

Comment 18

[Dão Gottwald [:dao]]

Andrew or Karl, could you please comment on whether -moz-gtk-info-bar is supposed to render a border?

Comment 19

[Andrew Comminos [:acomminos]]

It depends on the native theme; currently, we will draw a border frame for -moz-gtk-info-bar if the GTK3 theme provides one, exactly like GTK's implementation of GtkInfoBar. There may certainly exist themes that don't provide any sort of border decoration for info bars, in which case I think we should respect that.

Comment 20

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

Right, so, in that case I think this is what you asked for?

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8673720 [details] [diff] [review]
flatten notification styles on Windows and Linux,

>--- a/toolkit/themes/linux/global/notification.css
>+++ b/toolkit/themes/linux/global/notification.css
>@@ -4,16 +4,18 @@
> 
> @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> notification {
>   color: -moz-gtk-info-bar-text;
>   background-color: InfoBackground;
>   -moz-appearance: -moz-gtk-info-bar;
>   text-shadow: none;
>+  border-top: 1px solid ThreeDShadow;
>+  border-bottom: 1px solid ThreeDShadow;
> }

I believe -moz-gtk-info-bar is guaranteed to render something, so the top and bottom borders won't ever be used and you can remove them. Same for background-color.

>+notificationbox[notificationside="top"] .notification-inner {
>+  border-top-style: none;
>+}
>+
>+notificationbox[notificationside="bottom"] .notification-inner {
>+  border-bottom-style: none;
>+}

This is dead code too.

>--- a/toolkit/themes/windows/global/notification.css
>+++ b/toolkit/themes/windows/global/notification.css

>+.notification-inner {
>+}
>+
>+

ditto

Comment 22

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #21)
> Comment on attachment 8673720 [details] [diff] [review]
> flatten notification styles on Windows and Linux,
> 
> >--- a/toolkit/themes/linux/global/notification.css
> >+++ b/toolkit/themes/linux/global/notification.css
> >@@ -4,16 +4,18 @@
> > 
> > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> > 
> > notification {
> >   color: -moz-gtk-info-bar-text;
> >   background-color: InfoBackground;
> >   -moz-appearance: -moz-gtk-info-bar;
> >   text-shadow: none;
> >+  border-top: 1px solid ThreeDShadow;
> >+  border-bottom: 1px solid ThreeDShadow;
> > }
> 
> I believe -moz-gtk-info-bar is guaranteed to render something, so the top
> and bottom borders won't ever be used and you can remove them. Same for
> background-color.

Hm, I actually left the background-color. But thinking about this, that would mean that we don't control the background color of the notification anymore, and can't use it to indicate severity. Is that right? That seems not desirable, tbh...

Comment 24

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #23)
> (In reply to Dão Gottwald [:dao] from comment #21)
> > Comment on attachment 8673720 [details] [diff] [review]
> > flatten notification styles on Windows and Linux,
> > 
> > >--- a/toolkit/themes/linux/global/notification.css
> > >+++ b/toolkit/themes/linux/global/notification.css
> > >@@ -4,16 +4,18 @@
> > > 
> > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> > > 
> > > notification {
> > >   color: -moz-gtk-info-bar-text;
> > >   background-color: InfoBackground;
> > >   -moz-appearance: -moz-gtk-info-bar;
> > >   text-shadow: none;
> > >+  border-top: 1px solid ThreeDShadow;
> > >+  border-bottom: 1px solid ThreeDShadow;
> > > }
> > 
> > I believe -moz-gtk-info-bar is guaranteed to render something, so the top
> > and bottom borders won't ever be used and you can remove them. Same for
> > background-color.
> 
> Hm, I actually left the background-color. But thinking about this, that
> would mean that we don't control the background color of the notification
> anymore, and can't use it to indicate severity. Is that right? That seems
> not desirable, tbh...

We don't control what InfoBackground resolves to either, so this isn't new. We do hardcode the colors for type="info" and type="critical", though. It's a bit confusing... we use -moz-gtk-info-bar for type="warning" because -moz-gtk-info-bar tends to be yellow which is what we want for type="warning" but not for type="info".

Comment 25

[:Gijs (he/him)]

Attachment: flatten notification styles on Windows and Linux,

OK. So now without background-color, too...

Comment 26

[Karl Tomlinson (:karlt)]

GtkInfoBar comes in https://developer.gnome.org/gtk3/stable/GtkMessageDialog.html#GtkMessageType flavors.

-moz-gtk-info-bar is implemented with the default "info" flavor.
Warning and error variants could be implemented if desired.

Comment 27

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #26)
> GtkInfoBar comes in
> https://developer.gnome.org/gtk3/stable/GtkMessageDialog.html#GtkMessageType
> flavors.
> 
> -moz-gtk-info-bar is implemented with the default "info" flavor.
> Warning and error variants could be implemented if desired.

Depending on how popular gtk themes style these different flavors, this might indeed be a good idea.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8673750 [details] [diff] [review]
flatten notification styles on Windows and Linux,

>+notificationbox[notificationside="top"] > notification {
>+  border-top-style: none;
>+}
>+
>+notificationbox[notificationside="bottom"] > notification {
>+  border-bottom-style: none;
> }

Should we do this on OS X too? We have top and bottom borders there, but the colors are quite different, so they may not look as broken as on Windows.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ee52176c415e

Comment 30

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #28)
> Comment on attachment 8673750 [details] [diff] [review]
> flatten notification styles on Windows and Linux,
> 
> >+notificationbox[notificationside="top"] > notification {
> >+  border-top-style: none;
> >+}
> >+
> >+notificationbox[notificationside="bottom"] > notification {
> >+  border-bottom-style: none;
> > }
> 
> Should we do this on OS X too? We have top and bottom borders there, but the
> colors are quite different, so they may not look as broken as on Windows.

Yeah, I'm actually not even sure what the story is on OS X. Neither border seems particularly noticeable. We also seem to be using images for the background which could be represented as linear-gradients instead, and which we may or may not want to invert if the bar is on the other (which?) side. I'll file a followup bug and needinfo shorlander.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee52176c415e

Comment 32

[:Gijs (he/him)]

Giorgio, as Dão noted in bug 1215335, we suspect noscript is impacted by this fix (from code inspection; we could be wrong).

Comment 33

[Giorgio Maone [:ma1]]

(In reply to :Gijs Kruitbosch from comment #32)
> Giorgio, as Dão noted in bug 1215335, we suspect noscript is impacted by
> this fix (from code inspection; we could be wrong).

It's a legacy, backward-compatibility code path.
Feel free to proceed.

Comment 34

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 40.0a1 (2015-05-07)
with windows 8 (32 bit)
Build ID :20150507030203
Mozilla/5.0 (Windows NT 6.3; rv:40.0) Gecko/20100101 Firefox/40.0

I found this fix on latest aurora 44.0a2 (2015-12-11)

Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID :20151211004013

[testday-20151211]

Comment 35

[:Gijs (he/him)]

Thanks!