Closed Bug 624843 Opened 9 years ago Closed 9 years ago

PopupNotifications are horribly broken if the location bar is removed

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: Gavin, Assigned: Gavin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday][doorhanger])

Attachments

(1 file, 2 obsolete files)

...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).
"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".
blocking2.0: --- → final+
Whiteboard: [hardblocker]
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → gavin.sharp
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #504801 - Attachment is obsolete: true
Attachment #504945 - Flags: review?(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?
Attachment #504945 - Flags: review?(dolske) → review+
(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.
Attached patch updated patchSplinter Review
Attachment #504945 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/f9e25d57bb25
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Status: RESOLVED → VERIFIED
Flags: in-litmus?(anthony.s.hughes)
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Whiteboard: [hardblocker][fx4-fixed-bugday] → [hardblocker][fx4-fixed-bugday][doorhanger]
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.
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.
Flags: in-testsuite+
Just noticed this very old in-litmus request. Canceling the request. Please re-nom if a test is still needed.
Flags: in-litmus?(anthony.s.hughes)
You need to log in before you can comment on or make changes to this bug.