Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla2.0b10

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: whimboo, Assigned: Margaret)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla2.0b10
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][doorhanger])

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 496103 [details]
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
There is a high chance that people will see this. At least at the beginning when add-ons aren't compatible. Asking for blocking.
blocking2.0: --- → ?
blocking2.0: ? → final+
Duplicate of this bug: 619033
Faaborg might have a better idea here
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.
Keywords: uiwanted
Duplicate of this bug: 622780
(Assignee)

Updated

7 years ago
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
(Assignee)

Comment 6

7 years ago
Created attachment 501436 [details] [diff] [review]
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.
Attachment #501436 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 8

7 years ago
Created attachment 501687 [details] [diff] [review]
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.
Attachment #501436 - Attachment is obsolete: true
Attachment #501687 - Flags: review?(gavin.sharp)
Attachment #501436 - Flags: review?(gavin.sharp)
Comment on attachment 501687 [details] [diff] [review]
patch v2

another drive-by comment:

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

var useNotificationBox = TabsOnTop.enabled &&
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).
I guess PopupNotification's "options" parameters aren't supported by notification boxes...
(Assignee)

Comment 12

7 years ago
Created attachment 502075 [details] [diff] [review]
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.
Attachment #501687 - Attachment is obsolete: true
Attachment #502075 - Flags: review?(gavin.sharp)
Attachment #501687 - Flags: review?(gavin.sharp)
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.
Also, gnomestripe is missing.
(Assignee)

Comment 15

7 years ago
(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.
Whiteboard: [soft blocker]
(Assignee)

Comment 16

7 years ago
Comment on attachment 502075 [details] [diff] [review]
alternate patch

Obsoleting, since there's a different patch in the works.
Attachment #502075 - Attachment is obsolete: true
Attachment #502075 - Flags: review?(gavin.sharp)
Depends on: 624843
(Assignee)

Comment 17

7 years ago
Created attachment 502933 [details] [diff] [review]
patch + test
Attachment #502933 - Flags: review?(gavin.sharp)
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.
Whiteboard: [soft blocker] → [softblocker]
(Assignee)

Updated

7 years ago
Whiteboard: [softblocker] → [softblocker][needs review gavin]
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).
Attachment #502933 - Flags: review-
(Assignee)

Comment 20

7 years ago
(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.
(Assignee)

Comment 21

7 years ago
Created attachment 504800 [details] [diff] [review]
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.
Attachment #502933 - Attachment is obsolete: true
Attachment #504800 - Flags: review?(gavin.sharp)
Attachment #502933 - Flags: review?(gavin.sharp)
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 ||.
Attachment #504800 - Flags: review?(gavin.sharp) → review+
Blocks: 613408
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c0c31420a5e3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][needs review gavin] → [softblocker]
Target Milestone: --- → mozilla2.0b10
Blocks: 595810
Depends on: 627236
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
(Assignee)

Updated

7 years ago
Blocks: 628626
No longer blocks: 613408
Whiteboard: [softblocker] → [softblocker][doorhanger]
Blocks: 752216
You need to log in before you can comment on or make changes to this bug.