617553 - Doorhanger for add-on installation failures inside the AOM has no parent with chrome hidden

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot

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

When the browser chrome is hidden and you try to install an extension when the add-ons manager is open (tools button or drag and drop), the doorhanger for installation failures points to a non-existent location bar. See the screenshot. It's a fallout from bug 571970.

Steps:
1. Open the AOM
2. Install an in-compatible or erroneous XPI
3. Check the doorhanger warning

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There is a high chance that people will see this. At least at the beginning when add-ons aren't compatible. Asking for blocking.

Comment 3

[Dave Townsend [:mossop]]

Faaborg might have a better idea here

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Possible fixes, in order of preference:

1) use a notification system in the add-ons manager (boriss mentioned a top aligned message, but I don't know if it has been implemented yet)

2) use a top aligned notification bar

3) use a modal dialog box

doorhangers were never meant for app level notifications, things that don't relate to a particular Web  site.  This is a good example of why doorhangers can't be invoked for app level events.

Comment 6

[:Margaret Leibovic]

Attachment: patch

This implements Alex's second recommendation, since a notification system within the add-ons manager would be more difficult/complicated to implement right now.

Also, this solution takes care of the case of add-ons being installed from other future chromeless pages.

Comment 7

[Dão Gottwald [:dao]]

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

>+    // If chrome is disabled, there is no anchor for popup notifications,
>+    // so we need to use notification bars instead.
>+    var disableChrome = document.documentElement.hasAttribute("disablechrome");
>+    if (disableChrome) {

This is incorrect. The anchor will be there for tabs on bottom. However, it's going to be a bogus anchor since it's not a site-specific notification. Similarly, the notification isn't site-specific when dropping an XPI file on a random page.

Comment 8

[:Margaret Leibovic]

Attachment: patch v2

Updated patch to address Dão's comment about tabs on bottom.

Also, in the addon-install-failed case, there is no action, so I fixed the calls to show the notifications. The PopupNotifications call worked fine, but the notificationBox call would throw an error because I was passing in an buttons array with a null button.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 501687 [details] [diff] [review]
patch v2

another drive-by comment:

>+    var useNotificationBox = document.documentElement.getAttribute("tabsontop") == "true" &&

var useNotificationBox = TabsOnTop.enabled &&

Comment 10

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

A showNotification() helper that factors out the notificationbox-or-popupnotifications branch would make this code simpler, I think.

But this brings up the fact that popupnotifications are rather broken if the anchor box is hidden (i.e. if the location bar is removed or hidden), so perhaps we need a more generic solution. Margaret suggested having PopupNotification.jsm fallback to trying a notificationbox (similar to what nsLoginManagerPrompter.js does), which could work assuming the APIs are entirely compatible (I think they are).

Comment 11

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

I guess PopupNotification's "options" parameters aren't supported by notification boxes...

Comment 12

[:Margaret Leibovic]

Attachment: alternate patch

Based on IRC discussion, I implemented a patch that creates arrow-less panel notifications when the popup notification anchor isn't visible.

We decided that this is better than the notification bar idea because the popup notification and notification bar APIs are different enough that things could break. Also, there are custom notifications (such as the add-on download progress notification) that wouldn't work as notification bars.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 502075 [details] [diff] [review]
alternate patch

>   panel[type="arrow"][side="left"],
>   panel[type="arrow"][side="right"] {
>     margin-top: -23px;
>     margin-bottom: -23px;
>   }
> 
>+  panel[type="arrow"][arrowhidden] {
>+    margin: 0;
>+  }

At a first glance, looks like you should add :not([arrowhidden]) a few lines above.

Comment 14

[Dão Gottwald [:dao]]

Also, gnomestripe is missing.

Comment 15

[:Margaret Leibovic]

(In reply to comment #14)
> Also, gnomestripe is missing.

Oh, I didn't realize we landed the arrow panel styles for gnomestripe. I will add it.

I also discovered a problem with this patch when the location bar is hidden, so I will update it to fix that.

Comment 16

[:Margaret Leibovic]

Comment on attachment 502075 [details] [diff] [review]
alternate patch

Obsoleting, since there's a different patch in the works.

Comment 17

[:Margaret Leibovic]

Attachment: patch + test

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 502933 [details] [diff] [review]
patch + test

>+    // If the anchor element is hidden, use the tab as the anchor. We only ever
>+    // show notifications for the current browser, so we can just use the
>+    // current tab.
>+    let anchorVisibility = anchorElement.ownerDocument.defaultView.
>+                                         getComputedStyle(anchorElement).
>+                                         getPropertyValue("visibility");

mind the dots:

let anchorVisibility = anchorElement.ownerDocument.defaultView
                                    .getComputedStyle(anchorElement)
                                    .getPropertyValue("visibility");

>+    if (anchorVisibility != "visible")

This seems fragile... If the nav bar was hidden with display:none, this would fail.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 502933 [details] [diff] [review]
patch + test

You should go back to checking boxOject.width/height. More reliable and consistent with isElementVisible (utilityOverlay.js).

Comment 20

[:Margaret Leibovic]

(In reply to comment #19)
> Comment on attachment 502933 [details] [diff] [review]
> patch + test
> 
> You should go back to checking boxOject.width/height. More reliable and
> consistent with isElementVisible (utilityOverlay.js).

Unfortunately, that doesn't work because the iconBox can be empty if the caller passes in a null anchorID, in which case it has 0 width/height. In that case, we still want to use the empty iconBox as the anchor element, so we need to use something else to test if the iconBox is hidden.

As an alternative solution, we could try to ensure that the iconBox has a minimum width.

Comment 21

[:Margaret Leibovic]

Attachment: patch (w/ height/width check and null anchorID fix)

This patch goes back to using the height/width check. To fix the problem of a null anchorID, I added a default icon, which was actually suggested by Alex a while back in bug 613408 comment 10.

Comment 22

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

Comment on attachment 504800 [details] [diff] [review]
patch (w/ height/width check and null anchorID fix)

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#notification-popup-box[anchorid="notification-popup-box"] > #default-notification-icon,

Maybe add a comment that this icon is slightly different (in that it must use the anchorid of the iconBox that we pass in to the PopupNotifications constructor).
 
>diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js

>+      is(popup.anchorNode.className, "tabbrowser-tab", "notification anchored to tab");

Maybe we should add an opposite test to checkPopup() to catch the default case (make sure notifications aren't anchored to tabs all the time)?

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

>+    if (bo.height == 0 || bo.width == 0)
>+      anchorElement = this.tabbrowser.selectedTab;

I think this should be && rather than ||.

Comment 23

[:Margaret Leibovic]

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

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on OS X and Windows with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre

I will minus a Litmus test because it's already covered by tests which test invalid extensions.