Last Comment Bug 570004 - (sm-doorhanger) doorhanger notifications for SeaMonkey
(sm-doorhanger)
: doorhanger notifications for SeaMonkey
Status: RESOLVED FIXED
[doorhanger]
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1final
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
: 580260 (view as bug list)
Depends on: 575957 doorhanger 565187 572967 574228 574230 574530 595810 608123
Blocks: 608471 581974 594776 645417 753765
  Show dependency treegraph
 
Reported: 2010-06-03 13:46 PDT by Robert Kaiser
Modified: 2012-05-21 20:25 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
v1: Implement doorhanger and use it for geolocation (32.24 KB, patch)
2010-06-22 07:14 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.1: don't use Ci.* ;-) (32.22 KB, patch)
2010-06-22 07:51 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2: some comment addressing, incorporate bug 574230 work (33.17 KB, patch)
2010-06-24 16:51 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.1: some comment addressing, incorporate bug 574228 and bug 574230 work (33.59 KB, patch)
2010-06-24 17:41 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.2: some more comment addressing, incorporate bug 574228 and bug 574230 work (33.51 KB, patch)
2010-06-24 18:20 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.3: update for recent comments and checked-in state of bug 574230, also include bug 574530 patch (33.54 KB, patch)
2010-06-26 07:36 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.4: another update for the new bug 574530 patch (33.64 KB, patch)
2010-06-28 08:06 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.5: update bug 575957 and bug 565187 patches (77.32 KB, patch)
2010-07-01 13:27 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.6: get browser more efficiently (81.25 KB, patch)
2010-07-30 08:33 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.7: integrate bug 572967 work (81.69 KB, patch)
2010-08-03 12:36 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.8: update for recent comments, integrate new tests (82.93 KB, patch)
2010-08-24 07:36 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
Test only (20.46 KB, patch)
2010-10-20 05:39 PDT, neil@parkwaycc.co.uk
kairo: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-06-03 13:46:20 PDT
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.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-07 05:54:50 PDT
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/.
Comment 2 Robert Kaiser 2010-06-07 08:48:18 PDT
(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.
Comment 3 Robert Kaiser 2010-06-22 07:14:31 PDT
Created attachment 453054 [details] [diff] [review]
v1: Implement doorhanger and use it for geolocation

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.
Comment 4 Robert Kaiser 2010-06-22 07:51:42 PDT
Created attachment 453062 [details] [diff] [review]
v1.1: don't use Ci.* ;-)

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.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-22 10:54:43 PDT
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...
Comment 6 Robert Kaiser 2010-06-22 11:31:59 PDT
(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?
Comment 7 neil@parkwaycc.co.uk 2010-06-24 14:01:35 PDT
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()
Comment 8 Robert Kaiser 2010-06-24 16:42:35 PDT
(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? :-/
Comment 9 Robert Kaiser 2010-06-24 16:51:27 PDT
Created attachment 453915 [details] [diff] [review]
v2: some comment addressing, incorporate bug 574230 work

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. :)
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-24 16:52:33 PDT
(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!
Comment 11 Robert Kaiser 2010-06-24 17:41:08 PDT
Created attachment 453933 [details] [diff] [review]
v2.1: some comment addressing, incorporate bug 574228 and bug 574230 work

I just noticed I should add the bug 574228 additions to the test as well.
Comment 12 Robert Kaiser 2010-06-24 17:57:14 PDT
(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.
Comment 13 Robert Kaiser 2010-06-24 18:05:53 PDT
(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.
Comment 14 Robert Kaiser 2010-06-24 18:20:48 PDT
Created attachment 453942 [details] [diff] [review]
v2.2: some more comment addressing, incorporate bug 574228 and bug 574230 work

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.
Comment 15 neil@parkwaycc.co.uk 2010-06-25 02:10:05 PDT
(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?
Comment 16 Robert Kaiser 2010-06-26 07:36:25 PDT
Created attachment 454272 [details] [diff] [review]
v2.3: update for recent comments and checked-in state of bug 574230, also include bug 574530 patch

This patch addresses more recent comments, updates to the checked-in state of bug 574230, and also includes the bug 574530 patch.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-27 06:26:06 PDT
(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.
Comment 18 Robert Kaiser 2010-06-28 08:06:25 PDT
Created attachment 454519 [details] [diff] [review]
v2.4: another update for the new bug 574530 patch

Here's a v2.4, just because I can. Or possibly, because the new bug 574530 patch is different again. ;-)
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-28 10:57:51 PDT
(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...
Comment 20 Robert Kaiser 2010-06-30 04:54:46 PDT
(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.
Comment 21 Robert Kaiser 2010-07-01 13:27:49 PDT
Created attachment 455532 [details] [diff] [review]
v2.5: update bug 575957 and bug 565187 patches

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).
Comment 22 Philip Chee 2010-07-20 08:28:37 PDT
*** Bug 580260 has been marked as a duplicate of this bug. ***
Comment 23 neil@parkwaycc.co.uk 2010-07-23 13:39:58 PDT
Bah, doorhangers suck without bug 572967.
Comment 24 neil@parkwaycc.co.uk 2010-07-23 14:01:54 PDT
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;
Comment 25 Robert Kaiser 2010-07-26 06:28:13 PDT
(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.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-07-26 06:37:09 PDT
It's less concise, but it's more efficient.
Comment 27 Robert Kaiser 2010-07-26 08:00:58 PDT
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.
Comment 28 Philip Chee 2010-07-26 08:06:32 PDT
> 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.
Comment 29 Robert Kaiser 2010-07-26 10:14:49 PDT
(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!
Comment 30 Robert Kaiser 2010-07-26 10:40:24 PDT
(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
Comment 31 neil@parkwaycc.co.uk 2010-07-26 13:58:26 PDT
(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...
Comment 32 neil@parkwaycc.co.uk 2010-07-26 14:02:59 PDT
(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.
Comment 33 Robert Kaiser 2010-07-27 04:57:15 PDT
(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.
Comment 34 Robert Kaiser 2010-07-28 13:07:10 PDT
(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.
Comment 35 Robert Kaiser 2010-07-30 08:33:07 PDT
Created attachment 461555 [details] [diff] [review]
v2.6: get browser more efficiently

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).
Comment 36 Robert Kaiser 2010-08-03 12:36:53 PDT
Created attachment 462473 [details] [diff] [review]
v2.7: integrate bug 572967 work

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. ;-)
Comment 37 neil@parkwaycc.co.uk 2010-08-03 15:47:39 PDT
(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 38 neil@parkwaycc.co.uk 2010-08-23 09:35:25 PDT
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.
Comment 39 Robert Kaiser 2010-08-24 07:14:17 PDT
(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.
Comment 40 Robert Kaiser 2010-08-24 07:30:35 PDT
(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.
Comment 41 Robert Kaiser 2010-08-24 07:36:15 PDT
Created attachment 468675 [details] [diff] [review]
v2.8: update for recent comments, integrate new tests

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).
Comment 42 neil@parkwaycc.co.uk 2010-08-24 08:03:02 PDT
(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.
Comment 43 Robert Kaiser 2010-08-24 08:19:06 PDT
(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).
Comment 44 Robert Kaiser 2010-09-22 18:10:12 PDT
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.
Comment 45 neil@parkwaycc.co.uk 2010-10-20 05:39:00 PDT
Created attachment 484663 [details] [diff] [review]
Test only

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.
Comment 46 Robert Kaiser 2010-10-25 03:46:30 PDT
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 47 Robert Kaiser 2010-10-25 07:23:38 PDT
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.
Comment 48 Philip Chee 2010-11-12 10:07:36 PST
Adding to dependencies
Bug 611420: multiple popup notifications should stack vertically and be the same width
Comment 49 Philip Chee 2010-11-12 20:34:53 PST
Gah I meant:
Bug 608123 - Stack multiple notifications vertically instead of horizontally
Comment 50 neil@parkwaycc.co.uk 2010-11-15 05:35:17 PST
(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?
Comment 51 neil@parkwaycc.co.uk 2011-01-29 08:50:29 PST
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 :-(
Comment 52 Justin Wood (:Callek) 2011-05-07 11:19:39 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.