348576 - addonsMsg should use standard Notification styles

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jay Goldman]

Messages displayed at the top of the Add-Ons Manager should use the standard notification styling defined in toolkit/themes/pinstripe/global/notification.css. Suggest the "info" type for messages like "No updates found".

Comment 1

[Reed Loden [:reed]]

Does winstripe have new notification styles, too?

Comment 2

[Robert Strong (they/them - no direct email)]

The notification widget was added around the same time as the new EM ui. I planned on seeing what it will take to switch to use the notification widget for Firefox 3.0 and doubt I will have time to do so for Firefox 2.0

Comment 3

[Mike Connor [:mconnor]]

Basically, we need to stick a <notificationbox> around the content area, and call the right methods.  Not hard at all.

--> mwu

Comment 4

[Robert Strong (they/them - no direct email)]

note: the addonsmsg diverged significantly from browsermessage and there are a couple of methods not implemented by notification it requires.

Comment 5

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager

This is not trivial. I've put in a compatibility shim to ease the code into using the new notification box, so hopefully most problems are concentrated in a single place.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 233656 [details] [diff] [review]
Switch to notificationbox in addons manager

per face to face discussion with mwu this patch as it stands causes several regressions so -'ing.

Comment 7

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager, v2

Comment 8

[Michael Wu [:mwu]]

Attachment: notificationbox is racy

This can happen when opening two notifications before the window is visible.

Comment 9

[Mike Connor [:mconnor]]

bumping to discuss in next triage.

Basically, this is a nice-to-have, and the obvious solution is to a) fix notificationbox to not race and b) make this Just Work in EM (we should have a consistent interaction model for this type of dialog replacement).  There's bigger things in the list that need focus that we can't ship without.

Comment 10

[Mike Schroepfer]

per beltnzer/schrep weekend triage we'd take a patch but this is not a blocker.

Comment 11

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager, v3

This patch depends on the raciness in notification box being fixed. This patch seems to work perfectly with that fixed.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 235964 [details] [diff] [review]
Switch to notificationbox in addons manager, v3

I hope this isn't bitrotted.. should work fine if it isn't, now that the races in notification box widget are fixed.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 235964 [details] [diff] [review]
Switch to notificationbox in addons manager, v3

Could you first verify that the patch isn't bit-rotted before requesting review so reviewers don't have to review more than one time? Thanks

Comment 14

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager, v4

Might as well put up a new patch since the old one was for branch, and I don't expect this to hit branch at this point.

Comment 15

[Robert Strong (they/them - no direct email)]

Why does the 'branch' patch modify extensions.xul differently than the trunk patch? Is it that the changes to extensions.xul in the trunk patch actually what you wanted for extensions.xul?

Comment 16

[Robert Strong (they/them - no direct email)]

With the latest patch here and the patch from bug 350353 applied if you have two notifications (e.g. safe mode and compatibility checking off) when opening the EM ui this bails during startup. I'm re-building my debug build so I can't provide any more detail than that atm. It appears to work fine with a single notification though it was with minimal testing.

Comment 17

[Robert Strong (they/them - no direct email)]

btw: this was with branch

Comment 18

[Michael Wu [:mwu]]

