Closed Bug 570004 (sm-doorhanger) Opened 14 years ago Closed 13 years ago

doorhanger notifications for SeaMonkey

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1final
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: kairo, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [doorhanger])

Attachments

(2 files, 10 obsolete files)

82.93 KB, patch
Details | Diff | Splinter Review
20.46 KB, patch
kairo
: review+
Details | Diff | Splinter Review
The site-specific "doorhanger" notifications will land for Firefox 4, probably soon, and should be a good improvement to notification bars, e.g. harder to spoof by websites and other improvements noted in the FF bug.

While bug 398776 comment #17 states that this might be moved to toolkit later, it will be app-specific in this first installment but we should get it as well, so we'll need to port the complete work instead of just the notifications themselves.
Alias: sm-doorhanger
Version: unspecified → Trunk
We can move the JS module to toolkit really easily - don't go and duplicate all the code just because the current patch has it in browser/.
(In reply to comment #1)
> We can move the JS module to toolkit really easily - don't go and duplicate all
> the code just because the current patch has it in browser/.

That would be really nice to have, but I don't want to put you under pressure to actually do that move. If it can be done in time for us to implement doorhanger notifications, we will be very happy to use that from toolkit - but if it can't be done, we're prepared to temporarily put a copy on our side until that move is done.
I don't want either of us to feel pressured or slowed down by the other - esp. in the light of SeaMonkey (and possibly other toolkit apps) being apparently sometimes felt to slow down Firefox progress, which I really want to avoid where possible.
This just ports the Firefox part to SeaMonkey - including the slight shortcomings of the new geolocation notification, i.e. the icon being blown up instead of using a right-sized one and the missing "Learn More..." link, both of which can very easily be fixed in followups, just like they do it for Firefox as well.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #453054 - Flags: review?(neil)
Attached patch v1.1: don't use Ci.* ;-) (obsolete) — Splinter Review
I forgot a usage of Ci.* I copied over in the first patch, should be fixed in this one. And the test works with bug 573733 fixed.
Attachment #453054 - Attachment is obsolete: true
Attachment #453062 - Flags: review?(neil)
Attachment #453054 - Flags: review?(neil)
Comment on attachment 453062 [details] [diff] [review]
v1.1: don't use Ci.* ;-)

>diff --git a/suite/browser/nsBrowserStatusHandler.js b/suite/browser/nsBrowserStatusHandler.js

>+    // This code here does not compare uris exactly when determining

Eek, don't copy this code, it's an abomination :) I have a several-year-old patch somewhere that cleans it up a bit...
(In reply to comment #5)
> (From update of attachment 453062 [details] [diff] [review])
> >diff --git a/suite/browser/nsBrowserStatusHandler.js b/suite/browser/nsBrowserStatusHandler.js
> 
> >+    // This code here does not compare uris exactly when determining
> 
> Eek, don't copy this code, it's an abomination :) I have a several-year-old
> patch somewhere that cleans it up a bit...

What should I use instead to trigger that PopupNotifications.locationChange(); hidden in there?
Depends on: 574230
Comment on attachment 453062 [details] [diff] [review]
v1.1: don't use Ci.* ;-)

>-  <popupset id="mainPopupSet"/> <!-- Firefox extension compatibility -->
>+  <popupset id="mainPopupSet"> <!-- Firefox extension compatibility -->
Nit: no longer true if we're using it

>+XPCOMUtils.defineLazyGetter(this, "PopupNotifications", function () {
>+  let tmp = {};
>+  Components.utils.import("resource://gre/modules/PopupNotifications.jsm", tmp);
>+  return new tmp.PopupNotifications(gBrowser,
>+                                    document.getElementById("notification-popup"),
>+                                    document.getElementById("notification-popup-box"));
>+});
Actually this isn't lazy enough, since we get lots of location changes, but hardly any geolocation prompts.

