574230 - PopupNotifications should apply some attribute to its icon box

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dave Townsend [:mossop]]

It strikes me that we may want many notifications all attached to the same place that the geo ones are attached. Currently anything using the same PopupNotifications gets a geo icon there. If PopupNotifications added an attribute to the box as it unhides it, say popupid, then we'd be able to pick and choose what icon to show there.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This was the intent, yes, I just didn't follow through completely.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 3

[Dão Gottwald [:dao]]

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

>+#notification-popup-box > * {

Can we have a common class name for these nodes?

Comment 4

[Robert Kaiser]

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

>+#notification-popup-box > * {

I agree with dao, can we please have a common class set on those?

Also, any reason why those CSS rules are not in immediate vicinity to the existing #notification-popup-box rule?

>+  // Test that anchor icon appears
>+  { // Test #11

Why "Test #11" when the one immediately before it is "Test #9"?

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> Why "Test #11" when the one immediately before it is "Test #9"?

Oops, I didn't seem to have bug 574228 locally yet, so ignore that one.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I'm not sure I understand why you want the class. Is it just for the selector efficiency? All items in the notification-popup-box should be treated the same, so the downside to using a class is that it requires remembering to add the class when making additions to the box (whereas if you forget about the styling with the current patch, you won't see the added icon at all).

If the perf difference was significant I'd be all for it, but I don't think it is, so I don't really see any advantages to using a class name.

Comment 7

[Robert Kaiser]

IIRC, any * at the last place in the cascade (i.e. the first one that is checked by the style system, AFAIK) is always to be avoided as it's slowing down all style lookups in that scope.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: with class

Yes, I know the efficiency differences. My impression is that they're insignificant and thus outweighed by my maintainability argument in comment 6, but I don't feel strongly. I'll let Dao make the call.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Er, oops, that's just an interdiff. r+ one or both, I guess!

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 453954 [details] [diff] [review]
with class

I don't know whether the difference in efficiency is significant, but I'm leaning towards saving the CPU cycles either way. It seems that if you miss the class and the icon never hides, you'll notice this right away, and in most cases you'd probably just copy an existing icon and change the id anyway.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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