Provide an easy way to add custom content to popup notifications

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({dev-doc-needed})

Trunk
mozilla20
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 694279 [details] [diff] [review]
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.
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.)
(Assignee)

Comment 2

6 years ago
(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).
(Assignee)

Comment 3

6 years ago
Created attachment 694870 [details] [diff] [review]
patch v2

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Updated

6 years ago
Blocks: 615483
(Assignee)

Updated

6 years ago
No longer blocks: 615483
(Assignee)

Updated

6 years ago
Blocks: 824443
(Assignee)

Updated

6 years ago
Blocks: 824947

Comment 7

6 years ago
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.