634391 - PopupNotification is not attached to anchor

Status: NEW

(Toolkit :: PopupNotifications and Notification Bars, defect)

Description

[MyKey_]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Build Identifier: Firefox 4.0b11

PopupNotifications.show() does not attach the notification to the supplied anchorId. 

Reproducible: Always

Steps to Reproduce:
1. Execute the JavaScript command to show a notification attached to the addon bar on the bottom:
PopupNotifications.show(gBrowser.selectedBrowser,
  "id",
  "message",
  "addon-bar", /* <- anchorID */
  null, /* mainAction */
  null, /* secondaryActions */
  null /* options */
);

Actual Results:  
The notification is always attached to the original "notification-popup-box".

Expected Results:  
The notification should be attached to the addon bar ("addon-bar") on the bottom.

In PopupNotifications.jsm in line 76, the querySelector function returns null even if the element with the supplied anchorID exists (e.g., "addon-bar", "TabsToolbar", or even "notification-popup-box").

[code]
    anchorElement = this.owner.iconBox.querySelector("#"+this.anchorID);

    if (!anchorElement)
      anchorElement = this.owner.iconBox;
[/code]

Comment 1

[u279076]

Can you please create a test case and attach it to this bug so that we can try to reproduce it?

Thanks.

Comment 2

[MyKey_]

(In reply to comment #1)
> Can you please create a test case and attach it to this bug so that we can try
> to reproduce it?
> 
> Thanks.

You can reproduce this error by following these steps (in this exact order): 

1. Open Firefox 4.0b11
2. Navigate to "about:addons"
3. Open the Web Console (e.g. through Control-Shift-K)
4. Execute the following lines of code in the Web Console command line interpreter:

testMsgPos = function(anchorId) {
	
	// get main window:
	var w = QueryInterface(Components.interfaces.nsIInterfaceRequestor)
					   .getInterface(Components.interfaces.nsIWebNavigation)
					   .QueryInterface(Components.interfaces.nsIDocShellTreeItem)
					   .rootTreeItem
					   .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
					   .getInterface(Components.interfaces.nsIDOMWindow);
	
	var nfId = anchorId + "_notification_id";
	var nfMsg = "This message should be attached to \""  + anchorId + "\".";
	
	// display notification attached to anchor:
	var nf = w.PopupNotifications.show(w.gBrowser.selectedBrowser,
	  nfId,
	  nfMsg,
	  anchorId, /* <- anchorId */
	  null, /* mainAction */
	  null, /* secondaryActions */
	  null /* options */
	);
	
	// get anchor element:
	var anchor = nf.anchorElement;
	
	// report errors to firefox error console:
	if(!anchor) console.error( "Notification has no anchor" ); // report the error and continue execution
	else if(anchor.id != anchorId) console.error( "Found anchor.id '" + anchor.id + "' but expected anchorId '" + anchorId + "'!" ); // report the error and continue execution

	// return whether anchor is present and has the expected id:
	return anchor && anchor.id == anchorId;
};

testMsgPos("addon-bar");
testMsgPos("TabsToolbar");
testMsgPos("home-button");



5. You would expect that the 3 notifications would appear in different positions ("addon-bar", "TabsToolbar", and "home-button"). But they don't.
6. The following error messages will be shown in the Web Console:
> [19:50:31.973] "Found anchor.id 'notification-popup-box' but expected anchorId 'addon-bar'!"
> [19:50:31.989] "Found anchor.id 'notification-popup-box' but expected anchorId 'TabsToolbar'!"
> [19:50:32.006] "Found anchor.id 'notification-popup-box' but expected anchorId 'home-button'!"

Comment 3

[MyKey_]

Note that the above JavaScript code has two line breaks within the comments, which break the code. 

Before execution, please remove the two comments with the following text:

  //report the
error and continue execution

Sorry for the confusion.

Comment 4

[u279076]

Confirmed using the testcase from comment 2 in Firefox 4.0rc1.

Comment 5

[Mike Kaply [:mkaply]]

Here's the code from PopupNotifications.jsm

let anchorElement = null;
if (this.anchorID)
  anchorElement = this.owner.iconBox.querySelector("#"+this.anchorID);
 
if (!anchorElement)
  anchorElement = this.owner.iconBox;

So because querySelector is called on the iconBox, anchorID only works if the element specified by the anchorID is a child of the iconBox.

should be:

anchorElement = this.owner.iconBox.ownerDocument.querySelector("#"+this.anchorID);

Comment 6

[John]

When this will be fixed?
Can i do like that to fix it?

Components.utils.import("chrome://content/PopupNotifications.jsm");
PopupNotifications.show(gBrowser.selectedBrowser, "sample-popup",
        "This is a sample popup notification.",
        "MY-ANCHER-ID", /* anchor ID */
        {
          label: "Do Something",
          accessKey: "D",
          callback: function() {
            alert("Doing something awesome!");
          }
        },
        null  /* secondary action */
        );	

where chrome://content/PopupNotifications.jsm - is file with fixed code?

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Use iconBox ownerDocument for query

Because querySelector is called on the iconBox, anchorID only works if the element specified by the anchorID is a child of the iconBox.

querySelector should be called on the ownerDocument of the iconBox

Comment 8

[John]

In comment above - chrome://content/PopupNotifications.jsm has 

anchorElement = this.owner.iconBox.ownerDocument.querySelector("#"+this.anchorID);

but it still not works - attached to iconBox, did i misunderstanding something?

Comment 9

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

Comment on attachment 538325 [details] [diff] [review]
Use iconBox ownerDocument for query

This was intentional - the _onIconBoxCommand code won't work for elements outside of the iconBox.

We need a better solution to allow PopupNotifications anchored elsewhere, but I haven't had much time to come up with one. Bug 604473 takes the approach of just creating a separate PopupNotifications object, which may be a suitable workaround in the interim.

Comment 10

[Mike Kaply [:mkaply]]

The problem is people are reading the documentation:

https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm

And it indicates that the popup can be attached arbitrarily.

If this:

anchorID
The ID of the element that should serve as the anchor for the notification popup (that is, the element the arrow on the popup should point to). If you specify null, the notification will be anchored to the icon box at the start of the URL bar.

Can only ever be the iconBox, we need to change the docs.

Comment 11

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

It can any element, it just needs to be a descendant of the iconBox.

Comment 12

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

I've updated https://developer.mozilla.org/en/JavaScript_code_modules/PopupNotifications.jsm .

Comment 13

[Matthew N. [:MattN]]

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.