457632 - notificationbox doesn't remove notifications immediately

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dave Townsend [:mossop]]

Attachment: testcase

The attached browser chrome testcase fails when I do not believe it should.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 2

[Dave Townsend [:mossop]]

Attachment: improved testcase

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.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 4

[[e-mail]]

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 ;-)

Comment 5

[Dave Townsend [:mossop]]

(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

Comment 6

[[e-mail]]

(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 7

[[e-mail]]

Comment on attachment 341110 [details] [diff] [review]
patch rev 1

>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
Nit: trailing spaces

Comment 8

[Dave Townsend [:mossop]]

Attachment: ready for checkin

Fixed the one comment, ready to land.

Comment 9

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/b55e95f29973