Closed Bug 509732 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

ARM
Maemo
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta5-fixed
fennec 1.0+ ---

People

(Reporter: jmaher, Assigned: vingtetun)

References

Details

Attachments

(2 files, 7 obsolete files)

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
Flags: wanted-fennec1.0?
Hmm, I'm betting that is the notification box, without a notification
this only seems to happen when installing an addon via an AMO webpage vs the
add-on manager.
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.
tracking-fennec: --- → ?
(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
I'm wondering if the fact that panel.hidden==true prevents the xbl binding to be done properly
this is still a problem in the 1.9.2 200090903 nightly build
I believe you
Assignee: nobody → mark.finkle
tracking-fennec: ? → 1.0+
Duplicate of this bug: 522062
Duplicate of this bug: 527049
Assignee: mark.finkle → 21
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)
Attached patch Patch (obsolete) — Splinter Review
Prevent the height to be set to 0
Attachment #412095 - Flags: review?(gavin.sharp)
Summary: add-ons area has very large block of grey → Don't do slideIn animation if the height of the notification is 0
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #412095 - Attachment is obsolete: true
Attachment #412111 - Flags: review?(gavin.sharp)
(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.
Attachment #412111 - Attachment is obsolete: true
Attachment #412111 - Flags: review?(gavin.sharp)
Attached patch Patch v0.2 (obsolete) — Splinter Review
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
Attachment #412838 - Flags: review?(enndeakin)
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 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
Attachment #412838 - Flags: review?(enndeakin)
Component: Linux/Maemo → XUL Widgets
Product: Fennec → Toolkit
QA Contact: maemo-linux → xul.widgets
Flags: blocking1.9.2+
Attached patch Patch v0.3 (obsolete) — Splinter Review
Attachment #412838 - Attachment is obsolete: true
Attachment #413629 - Flags: review?(enndeakin)
Attachment #413629 - Flags: review?(enndeakin) → review?(neil)
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.
(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 on attachment 413629 [details] [diff] [review]
Patch v0.3

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

Thanks again.
Attachment #413629 - Flags: review?(neil)
tracking-fennec: 1.0+ → ---
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #413629 - Attachment is obsolete: true
Attachment #413966 - Flags: review?(enndeakin)
Attachment #413966 - Flags: review?(enndeakin) → review?(neil)
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.
tracking-fennec: --- → 1.0+
Whiteboard: [needs review]
He removed it because it's blocking-1.9.2+ already, I'm assuming.
I didn't mean to touch it.
Attachment #413966 - Flags: review?(neil) → review?(dtownsend)
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.
Attachment #413966 - Flags: review?(dtownsend) → review-
Attached patch Patch v0.3 (obsolete) — Splinter Review
> By changing this.currentNotification earlier you will change the behaviour of
> the test immediately following.

Adressed.
Thanks for review.
Attachment #414240 - Flags: review?(dtownsend)
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?
Attachment #414240 - Flags: review?(dtownsend) → review-
Also please check the existing tests still pass with the new patch, the current one causes 18 failures.
Whiteboard: [needs review] → [needs new patch]
Attached patch Patch v0.4 (obsolete) — Splinter Review
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.
Attachment #414240 - Attachment is obsolete: true
Attachment #414426 - Flags: review?(dtownsend)
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.
Attachment #414426 - Flags: review?(dtownsend) → review+
Attached patch final patchSplinter Review
This is a patch with all nits addressed ready to land.
Attachment #413966 - Attachment is obsolete: true
Attachment #414426 - Attachment is obsolete: true
Attachment #414438 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs new patch]
Pushed http://hg.mozilla.org/mozilla-central/rev/377d745a2242
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
These bugs landed after b4 was cut. Moving flag out.
Flags: wanted-fennec1.0?
You need to log in before you can comment on or make changes to this bug.