Closed Bug 577931 Opened 10 years ago Closed 9 years ago

Mac OS X specific styling of menu buttons in popup notifications

Categories

(Toolkit :: Themes, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Terepin, Assigned: Margaret)

References

Details

(Whiteboard: [doorhanger])

Attachments

(4 files, 17 obsolete files)

210 bytes, image/png
Details
21.66 KB, image/png
Details
48.74 KB, image/png
Details
7.60 KB, patch
dao
: review+
Details | Diff | Splinter Review
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
Blocks: 577927
Version: unspecified → Trunk
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
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.
Attached image WIP screenshot (obsolete) —
(In reply to comment #1)
> Should we create a new image for that?

Yep.
(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.
Attached image White drop down arrow
(In reply to comment #3)
> (In reply to comment #1)
> > Should we create a new image for that?
> 
> Yep.
Attached patch patch (obsolete) — Splinter Review
Polished doorhanger styling. I also updated some of the styles for the edit bookmarks panel to avoid regressions with the new hudButton styles.
Attachment #468863 - Attachment is obsolete: true
Attached image screenshot from build with patch (obsolete) —
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.
Attachment #468864 - Attachment is obsolete: true
Also, in the patch I moved around some of the css declarations to make them more organized/easy to follow.
Attached patch patch (with new arrow image) (obsolete) — Splinter Review
updated patch to include new arrow image
Attachment #469221 - Attachment is obsolete: true
Attachment #469237 - Flags: ui-review?(shorlander)
Attachment #469237 - Flags: review?(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?
Attachment #469237 - Attachment is obsolete: true
Attachment #471335 - Flags: ui-review?(shorlander)
Attachment #471335 - Flags: review?(dao)
Attachment #469237 - Flags: ui-review?(shorlander)
Attachment #469237 - Flags: review?(dao)
Oops, forgot to hg add image.
Attachment #471335 - Attachment is obsolete: true
Attachment #471337 - Flags: ui-review?(shorlander)
Attachment #471337 - Flags: review?(dao)
Attachment #471335 - Flags: ui-review?(shorlander)
Attachment #471335 - Flags: review?(dao)
Attached image 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.
(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?
(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.
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.
Has anyone made any progress on this issue? It would be nice to resolve this soon.
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.
Depends on: 600546
Attached patch new patch v1 (obsolete) — Splinter Review
Un-bitrotted and fixed depressed state. I filed bug 600546 for the menuitem highlight issue.
Attachment #471337 - Attachment is obsolete: true
Attachment #479508 - Flags: ui-review?(shorlander)
Attachment #479508 - Flags: review?(dao)
Attachment #471337 - Flags: review?(dao)
blocking2.0: --- → betaN+
Flags: in-litmus?(abillings)
Duplicate of this bug: 599355
Comment on attachment 479508 [details] [diff] [review]
new patch v1

Better patch on the way!
Attachment #479508 - Flags: review?(dao)
Attached patch new patch v2 (obsolete) — Splinter Review
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 ;)
Attachment #479508 - Attachment is obsolete: true
Attachment #480014 - Flags: review?(dao)
Attached image screenshot (new patch v2) (obsolete) —
Attachment #469223 - Attachment is obsolete: true
Attachment #480015 - Flags: ui-review?(shorlander)
Whiteboard: [needs view dao]
Whiteboard: [needs view dao] → [needs review dao]
Comment on attachment 480014 [details] [diff] [review]
new patch v2

Bug 554937 broke the margin/padding, so this patch needs updating.
Attachment #480014 - Flags: review?(dao)
Whiteboard: [needs review dao]
Attached patch patch v3 (obsolete) — Splinter Review
I increased the arrow panel padding to address https://bugzilla.mozilla.org/show_bug.cgi?id=554937#c51.
Attachment #480014 - Attachment is obsolete: true
Attachment #483225 - Flags: review?(dao)
Attached image patch v3 screenshot
Attachment #480015 - Attachment is obsolete: true
Attachment #480015 - Flags: ui-review?(shorlander)
Whiteboard: [needs review dao]
Duplicate of this bug: 462652
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #483225 - Attachment is obsolete: true
Attachment #483452 - Flags: review?(dao)
Attachment #483225 - Flags: review?(dao)
Attached patch patch v4.1 (obsolete) — Splinter Review
Oops, we don't need that color: MenuText I added when debugging.
Attachment #483452 - Attachment is obsolete: true
Attachment #483453 - Flags: review?(dao)
Attachment #483452 - Flags: review?(dao)
(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.
Attached patch patch v5 (obsolete) — Splinter Review
Addressed Stephen's comment about box-shadow.
Attachment #483453 - Attachment is obsolete: true
Attachment #484737 - Flags: review?(dao)
Attachment #483453 - Flags: review?(dao)
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.
Attachment #484737 - Flags: review?(dao)
Whiteboard: [needs review dao]
Attached patch patch v6 (obsolete) — Splinter Review
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.
Attachment #484737 - Attachment is obsolete: true
Attachment #487678 - Flags: review?(dao)
Attached patch patch v6 (with image) (obsolete) — Splinter Review
Oops, I forgot to hg add the arrow image.
Attachment #487678 - Attachment is obsolete: true
Attachment #487697 - Flags: review?(dao)
Attachment #487678 - Flags: review?(dao)
Whiteboard: [needs review 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?
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
Summary: Mac OS X specific styling of Doorhanger → Mac OS X specific styling of menu buttons in popup notifications
Attached patch patch v7 (obsolete) — Splinter Review
It looks like I just had to change button.css.
Attachment #487697 - Attachment is obsolete: true
Attachment #488484 - Flags: review?(dao)
Attachment #487697 - Flags: review?(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"].
Attached patch patch v8Splinter Review
Oops, I missed that.
Attachment #488484 - Attachment is obsolete: true
Attachment #488486 - Flags: review?(dao)
Attachment #488484 - Flags: review?(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
Attachment #488486 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/c609be50b0b2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review dao]
Depends on: 610123
Whiteboard: [doorhanger]
Status: RESOLVED → VERIFIED
Flags: in-litmus?(abillings) → in-litmus?
You need to log in before you can comment on or make changes to this bug.