1491840 - PopupNotifications doesn't handle anchors outside the iconbox very well

Status: NEW

(Toolkit :: PopupNotifications and Notification Bars, enhancement, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Basic support for this was added in bug 1130356 (which I've set this bug to block). While trying to implement a popup notification that anchors to a page-action item, I ran into some problems with the way the anchor/notifications are updated - the anchor doesn't get hidden properly when switching tabs.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Try to get anchor element by ID first

Probably first part of a series of improvements.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d868e794f201c9354e9fb6d22a6de5a9adcc4b3d

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Simply anchorElement getter

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Simplify anchorElement getter; hide previous tab's icons reliably

getAnchorFromBrowser is completely unnecessary as far as I can tell, there are no existing consumers of its specialties. I've removed it in this patch.

This patch also fixes the way _update() handles icons - previously, it ONLY hid the icon box icons (before re-showing the relevant ones) IFF none of the currentNotifications had an anchor outside it. This means that if there was a notification with an anchor elsewhere, irrelevant icons would remain visible.

Now, icons are hidden in the TabSelect event handler, since that's the only place where we actually need to do it and where we have information about which icons we need to hide. _update() simply handles the *newly* selected tab's notifications.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea0caa6fec970651c90b421f744c1ffb9d063c3

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9009994 [details] [diff] [review]
Simplify anchorElement getter; hide previous tab's icons reliably

Review of attachment 9009994 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/PopupNotifications.jsm
@@ -39,5 @@
>  
> -function getAnchorFromBrowser(aBrowser, aAnchorID) {
> -  let attrPrefix = aAnchorID ? aAnchorID.replace("notification-icon", "") : "";
> -  let anchor = aBrowser.getAttribute(attrPrefix + ICON_ANCHOR_ATTRIBUTE) ||
> -               aBrowser[attrPrefix + ICON_ANCHOR_ATTRIBUTE] ||

This is used by contextual feature recommender now.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

I've lost context.