Closed Bug 823443 Opened 7 years ago Closed 7 years ago

Provide an easy way to add custom content to popup notifications

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #694279 - Flags: review?(gavin.sharp)
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.)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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).
Attached patch patch v2Splinter Review
comments added
Attachment #694279 - Attachment is obsolete: true
Attachment #694279 - Flags: review?(gavin.sharp)
Attachment #694870 - Flags: review?(gavin.sharp)
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.
Attachment #694870 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/2721852327e8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 615483
No longer blocks: 615483
Blocks: 824443
Blocks: 824947
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.
(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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.