>+    // This code here does not compare uris exactly when determining
>+    // whether or not the message should be hidden since the message
>+    // may be prematurely hidden when an install is invoked by a click
>+    // on a link that looks like this:
>+    //
>+    // <a href="#" onclick="return install();">Install Foo</a>
On the other hand you could just check whether a document is loading, like the notification bar does.

>+    // Ignore requests from non-nsIStandardURLs
>+    if (!(requestingURI instanceof Components.interfaces.nsIStandardURL))
>+      return;
Shouldn't we cancel the request here?

>+        .QueryInterface(Components.interfaces.nsIDOMChromeWindow);
Don't need this bit, we have class info for that.

>-              aRequest.cancel();
No cancel action?

>+    var browser = chromeWin.gBrowser.getBrowserForDocument(requestingWindow.document);
Nit: we prefer getBrowser()
(In reply to comment #7)
> Actually this isn't lazy enough, since we get lots of location changes, but
> hardly any geolocation prompts.

I don't know how we could make it more lazy. I wonder if Gavin has an idea.

> On the other hand you could just check whether a document is loading, like the
> notification bar does.

Hrm, I'd like to avoid writing completely different code than Firefox for this, if possible, among other things for maintenance reasons (I'm actually quite a bit annoyed at how many subtle unneeded differences we have where users don't even notice them).
Gavin, is that comment something Firefox would also want to adopt?

> >+    // Ignore requests from non-nsIStandardURLs
> >+    if (!(requestingURI instanceof Components.interfaces.nsIStandardURL))
> >+      return;
> Shouldn't we cancel the request here?

Possibly. Gavin?

> >+        .QueryInterface(Components.interfaces.nsIDOMChromeWindow);
> Don't need this bit, we have class info for that.

Erm, sorry to ask a stupid question, but what does that mean code-wise?

> >-              aRequest.cancel();
> No cancel action?

No, the popup notification doesn't have one right now.

> >+    var browser = chromeWin.gBrowser.getBrowserForDocument(requestingWindow.document);
> Nit: we prefer getBrowser()

Because it is less performant (function call instead of direct variable reference) and an artificial difference to Firefox? :-/
This patch addresses some comment(s) from Neil, and incorporates bug 574230 as pointed out by Gavin over IRC, but note my questions in bug 574230 comment #4 - I hope some things can be made nicer in the end. :)
Attachment #453062 - Attachment is obsolete: true
Attachment #453915 - Flags: review?(neil)
Attachment #453062 - Flags: review?(neil)
(In reply to comment #8)
> I don't know how we could make it more lazy.

We could add a flag to mark whether it has been initialized, and only call onLocationChange if it has. File a bug on Firefox and I'll patch it.

> > On the other hand you could just check whether a document is loading, like
> > the notification bar does.

I don't understand this proposal.

> > >+    // Ignore requests from non-nsIStandardURLs
> > >+    if (!(requestingURI instanceof Components.interfaces.nsIStandardURL))
> > >+      return;
> > Shouldn't we cancel the request here?

I suppose so. This was just belt-and-braces - the old code threw an exception.

> > >+        .QueryInterface(Components.interfaces.nsIDOMChromeWindow);
> > Don't need this bit, we have class info for that.
> 
> Erm, sorry to ask a stupid question, but what does that mean code-wise?

It means "just remove this line", but it's not worth the divergence from the original code, IMO.

> Because it is less performant (function call instead of direct variable
> reference) and an artificial difference to Firefox? :-/

IIRC yours isn't a getter, so there's potential for it to be uninitialized. I think you should fix that!
Depends on: 574228
I just noticed I should add the bug 574228 additions to the test as well.
Attachment #453915 - Attachment is obsolete: true
Attachment #453933 - Flags: review?(neil)
Attachment #453915 - Flags: review?(neil)
Depends on: 574530
(In reply to comment #10)
> (In reply to comment #8)
> > I don't know how we could make it more lazy.
> 
> We could add a flag to mark whether it has been initialized, and only call
> onLocationChange if it has. File a bug on Firefox and I'll patch it.

Filed bug 574530 on that.
(In reply to comment #8)
> > >-              aRequest.cancel();
> > No cancel action?
> 
> No, the popup notification doesn't have one right now.

To be clear: There is a cancel action in the form of the "Never share" selection that also sets the permission, there is no explicit "Not this time" action right now, only the location change or clicking somewhere outside the notification, which will make it go away.
I shouldn't do such work with a slight headache, I guess... I forgot some parts in the last two patches, and for added bonus I addressed more comments this time.
Attachment #453933 - Attachment is obsolete: true
Attachment #453942 - Flags: review?(neil)
Attachment #453933 - Flags: review?(neil)
(In reply to comment #10)
>(In reply to comment #8)
>>I don't know how we could make it more lazy.
>We could add a flag to mark whether it has been initialized, and only call
>onLocationChange if it has. File a bug on Firefox and I'll patch it.
Strangely enough the "dumb" getter approach works well here, the location code simply uses the global variable while the geolocation code calls the getter function which creates the object.

>>On the other hand you could just check whether a document is loading, like
>>the notification bar does.
>I don't understand this proposal.
SeaMonkey's notification bar uses the following code to avoid clearing notifications unnecessarily:
var errorPage = aRequest && !Components.isSuccessCode(aRequest.status);
if (aWebProgress.DOMWindow == this.activeBrowser.contentWindow &&
    (aWebProgress.isLoadingDocument || errorPage)) {
  this.removeTransientNotifications();
}

>>>+    // Ignore requests from non-nsIStandardURLs
>>>+    if (!(requestingURI instanceof Components.interfaces.nsIStandardURL))
>>>+      return;
>> Shouldn't we cancel the request here?
>I suppose so. This was just belt-and-braces - the old code threw an exception.
Ah, in that case this behaviour is no worse than the previous one.

>>>>+        .QueryInterface(Components.interfaces.nsIDOMChromeWindow);
>>> Don't need this bit, we have class info for that.
>> Erm, sorry to ask a stupid question, but what does that mean code-wise?
>It means "just remove this line", but it's not worth the divergence from the
>original code, IMO.
Bah. Can you at least comment it out?

>> Because it is less performant (function call instead of direct variable
>> reference) and an artificial difference to Firefox? :-/
>IIRC yours isn't a getter, so there's potential for it to be uninitialized. I
>think you should fix that!
At least nowadays we have XPCOMUtils to hide some of the ugliness for us...

(In reply to comment #8)
>>>-              aRequest.cancel();
>> No cancel action?
> No, the popup notification doesn't have one right now.
The geolocation backend doesn't mind, I hope?
This patch addresses more recent comments, updates to the checked-in state of bug 574230, and also includes the bug 574530 patch.
Attachment #453942 - Attachment is obsolete: true
Attachment #454272 - Flags: review?(neil)
Attachment #453942 - Flags: review?(neil)
(In reply to comment #15)
> >It means "just remove this line", but it's not worth the divergence from the
> >original code, IMO.
> Bah. Can you at least comment it out?

Remove it in both places if you care that much. That requires testing to make sure that it works.
Here's a v2.4, just because I can. Or possibly, because the new bug 574530 patch is different again. ;-)
Attachment #454272 - Attachment is obsolete: true
Attachment #454519 - Flags: review?(neil)
Attachment #454272 - Flags: review?(neil)
(In reply to comment #15)
> SeaMonkey's notification bar uses the following code to avoid clearing
> notifications unnecessarily:

This looks better than what we use. We should get a bug on file on changing it in Firefox.

> (In reply to comment #8)
> >>>-              aRequest.cancel();
> >> No cancel action?
> > No, the popup notification doesn't have one right now.
> The geolocation backend doesn't mind, I hope?

It doesn't - requests already time out on their own. Which reminds me that I need to check what happens when the notification outlives its request...
Depends on: 575957
(In reply to comment #19)
> (In reply to comment #15)
> > SeaMonkey's notification bar uses the following code to avoid clearing
> > notifications unnecessarily:
> 
> This looks better than what we use. We should get a bug on file on changing it
> in Firefox.

I filed bug 575957 for this.
Depends on: 565187
This integrates the current WIP patch for bug 575957 and the icons from bug 565187 in a form that should fit for us (I used the winstripe ones for Classic, the pinstripe ones for Mac Classic and the gnomestripe ones for Modern - the latter mostly out of looking at the screenshot in attachment 455319 [details] and deciding that the gnomestripe one in the middle probably fits Modern's color scheme best).
Attachment #454519 - Attachment is obsolete: true
Attachment #455532 - Flags: review?(neil)
Attachment #454519 - Flags: review?(neil)
Bah, doorhangers suck without bug 572967.
Depends on: 572967
Comment on attachment 455532 [details] [diff] [review]
v2.5: update bug 575957 and bug 565187 patches

>+          <box id="notification-popup-box" hidden="true" align="center">
>+            <image id="geo-notification-icon"/>
>+          </box>
>           <deck id="page-proxy-deck"
IMHO this would look better at the end of the URLbar, just after the EV and feed icons.

>+    if (aWebProgress.DOMWindow == content) {
We already have an if (aWebProgress.DOMWindow == content) block.

>+    var browser = chromeWin.getBrowser().getBrowserForDocument(requestingWindow.document);
There is actually a neater way to do this, see getNotificationBox:
aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
       .getInterface(Components.interfaces.nsIWebNavigation)
       .QueryInterface(Components.interfaces.nsIDocShell)
       .chromeEventHandler.wrappedJSObject;
(In reply to comment #23)
> Bah, doorhangers suck without bug 572967.

Well, not more than notification bars - but doorhangers at least can't be spoofed that easily. That said, this bug will surely be one good improvement over notification bars - once it's done (in a followup).

(In reply to comment #24)
> Comment on attachment 455532 [details] [diff] [review]
> v2.5: update bug 575957 and bug 565187 patches
> 
> >+          <box id="notification-popup-box" hidden="true" align="center">
> >+            <image id="geo-notification-icon"/>
> >+          </box>
> >           <deck id="page-proxy-deck"
> IMHO this would look better at the end of the URLbar, just after the EV and
> feed icons.

That would go against the design philosophy it follows in Firefox and which it's designed for, and that is that it's attached to the site (favicon/url).

> >+    var browser = chromeWin.getBrowser().getBrowserForDocument(requestingWindow.document);
> There is actually a neater way to do this, see getNotificationBox:
> aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>        .getInterface(Components.interfaces.nsIWebNavigation)
>        .QueryInterface(Components.interfaces.nsIDocShell)
>        .chromeEventHandler.wrappedJSObject;

Umm, that un-understandable pile of complicated functions is "neater" than the one-liner? I can't really follow you there.
It's less concise, but it's more efficient.
OK, I still don't see what code I should put there, as this all sounds like voodoo to me. I need to get chromeWin anyhow, so how again do I come from chromeWin to the browser I need with those strange, said to be more efficient functions that are completely obscure to a mere mortal?
I really need the help of you gods of object-walking if I should write that differently - i.e. I need to actual code, not just a hint that consists of a blob that gets some other object than the one I want.
> I need to actual code, not just a hint that consists of a
> blob that gets some other object than the one I want.

KaiRo, Neil did say "see getNotificationBox". That's a hint.
Blocks: 581974
(In reply to comment #28)
> > I need to actual code, not just a hint that consists of a
> > blob that gets some other object than the one I want.
> 
> KaiRo, Neil did say "see getNotificationBox". That's a hint.

Yes, that's what it is, a HINT. And my message clearly says that I don't understand any of that voodoo that's done there, so I need the actual code I can use there, as a HINT doesn't help me!
(In reply to comment #24)
> >+    if (aWebProgress.DOMWindow == content) {
> We already have an if (aWebProgress.DOMWindow == content) block.

Oh, and that part causes the added test to fail - see bug 575957 comment #2
(In reply to comment #25)
> (In reply to comment #23)
> > Bah, doorhangers suck without bug 572967.
> Well, not more than notification bars
At least notification bars aren't modal, but doorhangers, without bug 572967, are system modal - one accidental focus loss and they're gone.

>(In reply to comment #24)
>>(From update of attachment 455532 [details] [diff] [review])
>>>+          <box id="notification-popup-box" hidden="true" align="center">
>>>+            <image id="geo-notification-icon"/>
>>>+          </box>
>>>           <deck id="page-proxy-deck"
>>IMHO this would look better at the end of the URLbar, just after the EV and
>>feed icons.
>That would go against the design philosophy it follows in Firefox and which
>it's designed for, and that is that it's attached to the site (favicon/url).
Again, I'd need 572967 to be sure, but how about after the favicon instead?

> > There is actually a neater way to do this, see getNotificationBox:
> Umm, that un-understandable pile of complicated functions is "neater" than the
> one-liner? I can't really follow you there.
But that one-liner just calls an even bigger pile of complicated functions...
(In reply to comment #30)
> (In reply to comment #24)
> > >+    if (aWebProgress.DOMWindow == content) {
> > We already have an if (aWebProgress.DOMWindow == content) block.
> Oh, and that part causes the added test to fail
Not sure what you mean by this.
(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #24)
> > > >+    if (aWebProgress.DOMWindow == content) {
> > > We already have an if (aWebProgress.DOMWindow == content) block.
> > Oh, and that part causes the added test to fail
> Not sure what you mean by this.

The test I'm adding in this bug fails with the changed logic for detecting a page change - not with which block we are doing it in, but with the fact that we are doing it differently than Firefox does right now. Gavin' patch for Firefox exposes the same test failure, we still need to find out why, see the referenced comment in the other bug.
(In reply to comment #31)
> At least notification bars aren't modal, but doorhangers, without bug 572967,
> are system modal - one accidental focus loss and they're gone.

Hmm, OK, that's an understandable argument.

> Again, I'd need 572967 to be sure, but how about after the favicon instead?

Hmm, putting something in between favicon and URL sounds strange to me.

> > > There is actually a neater way to do this, see getNotificationBox:
> > Umm, that un-understandable pile of complicated functions is "neater" than the
> > one-liner? I can't really follow you there.
> But that one-liner just calls an even bigger pile of complicated functions...

Well, it abstracts things enough that I understand them, while I have not the slightest clue about how to make things work the way you want it. If you want me to change this, please give me the exact code as I don't understand the slightest bit of what your getNotificationBox magic does so I can't derive anything from it.
This patch should solve the issue about getting the browser, adds a workaround for the test failure with the bug 575957 logic and adds the new cases to the test that bug 581229 added (just testing features of the toolkit implementation).
Attachment #455532 - Attachment is obsolete: true
Attachment #461555 - Flags: review?(neil)
Attachment #455532 - Flags: review?(neil)
Attached patch v2.7: integrate bug 572967 work (obsolete) — Splinter Review
Even if that patch itself has been backed out for the moment, the bug 572967 parts that are app-specific do apply well on our side and should be good to go there, so I added them here.

Note to Neil: This review should be lower priority than the places stuff. ;-)
Attachment #461555 - Attachment is obsolete: true
Attachment #462473 - Flags: review?(neil)
Attachment #461555 - Flags: review?(neil)
(In reply to comment #13)
> (In reply to comment #8)
> > > >-              aRequest.cancel();
> > > No cancel action?
> > No, the popup notification doesn't have one right now.
> To be clear: There is a cancel action in the form of the "Never share"
> selection that also sets the permission, there is no explicit "Not this time"
> action right now, only the location change or clicking somewhere outside the
> notification, which will make it go away.
Given bug 572967, won't the doorhanger, um, hang around? But I still want to be able to tell the page that I cancelled this geolocation prompt anyway.

The sidebar geolocation notification is unusable as it stands, so it's not much of a loss. But it would be nice to be able to keep some sort of notification for events on web pages in the sidebar.
Comment on attachment 462473 [details] [diff] [review]
v2.7: integrate bug 572967 work

>+#notification-popup-box[anchorid="geo-notification-icon"] > #geo-notification-icon,
>+#notification-popup-box[anchorid="addons-notification-icon"] > #addons-notification-icon {
>+  display: -moz-box;
Will we have to repeat this for every single type of notification?

>diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
[Haven't looked at this yet.]

>+/* Notification icon box */
>+#notification-popup-box {
>+  margin: 0 3px;
This margin looks wrong. (If notifications were at the end of the URLbar then I wouldn't have noticed...)

>+.notification-anchor-icon:focus {
>+  outline: 1px dotted -moz-DialogText;
>+  outline-offset: -3px;
This offset looks wrong. (And the Modern lack of offset also looks wrong.)

>+.notification-anchor-icon:focus {
>+  outline: 1px dotted -moz-DialogText;
This colour is wrong for Modern.
(In reply to comment #38)
> Comment on attachment 462473 [details] [diff] [review]
> v2.7: integrate bug 572967 work
> 
> >+#notification-popup-box[anchorid="geo-notification-icon"] > #geo-notification-icon,
> >+#notification-popup-box[anchorid="addons-notification-icon"] > #addons-notification-icon {
> >+  display: -moz-box;
> Will we have to repeat this for every single type of notification?

Looks like it, yes.

> >diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
> [Haven't looked at this yet.]
> 
> >+/* Notification icon box */
> >+#notification-popup-box {
> >+  margin: 0 3px;
> This margin looks wrong. (If notifications were at the end of the URLbar then I
> wouldn't have noticed...)

Not having this margin looks wrong to me. Note that this is around the anchor icon, not a box of the actual notification. And note that positioning of the notification itself will probably change somewhat once it becomes an arrow panel, the current styling is somewhat preliminary.

> >+.notification-anchor-icon:focus {
> >+  outline: 1px dotted -moz-DialogText;
> >+  outline-offset: -3px;
> This offset looks wrong. (And the Modern lack of offset also looks wrong.)

That's the same value in in Firefox, need to ask gavin about that. And why does the lack in Modern look wrong, if it's not there, it's just 0, right?

> >+.notification-anchor-icon:focus {
> >+  outline: 1px dotted -moz-DialogText;
> This colour is wrong for Modern.

Oops.
(In reply to comment #39)
> > >+/* Notification icon box */
> > >+#notification-popup-box {
> > >+  margin: 0 3px;
> > This margin looks wrong. (If notifications were at the end of the URLbar then I
> > wouldn't have noticed...)
> 
> Not having this margin looks wrong to me.

OK, looks right in default for me, but wrong in Modern. Fixed there.

> > >+.notification-anchor-icon:focus {
> > >+  outline: 1px dotted -moz-DialogText;
> > >+  outline-offset: -3px;
> > This offset looks wrong. (And the Modern lack of offset also looks wrong.)
> 
> That's the same value in in Firefox, need to ask gavin about that.

And yes, there seems to be some difference here that makes -3px be wrong on our side. Not specifying one (i.e. setting it to 0) makes it look correctly for me.
Here's an updated patch for the latest comments, and adding the new tests that meanwhile have been added to the test on the FF side (all code changes for that are on the toolkit side).
Attachment #462473 - Attachment is obsolete: true
Attachment #468675 - Flags: review?(neil)
Attachment #462473 - Flags: review?(neil)
(In reply to comment #39)
>(In reply to comment #38)
>>(From update of attachment 462473 [details] [diff] [review])
>>>+#notification-popup-box[anchorid="geo-notification-icon"] > #geo-notification-icon,
>>>+#notification-popup-box[anchorid="addons-notification-icon"] > #addons-notification-icon {
>>>+  display: -moz-box;
>>Will we have to repeat this for every single type of notification?
>Looks like it, yes.
That sucks :-(

>Not having this margin looks wrong to me. Note that this is around the anchor
>icon, not a box of the actual notification.
I understand. But none of our other URLbar icons has a margin.

>>>+.notification-anchor-icon:focus {
>>>+  outline: 1px dotted -moz-DialogText;
>>>+  outline-offset: -3px;
>>This offset looks wrong. (And the Modern lack of offset also looks wrong.)
>That's the same value in in Firefox, need to ask gavin about that. And why does
>the lack in Modern look wrong, if it's not there, it's just 0, right?
Right, but I thought we normally use a -1px offset.
(In reply to comment #42)
> (In reply to comment #39)
> >Not having this margin looks wrong to me. Note that this is around the anchor
> >icon, not a box of the actual notification.
> I understand. But none of our other URLbar icons has a margin.

I think the page proxy icon actually does. At least it isn't as near to the start of the urlbar than this anchor would be without the margin. And then, we need something to make a bit of space between this anchor and the page proxy icon.

> >And why does
> >the lack in Modern look wrong, if it's not there, it's just 0, right?
> Right, but I thought we normally use a -1px offset.

On elements that have a border, yes. Right here, we don't have a border, so 0 is fine - and actually is using the pixel right outside the image, which is what we want here, IMHO (and that matches the Firefox look).
Blocks: 593512
Blocks: 594776
Blocks: 595024
Blocks: 595155
Assignee: kairo → nobody
Depends on: 595810
Comment on attachment 468675 [details] [diff] [review]
v2.8: update for recent comments, integrate new tests

Doesn't apply any more after bug 595810 part 2 and I have no idea how to update it, so dropping review request.

Feel free to pick up this bug and patch, I can't as long as the API for doorhangers in SeaMonkey isn't clear and I probably will have no time to do much work on anything before November anyhow.
Attachment #468675 - Flags: review?(neil)
Attached patch Test onlySplinter Review
This is just the test from v2.8, but seeking review on two small changes to get it to work with current trunk builds.
* Bug 591026 changed the id used by the popupnotification element
* Changed test #9 to use the password anchor as we don't have addons yet.
Attachment #484663 - Flags: review?(kairo)
No longer blocks: 595155
Comment on attachment 484663 [details] [diff] [review]
Test only

This test, because of the time it has been sitting around, is missing the last three checkins that happened for http://hg.mozilla.org/mozilla-central/log/3ddfa411f716/browser/base/content/test/browser_popupNotification.js - I guess we probably should apply those as well.
Comment on attachment 484663 [details] [diff] [review]
Test only

r=me but please add those changes that were added on the Firefox side so that we end up testing the same things.
Attachment #484663 - Flags: review?(kairo) → review+
Blocks: 608471
Adding to dependencies
Bug 611420: multiple popup notifications should stack vertically and be the same width
Depends on: 611420
No longer depends on: 611420
Gah I meant:
Bug 608123 - Stack multiple notifications vertically instead of horizontally
Depends on: 608123
(In reply to comment #45)
> * Changed test #9 to use the password anchor as we don't have addons yet.
Did KaiRo's patch provided the addons icon solely to get the test to pass?
No longer blocks: 595024
Comment on attachment 468675 [details] [diff] [review]
v2.8: update for recent comments, integrate new tests

>+    run: function () {
>+      this.notifyObj = new basicNotification(),
>+      showNotification(this.notifyObj);
                                                ^
Bah, I've only just noticed this, and it's infiltrated most of the tests :-(
Whiteboard: [doorhanger]
No longer blocks: 593512
Neil can we spin off a new meta bug for the rest of this stuff that didn't make 2.1

Will make life so much easier to track for everyone else.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.