(In reply to comment #15)
> Why does the 'branch' patch modify extensions.xul differently than the trunk
> patch? Is it that the changes to extensions.xul in the trunk patch actually
> what you wanted for extensions.xul?
> 
The branch patch wraps the extensionsBox and commandBarBottom, whereas the trunk patch just wraps the extensionsBox. When redoing this patch for trunk, I felt that wrapping just extensionsBox would make more sense since we never want to affect the height of commandBarBottom.. not that it makes much of a difference in practice, I think.

Comment 19

[Michael Wu [:mwu]]

(In reply to comment #17)
> btw: this was with branch
> 
Er, the v4 patch from bug 350353 does not apply cleanly to branch, and v3 is buggy despite applying to branch. The blocking state stuff will have to be removed from the v4 patch to work on branch.

Oh, and that reminds me - what elements we wrap is important if we ever want to use the blocking state.. but we also have other, more severe problems if we want to use it.

Comment 20

[Robert Strong (they/them - no direct email)]

I tried v4 with the branch and hand merged the patch in bug 350353. I'd like to get this on the branch but bug 350353 needs to also be on the branch.

I just tried this on a trunk build and the accesskey for the notification is always U. :/

Comment 21

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager, v5

Hm, need to check if the behavior of noUpdatesDismiss is correct..

This fixes the accesskey issue and makes sure the no updates notification gets a proper closing animation.

Comment 22

[Robert Strong (they/them - no direct email)]

Comment on attachment 237591 [details] [diff] [review]
Switch to notificationbox in addons manager, v5

Just a couple of nits.

>Index: toolkit/mozapps/extensions/content/extensions.js
...
>@@ -543,44 +581,41 @@ function Startup() 
...
>   if (gInSafeMode) {
>-    var addonsMsg = document.getElementById("addonsMsg");
>-    addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
>-                          getExtensionString("safeModeMsg"),
>-                          null, null, true, null);
>+    showMessage("chrome://mozapps/skin/extensions/question.png",
>+                getExtensionString("safeModeMsg"),
>+                null, null, true, null);
>   }
nit: remove brackets

>@@ -1240,33 +1275,31 @@ function isXPInstallEnabled() {
...
>   if (ioService.offline) {
>-    var addonsMsg = document.getElementById("addonsMsg");
>-    addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
>-                          getExtensionString(messageKey, [getBrandShortName()]),
>-                          getExtensionString("goOnlineButtonLabel"),
>-                          getExtensionString("goOnlineButtonAccesskey"),
>-                          true, "addons-go-online");
>+    showMessage("chrome://mozapps/skin/extensions/question.png",
>+                getExtensionString(messageKey, [getBrandShortName()]),
>+                getExtensionString("goOnlineButtonLabel"),
>+                getExtensionString("goOnlineButtonAccesskey"),
>+                true, "addons-go-online");
>   }
nit: remove brackets

Are there other issues that still need to be addressed? The only things I see that are a bit off are the image being up against the left border and the way the notificationbox briefly displays at its full height and then slides out... it seems to do this in the content window as well.

Comment 23

[Michael Wu [:mwu]]

(In reply to comment #22)
> Are there other issues that still need to be addressed? The only things I see
> that are a bit off are the image being up against the left border and the way
> the notificationbox briefly displays at its full height and then slides out...
> it seems to do this in the content window as well.
> 
The image not having enough padding bothers me too, but I guess if the content window can tolerate it..

The one that that needs to be checked is if noUpdatesDismiss is being called at all the right times. Also, before, closing the notification would always send off some sort of notification.. I don't know if we still do that, or how important it is. If you can verify the notifications and callbacks still act the same/correctly, then the only problem remaining is the very interesting counter we use to enforce correct ordering of notifications, which can't be helped for now. :)

Still want this for 1.8.1? We should nominate the patch in bug 350353 real soon if this is gonna make it.

Comment 24

[Robert Strong (they/them - no direct email)]

Either way, if the patch in bug 350353 is safe we should get it in there so the door is open to take this. I'll tale a look at noUpdatesDismiss tomorrow but at a glance it looks fine.

The image in the notificationbox when in the content window isn't up against the left border but it is in the EM ui... I suspect it can be easily fixed via extensions.css

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 237591 [details] [diff] [review]
Switch to notificationbox in addons manager, v5

With a fix for bug 352456 this patch is an improvement ui wise over the existing functionality. We can address the use of priority if needed later. Since the flashing in bug 352456 is more pronounced in the add-ons mgr. I'd like to get that fixed for 1.8.1 before this lands on branch but I'll think on that more tomorrow morning.

Comment 26

[Robert Strong (they/them - no direct email)]

I've done several tests on both trunk and branch of this patch along with the patch in bug 352456 and it is a significant improvement.

Comment 27

[Michael Wu [:mwu]]

Checking in toolkit/mozapps/extensions/content/extensions.css;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.css,v  <--  extensions.css
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.115; previous revision: 1.114
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.42; previous revision: 1.41
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.52; previous revision: 1.51
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.27; previous revision: 1.26
done

Comment 28

[Michael Wu [:mwu]]

Er, I forgot to eliminate the extra braces in the checkin.

Comment 29

[Robert Strong (they/them - no direct email)]

Those were just nits so don't worry about them... I'll fix them sometime in the 3.0 cycle

Comment 30

[Robert Strong (they/them - no direct email)]

Comment on attachment 237591 [details] [diff] [review]
Switch to notificationbox in addons manager, v5

If Bug 352456 gets approved for the branch I'd like to get this on the branch

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 237591 [details] [diff] [review]
Switch to notificationbox in addons manager, v5

a=mconnor on behalf of drivers for 1.8 branch checkin

Comment 32

[Michael Wu [:mwu]]

Attachment: Switch to notificationbox in addons manager, v5 (branch)

Checking in toolkit/mozapps/extensions/content/extensions.css;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.css,v  <--  extensions.css
new revision: 1.9.2.4; previous revision: 1.9.2.3
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.72.2.40; previous revision: 1.72.2.39
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.17.2.21; previous revision: 1.17.2.20
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.37.2.15; previous revision: 1.37.2.14
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.14.2.10; previous revision: 1.14.2.9
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.16.2.11; previous revision: 1.16.2.10
done