823443 - Provide an easy way to add custom content to popup notifications

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

We should provide a way to add custom content to popup notifications without extending the XBL binding re-creating the entire anonymous content.

See the patch in bug 802421 as an example for how this can be used.

Comment 1

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

Rather than removing the popupnotification from its containing document and then tracking its parent so that we can re-insert it, can't we just cloneNode() it on display, and then just remove it entirely on dismissal?

(It would be useful to have some comments in the patch about how this is supposed to work overall. The logic in _clearPanel is hard to follow.)

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> Rather than removing the popupnotification from its containing document and
> then tracking its parent so that we can re-insert it, can't we just
> cloneNode() it on display, and then just remove it entirely on dismissal?

We'd need to do something to prevent duplicate ids for the popupnotification node and its children. Additionally, it would make modifying the content more fragile, as you'd need to do it at the right time. Currently you don't need to care about this and can just liberally mess with the one and only popupnotification element whenever you want (even while the panel is open).

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

comments added

Comment 4

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

Comment on attachment 694870 [details] [diff] [review]
patch v2

Thanks, the comments are great. This is a much better solution to customizing popupNotification content. We should get bugs on file to fix other cases where we currently extend the base binding.

This comment has a couple typos: "by be hidden when we got it the chrome document"

I think _clearPanel also needs to remove the popupnotification.notification reference to the notification object added in _refreshPanel, otherwise we will "leak" it for the rest of the browsing session. The menu items also reference the "notification" and the "action", but those get removed completely so that shouldn't be a problem.

r=me with those addressed.

Comment 5

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2721852327e8

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2721852327e8

Comment 7

[Florian Bender]

I guess this needs dev-doc-needed? 

Nit: The first added line states `new WeakMap;` (without parentheses). Correct me if I'm wrong, but although this may work, this does not follow the overall style.

Comment 8

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

(In reply to Florian Bender from comment #7)
> I guess this needs dev-doc-needed?

I suppose so. Not sure how to best fit this in to the existing PopupNotifications documentation.

> Nit: The first added line states `new WeakMap;` (without parentheses).
> Correct me if I'm wrong, but although this may work, this does not follow
> the overall style.

We don't really strive for consistency for something as minor as this.