577931 - Mac OS X specific styling of menu buttons in popup notifications

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Peter Henkel [:Terepin]]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; sk; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; sk; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre

Doorhanger is using dark grey pop-up with rounded buttons.

Reproducible: Always

Comment 1

[:Margaret Leibovic]

Attachment: WIP

First attempt at refining the theme.

One issue is that the default dropmarker (http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/dropmarker.css#13) is black. Should we create a new image for that? Also, I just used the @hudButton@ style that was used in the identity notifications, even though that doesn't exactly match the button style in the mockup.

Comment 2

[:Margaret Leibovic]

Attachment: WIP screenshot

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Should we create a new image for that?

Yep.

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #1)
> Also, I just used the
> @hudButton@ style that was used in the identity notifications, even though that
> doesn't exactly match the button style in the mockup.

It would be great if you could change the @hudButton@ style to match the mockup in bug 573599. I signed up for this originally but haven't gotten to it yet (and might not for several weeks), so I'd really appreaciate it if you could do it.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: White drop down arrow

(In reply to comment #3)
> (In reply to comment #1)
> > Should we create a new image for that?
> 
> Yep.

Comment 6

[:Margaret Leibovic]

Attachment: patch

Polished doorhanger styling. I also updated some of the styles for the edit bookmarks panel to avoid regressions with the new hudButton styles.

Comment 7

[:Margaret Leibovic]

Attachment: screenshot from build with patch

I had a tough time trying to get the divider in the middle of the button to be styled correctly because of the elements inside the main hud style button. I feel like it would require messing with the hud style to make a new menubutton hud style.

Comment 8

[:Margaret Leibovic]

Also, in the patch I moved around some of the css declarations to make them more organized/easy to follow.

Comment 9

[:Margaret Leibovic]

Attachment: patch (with new arrow image)

updated patch to include new arrow image

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 469237 [details] [diff] [review]
patch (with new arrow image)

This makes hud-style-button-middle-background.png unused, please remove it.
hud-style-button-middle-background-active.png is still used in one place, but it looks like @hudButtonPressed@ should be used there.

>+.popup-notification-description {
>+  margin-bottom: 15px;
>+  margin-left: 5px;

-moz-margin-start?

>   skin/classic/global/arrow/arrow-dn-dis.gif                         (arrow/arrow-dn-dis.gif)
>   skin/classic/global/arrow/arrow-dn-dis.png                         (arrow/arrow-dn-dis.png)
>   skin/classic/global/arrow/arrow-dn-sharp.gif                       (arrow/arrow-dn-sharp.gif)
>   skin/classic/global/arrow/arrow-dn.gif                             (arrow/arrow-dn.gif)
>   skin/classic/global/arrow/arrow-dn.png                             (arrow/arrow-dn.png)
>+  skin/classic/global/arrow/arrow-dn-white.png                       (arrow/arrow-dn-white.png)

Since the arrow size varies depending on the context, I'm not sure this is going to be generally useful. Maybe put it in browser/ as hud-button-dropdown.png?

Comment 11

[:Margaret Leibovic]

Attachment: updated patch w/ moved arrow image

Comment 12

[:Margaret Leibovic]

Attachment: updated patch w/ moved arrow image

Oops, forgot to hg add image.

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Split Button and Menu

This looks great Margaret. I noticed two things: Split buttons don't retain their depressed state when they are active and the menu highlighted text should be white instead of black.

Comment 14

[:Margaret Leibovic]

(In reply to comment #13)
> Split buttons don't retain their depressed state when they are active

This is an easy fix.

> and the menu highlighted text should be white instead of black.

I'm having trouble hunting down the root of this problem. Shouldn't the menuitem widget styling be taking care of this? Dao, do you have any ideas?

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> > and the menu highlighted text should be white instead of black.
> 
> I'm having trouble hunting down the root of this problem. Shouldn't the
> menuitem widget styling be taking care of this? Dao, do you have any ideas?

It should: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/menu.css#168
I don't see offhand what's going wrong.

Comment 16

[Markus Stange [:mstange]]

I don't see it either. It's a little hard to debug because you can only change the popup's contents with the DOM Inspector while the popup is closed, and as soon as you reopen it, everything gets regenerated.

Maybe Neil can find out what's wrong.

Comment 17

[:Margaret Leibovic]

Has anyone made any progress on this issue? It would be nice to resolve this soon.

Comment 18

[Markus Stange [:mstange]]

Let's file a new bug on the menu highlight color part; it's not caused by this patch. You can just address Stephen's other comment and request review again.

Comment 19

[:Margaret Leibovic]

Attachment: new patch v1

Un-bitrotted and fixed depressed state. I filed bug 600546 for the menuitem highlight issue.

Comment 21

[:Margaret Leibovic]

Comment on attachment 479508 [details] [diff] [review]
new patch v1

Better patch on the way!

Comment 22

[:Margaret Leibovic]

Attachment: new patch v2

I made the split buttons actually behave the way they're supposed to. I also put in some fixes for rtl locales.

Also, with this patch I'm not noticing bug 595201 anymore (and I made sure my patch for that bug wasn't applied ;)

Comment 23

[:Margaret Leibovic]

Attachment: screenshot (new patch v2)

Comment 24

[:Margaret Leibovic]

Comment on attachment 480014 [details] [diff] [review]
new patch v2

Bug 554937 broke the margin/padding, so this patch needs updating.

Comment 25

[:Margaret Leibovic]

Attachment: patch v3

I increased the arrow panel padding to address https://bugzilla.mozilla.org/show_bug.cgi?id=554937#c51.

Comment 26

[:Margaret Leibovic]

Attachment: patch v3 screenshot

Comment 28

[:Margaret Leibovic]

Attachment: patch v4

I moved the arrow panel alignment style to toolkit because https://bugzilla.mozilla.org/show_bug.cgi?id=577928#c11 also applies to pinstripe. Also, we don't need to style the text color for the panel anymore because arrow panels do that for us.

Comment 29

[:Margaret Leibovic]

Attachment: patch v4.1

Oops, we don't need that color: MenuText I added when debugging.

Comment 30

[Stephen Horlander [:shorlander]]

(In reply to comment #29)
> Created attachment 483453 [details] [diff] [review]
> patch v4.1
> 
> Oops, we don't need that color: MenuText I added when debugging.

You can also change the -moz-box-shadow in the hudbutton* rules to box-shadow now I think.

Comment 31

[:Margaret Leibovic]

Attachment: patch v5

Addressed Stephen's comment about box-shadow.

Comment 32

[:Margaret Leibovic]

Comment on attachment 484737 [details] [diff] [review]
patch v5

Cancelling review request because we'll probably change where we put these styles when we decide what to do in bug 577928.

Comment 33

[:Margaret Leibovic]

Attachment: patch v6

I changed the patch to basically do the same thing as the patch in bug 577928. This patch will depend on the patch in that bug to add notification.css to the popup notification binding.

The older versions of this patch changed the button styles in other panels, but I think that should just be done in bug 597557.

Comment 34

[:Margaret Leibovic]

Attachment: patch v6 (with image)

Oops, I forgot to hg add the arrow image.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 487697 [details] [diff] [review]
patch v6 (with image)

>+.popup-notification-menubutton > .button-menubutton-button,
>+.popup-notification-menubutton > .button-menubutton-dropmarker {
>+  color: #fff;
>+  text-shadow: 0 -1px 0 rgba(0,0,0,.5);
>+  border-radius: 12px;
>+  border: 1px solid rgba(0,0,0,.65) !important;

Would you mind fixing button.css and dropmarker.css so that !important isn't needed here?

Comment 36

[:Margaret Leibovic]

Attachment: patch v7

It looks like I just had to change button.css.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 488484 [details] [diff] [review]
patch v7

>--- a/toolkit/themes/pinstripe/global/button.css
>+++ b/toolkit/themes/pinstripe/global/button.css
>@@ -82,17 +82,17 @@ button[type="menu-button"] {
> .button-menubutton-button {
>   -moz-appearance: none;
>   margin: 0;
> }
> 
> .button-menu-dropmarker,
> .button-menubutton-dropmarker {
>   -moz-appearance: none !important;
>-  border: none !important;
>+  border: none;
>   background-color: transparent !important;
>   margin: 1px;
> }

It seems that !important was used here because dropmarker.css uses !important for dropmarker[disabled="true"].

Comment 38

[:Margaret Leibovic]

Attachment: patch v8

Oops, I missed that.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 488486 [details] [diff] [review]
patch v8

>--- a/toolkit/themes/pinstripe/global/dropmarker.css
>+++ b/toolkit/themes/pinstripe/global/dropmarker.css

> dropmarker[disabled="true"] {
>   list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.gif");
>-  -moz-border-top-colors: ThreeDLightShadow ThreeDHighlight !important;
>-  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow !important;
>-  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow !important;
>-  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight !important;
>+  -moz-border-top-colors: ThreeDLightShadow ThreeDHighlight;
>+  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>+  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>+  -moz-border-left-colors: ThreeDLightShadow ThreeDHighlight;
>   padding: 1px !important;
> }

This is actually more bogus than it seemed at first. Please completely remove the -moz-border-*-colors and padding properties and add :not([disabled="true") to the dropmarker:hover:active rule above.

>+.popup-notification-menubutton > .button-menubutton-button {
>+  padding: 2px 0;
>+  -moz-border-end: 1px solid rgba(0,0,0,.65);
>+  -moz-padding-start: 8px;
>+  -moz-padding-end: 5px;
>+}

replace padding: 2px 0 with padding-top/bottom.

>+.popup-notification-menubutton > .button-menubutton-dropmarker {
>+  padding: 7px 0;
>+  -moz-padding-start: 5px;
>+  -moz-padding-end: 8px;
>+  -moz-margin-start: -1px;
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-white.png");
>+}

ditto

Comment 40

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/c609be50b0b2