610130 - Doorhanger notifications confused when switching between tabs with different types of doorhangers

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Nickolay_Ponomarev]

Attachment: screenshot #1

1. Open http://gavinsharp.com/tmp/geo.html (a geolocation doorhanger should appear)
2. Open http://quicknote.mozdev.org/installation.html
3. Click  Install QuickNote 0.6.0.5 (an addon installation doorhanger should appear)
4. switch back to gavinsharp.com using keyboard (Cmd+alt+left on Mac)
-> the doorhanger notification appears behind tabs anchored to the top-left of the window (screenshot #1)
5. switch back to quicknote.mozdev.org
-> the doorhanger notification becomes as wide as the screen (screenshot #2)

The notifications are confused from this moment until a restart.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b8pre) Gecko/20101106 Firefox/4.0b8pre

Comment 1

[Nickolay_Ponomarev]

Attachment: screenshot #2

Comment 2

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

So part of the issue is that we use display:none to hide the icons on tab switch, and the showing of the anchor isn't guaranteed to happen before we try to show the popup (popups don't like being anchored to display:none elements).

Using visibility: collapse to do the hiding solves part of that problem, but it still results in the popup being positioned at the wrong spot, because the we use the anchor image's original position rather than the post-hiding position. I guess we could work around that by positioning all the icons so that they're in the same place all of the time...

Comment 3

[Drew Willcoxon :adw]

Attachment: patch

Making sure the popup is hidden before changing its anchor seems to work.  In my testing both icon images actually did have a display of -moz-box before the popup was shown, not none.

I noticed that with Nickolay's example the tab you choose second is the problem.  Like, if you do the STR in comment 0, the gavinsharp.com doorhanger position is off, but then if you keyboard to an unrelated tab, back to gavinsharp.com, then back to quicknote.mozdev.org, the gavinsharp.com doorhanger is fine while the quicknote doorhanger position is off.

Comment 4

[Drew Willcoxon :adw]

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

This isn't right, the test fails.

Comment 5

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

Attachment: patch

Here is the patch that I was playing with, fwiw.

Comment 6

[Drew Willcoxon :adw]

Attachment: hide popup first patch 2

Gavin, how's this?  It still hides the popup first if it's shown but in a more correct way.  I had to modify one test to get it to pass, because an extra popuphidden is fired when the popup is removed, and the harness expects only one popuphidden event to be fired for each test.

Comment 7

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

Comment on attachment 494899 [details] [diff] [review]
hide popup first patch 2

>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm

>     if (this.isPanelOpen && this._currentAnchorElement == anchorElement)
>       return;

>+    // If the panel is already open but we're changing anchors, we need to hide
>+    // it first.  Otherwise it can appear in the wrong spot.
>+    if (this.isPanelOpen && this._currentAnchorElement != anchorElement)
>+      this._hidePanel();

You can just call _hidePanel() unconditionally (it never throws) after the first isPanelOpen check, r=me with that.

Comment 9

[Drew Willcoxon :adw]

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