624843 - PopupNotifications are horribly broken if the location bar is removed

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

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

...since there is no iconBox in that case, and so the constructor throws. It should handle a null iconBox gracefully (by not throwing, at least - bug 617553 will still cause weirdness).

Comment 1

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

"if the location bar is removed" is really "if the location bar is added or removed for any reason, including e.g. someone using "restore default set".

Comment 2

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

Attachment: patch

This patch makes things continue to work after customization as long as the URL bar is there. It also fixes the the case where the URL bar is completely removed, but that case is not ideal since we no longer anchor the popup to anything. That is easy enough to fix once bug 617553 is landed, because we'll already have a fallback path to using the tab as an anchor.

Comment 3

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

Attachment: patch

Main idea is this:
- make the consumer responsible for keeping iconBox reference fresh across any possible customizations etc.
- have the PopupNotifications code handle having a null iconBox (for the URL-bar removed case)
- fall back to the tab-anchoring case added by bug 617553 if the anchorElement is null, which can happen for anchor-less notifications, which fall back to the iconBox as an anchor.

The browser_popupNotification change is mostly unrelated debugging code that is useful for debugging that test, which I left it because it's useful.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 504945 [details] [diff] [review]
patch

>+  this._onIconBoxCommand = this._onIconBoxCommand.bind(this);
>+  this.iconBox = iconBox;
>+
>   let self = this;
>-  this.iconBox.addEventListener("click", function (event) {
>-    self._onIconBoxCommand(event);
>-  }, false);
>-  this.iconBox.addEventListener("keypress", function (event) {
>-    self._onIconBoxCommand(event);
>-  }, false);
>+
>   this.panel.addEventListener("popuphidden", function (event) {
>     self._onPopupHidden(event);
>   }, true);

Want to sprinkle some bind() pixie dust here too?

>   function updateFromListeners() {
>     // setTimeout(..., 0) needed, otherwise openPopup from "activate" event
>     // handler results in the popup being hidden again for some reason...
>     self.window.setTimeout(function () {
>       self._update();
>     }, 0);
>   }
>   this.window.addEventListener("activate", updateFromListeners, true);
>   this.tabbrowser.tabContainer.addEventListener("TabSelect", updateFromListeners, true);

Ooooh, could do some fun binding here too.

let update = this._update.bind(this);
let spinUpdate = this.window.setTimeout.bind(this, update, 0);
addEventListener("foo", spinUpdate, true);


> PopupNotifications.prototype = {
>+  set iconBox(iconBox) {
>+    this._iconBox = iconBox;
>+    if (iconBox) {
>+      iconBox.addEventListener("click", this._onIconBoxCommand, false);
>+      iconBox.addEventListener("keypress", this._onIconBoxCommand, false);
>+    }
>+  },

Guess there's no need for removeEventListener() here?

Comment 5

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

(In reply to comment #4)
> >   this.panel.addEventListener("popuphidden", function (event) {

> Want to sprinkle some bind() pixie dust here too?

Done.

> Ooooh, could do some fun binding here too.

> let spinUpdate = this.window.setTimeout.bind(this, update, 0);

Didn't do this, because setTimeout really doesn't seem to like being bound (threw an exception with |this|, and seemed to fail silently with |this.window| as the thisArg).

> Guess there's no need for removeEventListener() here?

Hmm, there shouldn't be, unless people decide to switch iconBoxes between existing active elements, I guess... I suppose I could remove the listeners on the old element if it exists.

Comment 6

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

Attachment: updated patch

Comment 7

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/f9e25d57bb25

Comment 8

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 9

[u279076]

How well is this covered by automation?  Is there value in adding a Litmus test?  If so, does this apply to all PopupNotifications or just certain ones?

Thanks.

Comment 10

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

The automated test covers the fix. It's a bit of a hacky test, though, so a litmus test that exercises the actual customization code paths could still be useful.

Comment 11

[u279076]

Just noticed this very old in-litmus request. Canceling the request. Please re-nom if a test is still needed.