797207 - fix the css styles and classes for popup notification buttons

Status: NEW

(Toolkit :: Themes, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

We want all buttons in a popup notification to have the same styling, but those css classes all refer to menubuttons (the additional buttons were added in bug 754472). They should just be popup-notification-button, with some special cases where type="menu-button".

From https://bugzilla.mozilla.org/show_bug.cgi?id=754472#c30 :
::: browser/base/content/urlbarBindings.xml
@@ +1426,5 @@
> +                   xbl:inherits="src=itemicon"/>
> +        <xul:description class="center-item-label"
> +                         xbl:inherits="xbl:text=itemtext"/>
> +        <xul:spacer flex="1"/>
> +        <xul:button class="popup-notification-menubutton center-item-button"

Drive-by on the XBL: This class name doesn't really make sense because this isn't a menubutton. If we want to apply the same styles, I think it would make more sense to make common popup-notification-button styles, then we can do special things for type="menu-button" where necessary.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Margaret - how's this?
(this just changes the name - I think there may be a bit of other cleanup we also want to do, but that can be done in another bug or another patch on this one)

Comment 2

[:Margaret Leibovic]

Sorry I've been so slow to review here... seeing the size of this patch actually makes me want to reconsider whether or not this is a worthwhile change. After thinking about it more, it feels like a pedantic change with little practical benefit, but that might introduce regressions. What do you think?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Well, I don't think it's that likely to introduce regressions - it's basically a find-and-replace. However, I'm fine leaving it if it's not going to be too confusing to future coders.

Comment 4

[:Margaret Leibovic]

Comment on attachment 669678 [details] [diff] [review]
patch

I'm sorry I let this sit in my queue for so long. By now it's bitrotten :( I don't feel like I work with desktop themeing code often enough anymore to have a strong opinion about this. If you do feel like resurrecting this patch, it would probably be best for a desktop peer to review it, instead of me.