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.
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 email@example.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).
Created attachment 694870 [details] [diff] [review] patch v2 comments added
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+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.
You need to log in before you can comment on or make changes to this bug.