notificationbox doesn't remove notifications immediately

RESOLVED FIXED in mozilla1.9.1b2

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla1.9.1b2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted file testcase (obsolete) —
The attached browser chrome testcase fails when I do not believe it should.
We animate the removal if you're removing the current notification, so it doesn't happen immediately. We could probably flag the notification somehow as in the process of being removed, and refuse to return it from getNotification, I guess... We'd probably want to make sure it wasn't otherwise retrievable too (e.g. via currentNotification).
Assignee

Comment 2

11 years ago
Posted file improved testcase (obsolete) —
Yeah I meant programmatically. Whether it is still displayed on screen as it goes away is another issue.

So an improved testcase would be something like this? (ignore the excessive timeouts for now).

I can't actually tell you how well this testcase passes, it seems to induce some catastrophic failure in notificationbox leaving to an infinite loop.
Attachment #340871 - Attachment is obsolete: true
Assignee

Comment 3

11 years ago
Posted patch patch rev 1 (obsolete) — Splinter Review
This removes the closing notification from allNotifications and hence from getNotificationWithValue. currentNotification was already never the closing notification.

It also sanely copes with calling removeNotification for the front most notification twice in succession by ignoring the second call (arguably we could throw on the second call). This stops us removing the notification from the document while it is still being animated away.

It also properly cancels the timer after performing an immediate removeAllNotifications, otherwise things fail when the timer completes and it attempts to remove any already removed closing node.

There was also a failure if the notificationbox is removed from the document while a notification is being animated. This detaches the binding and so none of the XBL methods or fields are available when the timer next fires. I've added code to detect that the element is disconnected and so cancel the timer and bail out.

The existing mochitests all pass and I've added some new ones to test the specific bits I've changed.
Assignee: nobody → dtownsend
Attachment #340875 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #341110 - Flags: review?
Assignee

Updated

11 years ago
Attachment #341110 - Flags: review? → review?(neil)
Comment on attachment 341110 [details] [diff] [review]
patch rev 1

>-      <property name="allNotifications" readonly="true"
>-                onget="return this.getElementsByTagName('notification');"/>
>+      <property name="allNotifications" readonly="true">
>+        <getter>
>+        <![CDATA[
>+          var notifications = [];
>+          var list = this.getElementsByTagName('notification');
>+          for (let i = 0; i < list.length; i++)
>+            if (list[i] != this._closedNotification)
>+              notifications.push(list[i]);
>+          return notifications;
I don't suppose we can use Array.filter to do this?
Do people expect to be able to call .allNotifications.item(N)?

>-            function slide(self, off) {
>+            function slide(self, args, off) {
>+              // If the notificationbox is disconnected then stop the timer
>+              if (self.ownerDocument.compareDocumentPosition(self) &
>+                  Node.DOCUMENT_POSITION_DISCONNECTED) {
>+                clearInterval(args.timer);
Why not clearInterval(self._timer); ?

>               clearInterval(self._timer);
>               self._timer = null;
>+              if (self._closedNotification) {
>+                self.removeChild(self._closedNotification);
>+                self._closedNotification = null;
>+              }
This is getting repetitive ;-)
Assignee

Comment 5

11 years ago
(In reply to comment #4)
> (From update of attachment 341110 [details] [diff] [review])
> >-      <property name="allNotifications" readonly="true"
> >-                onget="return this.getElementsByTagName('notification');"/>
> >+      <property name="allNotifications" readonly="true">
> >+        <getter>
> >+        <![CDATA[
> >+          var notifications = [];
> >+          var list = this.getElementsByTagName('notification');
> >+          for (let i = 0; i < list.length; i++)
> >+            if (list[i] != this._closedNotification)
> >+              notifications.push(list[i]);
> >+          return notifications;
> I don't suppose we can use Array.filter to do this?
> Do people expect to be able to call .allNotifications.item(N)?

A NodeList isn't an array so I couldn't get that to work, maybe I'm missing something though.

> >-            function slide(self, off) {
> >+            function slide(self, args, off) {
> >+              // If the notificationbox is disconnected then stop the timer
> >+              if (self.ownerDocument.compareDocumentPosition(self) &
> >+                  Node.DOCUMENT_POSITION_DISCONNECTED) {
> >+                clearInterval(args.timer);
> Why not clearInterval(self._timer); ?

Because when removed from the document the binding seems to get detached and the field _timer vanishes, so self._timer is null.

> >               clearInterval(self._timer);
> >               self._timer = null;
> >+              if (self._closedNotification) {
> >+                self.removeChild(self._closedNotification);
> >+                self._closedNotification = null;
> >+              }
> This is getting repetitive ;-)

Well I could pull that into it's own tiny method I guess. For the 3 calls I think it almost takes up more lines that way though
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 341110 [details] [diff] [review])
> > >-      <property name="allNotifications" readonly="true"
> > >-                onget="return this.getElementsByTagName('notification');"/>
> > >+      <property name="allNotifications" readonly="true">
> > >+        <getter>
> > >+        <![CDATA[
> > >+          var notifications = [];
> > >+          var list = this.getElementsByTagName('notification');
> > >+          for (let i = 0; i < list.length; i++)
> > >+            if (list[i] != this._closedNotification)
> > >+              notifications.push(list[i]);
> > >+          return notifications;
> > I don't suppose we can use Array.filter to do this?
> > Do people expect to be able to call .allNotifications.item(N)?
> A NodeList isn't an array so I couldn't get that to work, maybe I'm missing
> something though.
return Array.filter(notifications, function(e) e != closedNotification);
(but that doesn't support .item unlike a real nodelist)

I hope nobody calls allNotifications once hoping it to stay live ;-)
> > >+                clearInterval(args.timer);
> > Why not clearInterval(self._timer); ?
> Because when removed from the document the binding seems to get detached and
> the field _timer vanishes, so self._timer is null.
Thanks, that makes sense.

> > >               clearInterval(self._timer);
> > >               self._timer = null;
> > >+              if (self._closedNotification) {
> > >+                self.removeChild(self._closedNotification);
> > >+                self._closedNotification = null;
> > >+              }
> > This is getting repetitive ;-)
> Well I could pull that into it's own tiny method I guess. For the 3 calls I
> think it almost takes up more lines that way though
Oh, I'd counted one, two, many ;-)
Comment on attachment 341110 [details] [diff] [review]
patch rev 1

>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
Nit: trailing spaces
Attachment #341110 - Flags: review?(neil) → review+
Assignee

Comment 8

11 years ago
Fixed the one comment, ready to land.
Attachment #341110 - Attachment is obsolete: true
Attachment #341616 - Flags: review+
Flags: in-testsuite+
Assignee

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
You need to log in before you can comment on or make changes to this bug.