509732 - Don't do slideIn animation if the height of the notification is 0

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: large grey area for addons

was looking at addons in the latest nightly build (20090811...) and I clicked on url fixer, go to page, then download item.  I came back to my browser and looked at the addons and saw the attached screen

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Hmm, I'm betting that is the notification box, without a notification

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

this only seems to happen when installing an addon via an AMO webpage vs the
add-on manager.

Comment 3

[Aakash Desai [:aakashd]]

This is a pretty big UI issue if addons are going to be available to users with fennec1.0. I'm going to set the blocking-fennec? flag.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #1)
> Hmm, I'm betting that is the notification box, without a notification

Strangely there is a notification, but without type nor priority

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm wondering if the fact that panel.hidden==true prevents the xbl binding to be done properly

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

this is still a problem in the 1.9.2 200090903 nightly build

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I believe you

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

right I got the source of the bug I still need to figure out how to patch it.

Because we are hidden the height of the notification is 0 and so the change var is 0 too. (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#222).

This situation lead to have a opacitychange var at 0 (0/0 !!) (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#250) and because of that we never increment the opacity of the notification (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#278)

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Prevent the height to be set to 0

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Don't do slideIn animation if the height of the notification is 0.

Should I also handle slideOut? Actually the removeChild is handle in the slide function but I can add one in the else condition if needed.

Comment 13

[Neil Deakin]

(In reply to comment #12)
> Created an attachment (id=412111) [details]
> Patch
> 
> Don't do slideIn animation if the height of the notification is 0.
> 
> Should I also handle slideOut? Actually the removeChild is handle in the slide
> function but I can add one in the else condition if needed.

If height is 0, then yes you should since
  var opacitychange = change / height;
will likely cause a problem of some kind.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

I've mode the boxObject.height check sooner in the process because showNotification seems to be always called for the top notification, not the one that appear after

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 412838 [details] [diff] [review]
Patch v0.2

>             const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>             var newitem = document.createElementNS(XULNS, "notification");
>             newitem.setAttribute("label", aLabel);
>             newitem.setAttribute("value", aValue);
>             if (aImage)
>               newitem.setAttribute("image", aImage);
>-            if (!insertPos) {
>+            if (!insertPos && newitem.boxObject.height) {
>               newitem.style.position = "fixed";
>               newitem.style.top = "100%";
>             }
>             this.insertBefore(newitem, insertPos);
> 
Hm, can newitem.boxObject.height really be obtained before inserting newitem?

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 412838 [details] [diff] [review]
Patch v0.2

> Hm, can newitem.boxObject.height really be obtained before inserting newitem?

No. That's a mistake from my side

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Comment 18

[Michael Wu [:mwu]]

Comment on attachment 413629 [details] [diff] [review]
Patch v0.3

>-            if (!insertPos) {
>+            var isContainerVisible = this.boxObject.height || this.boxObject.width;
>+            if (!insertPos && isContainerVisible) {
I think there's a chance that this.boxObject.height and width may return 0 without necessarily having "display: none" set if the notification box has nothing inside it. It's possible to use it without putting something inside, possibly causing the element to have 0 width/height. Not 100% sure about this though, and Firefox always puts something inside the notification box.

>-            if (!insertPos)
>+            if (!insertPos && newitem.boxObject.height)
>               this._showNotification(newitem, true);
> 
I guess it's ok to make the requirement that callers know about the height != 0 requirement when calling this internal method, but..

>       <method name="removeNotification">
>         <parameter name="aItem"/>
>         <body>
>           <![CDATA[
>-            if (aItem == this.currentNotification)
>+            if (aItem == this.currentNotification && aItem.boxObject.height)
>               this.removeCurrentNotification();
removeCurrentNotification is a public method and probably shouldn't require callers to check aItem.boxObject.height before calling.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #18)
> (From update of attachment 413629 [details] [diff] [review])
> >-            if (!insertPos) {
> >+            var isContainerVisible = this.boxObject.height || this.boxObject.width;
> >+            if (!insertPos && isContainerVisible) {
> I think there's a chance that this.boxObject.height and width may return 0
> without necessarily having "display: none" set if the notification box has
> nothing inside it. It's possible to use it without putting something inside,
> possibly causing the element to have 0 width/height. Not 100% sure about this
> though, and Firefox always puts something inside the notification box.
> 

yeah, there a chance for that, I didn't check for hidden or display:none because the container can be nested into something that have this.

I'll be happy to find a better way of knowing if an element if invisible or not.
I want to avoid adding check into the __showNotification method because I think it make more sense to no call it if it not need than the inverse.

I also could check this right after the insertBefore call.

> 
> >       <method name="removeNotification">
> >         <parameter name="aItem"/>
> >         <body>
> >           <![CDATA[
> >-            if (aItem == this.currentNotification)
> >+            if (aItem == this.currentNotification && aItem.boxObject.height)
> >               this.removeCurrentNotification();
> removeCurrentNotification is a public method and probably shouldn't require
> callers to check aItem.boxObject.height before calling.

Right that make sense.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 413629 [details] [diff] [review]
Patch v0.3

I need to change a few accordingly to mwu's comments.

Thanks again.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Finally, I've decided to put back the check in _showNotification because this function supervise the state of currentNotification && _closedNotification and I don't want to duplicate code outside it.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao: why did you remove blocking-fennec:1.0+ from this bug? Restoring on the assumption that it wasn't intentional of you to do that.

Comment 23

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

He removed it because it's blocking-1.9.2+ already, I'm assuming.

Comment 24

[Dão Gottwald [:dao]]

I didn't mean to touch it.

Comment 25

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

Ah, so bug 530482 then :)

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 413966 [details] [diff] [review]
Patch

>diff -r 1d8c921ccc49 toolkit/content/widgets/notification.xml
>--- a/toolkit/content/widgets/notification.xml	Thu Nov 19 11:32:52 2009 +0100
>+++ b/toolkit/content/widgets/notification.xml	Mon Nov 23 01:22:09 2009 +0100
>@@ -218,40 +218,46 @@
>               }
>               this._setBlockingState(this.currentNotification);
>             }
> 
>             var height = aNotification.boxObject.height;
>             var change = height / this.slideSteps;
>             var margin;
>             if (aSlideIn) {
>+              this.currentNotification = aNotification;
>+              aNotification.style.removeProperty("position");
>+              aNotification.style.removeProperty("top");
>+
>+              if (height == 0)
>+                return;
>+
>               if (this.currentNotification &&
>                   this.currentNotification.boxObject.height > height)
>                 height = this.currentNotification.boxObject.height;
> 
>-              this.currentNotification = aNotification;
>-              aNotification.style.removeProperty("position");
>-              aNotification.style.removeProperty("top");
>               aNotification.style.marginTop = -height + "px";

By changing this.currentNotification earlier you will change the behaviour of the test immediately following.

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

> By changing this.currentNotification earlier you will change the behaviour of
> the test immediately following.

Adressed.
Thanks for review.

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 414240 [details] [diff] [review]
Patch v0.3

>diff -r 1d8c921ccc49 toolkit/content/widgets/notification.xml
>--- a/toolkit/content/widgets/notification.xml	Thu Nov 19 11:32:52 2009 +0100
>+++ b/toolkit/content/widgets/notification.xml	Tue Nov 24 13:44:43 2009 +0100
>@@ -225,33 +225,39 @@
>             if (aSlideIn) {
>               if (this.currentNotification &&
>                   this.currentNotification.boxObject.height > height)
>                 height = this.currentNotification.boxObject.height;
> 
>               this.currentNotification = aNotification;
>               aNotification.style.removeProperty("position");
>               aNotification.style.removeProperty("top");
>+
>+              if (height == 0)
>+                return;

You need to call _setBlockingState for the new notification.

>               aNotification.style.marginTop = -height + "px";
>               aNotification.style.opacity = 0;
>               margin = -height;
>             }
>             else {
>+              var notifications = this.allNotifications;
>+              var idx = notifications.length - 1;
>+              this.currentNotification = (idx >= 0) ? notifications[idx] : null;
>+
>+              if (height == 0)
>+                return;

You need to remove the old notification and call _setBlockingState for the new currentNotification.

>               change = -change;
>               this._closedNotification = aNotification;
>-              var notifications = this.allNotifications;
>-              var idx = notifications.length - 1;
>-              if (idx >= 0)
>-                this.currentNotification = notifications[idx];
>-              else
>-                this.currentNotification = null;
>               var style = window.getComputedStyle(aNotification, null);
>-              margin = style.getPropertyCSSValue("margin-top").
>-                         getFloatValue(CSSPrimitiveValue.CSS_PX);
>+              margin = style.getPropertyCSSValue("margin-top")
>+                            .getFloatValue(CSSPrimitiveValue.CSS_PX);
>             }
>+
>             var opacitychange = change / height;
>             const FRAME_LENGTH = 50;
> 
>             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);

Can we get some tests for this?

Comment 29

[Dave Townsend [:mossop]]

Also please check the existing tests still pass with the new patch, the current one causes 18 failures.

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4

Addressed comments.

This also passed all the tests and add a a few tests. I've changed the getNotificationWithValue code otherwise it was impossible to remove a notification in a hidden notification box since the binding is not build and the .value return undefined.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 414426 [details] [diff] [review]
Patch v0.4

Couple of minor nits but otherwise this is good to go:

>diff -r 67ba33b024f4 toolkit/content/tests/widgets/Makefile.in
>--- a/toolkit/content/tests/widgets/Makefile.in	Tue Nov 24 15:42:03 2009 -0800
>+++ b/toolkit/content/tests/widgets/Makefile.in	Wed Nov 25 04:18:41 2009 +0100
>@@ -45,16 +45,17 @@ include $(DEPTH)/config/autoconf.mk
> include $(topsrcdir)/config/rules.mk
> 
> _TEST_FILES = 	test_bug360220.xul \
> 		test_bug359754.xul \
> 		test_bug365773.xul \
> 		test_bug382990.xul \
> 		test_bug457632.xul \
> 		test_bug460942.xul \
>+    test_bug509732.xul \

Fix up the indenting

>diff -r 67ba33b024f4 toolkit/content/widgets/notification.xml
>--- a/toolkit/content/widgets/notification.xml	Tue Nov 24 15:42:03 2009 -0800
>+++ b/toolkit/content/widgets/notification.xml	Wed Nov 25 04:18:41 2009 +0100
>@@ -59,17 +59,17 @@
>       </property>
> 
>       <method name="getNotificationWithValue">
>         <parameter name="aValue"/>
>         <body>
>           <![CDATA[
>             var notifications = this.allNotifications;
>             for (var n = notifications.length - 1; n >= 0; n--) {
>-              if (aValue == notifications[n].value)
>+              if (aValue == notifications[n].value || aValue == notifications[n].getAttribute("value"))

Testing both is wasteful since the first checks the second normally anyway. Lets just use the attribute test.

Comment 32

[Dave Townsend [:mossop]]

Attachment: final patch

This is a patch with all nits addressed ready to land.

Comment 33

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/377d745a2242

Comment 34

[Justin Dolske [:Dolske]]

Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/48677fe27b68

Comment 35

[Samuel Sidler (old account; do not CC)]

These bugs landed after b4 was cut. Moving flag out.