Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

(Blocks: 1 bug, {fixed1.8.1})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

12 years ago
If notifications are added to the notificationbox too quickly, the notification box gets confused.
(Assignee)

Comment 1

12 years ago
Created attachment 235615 [details]
A symptom of a race in the notificationbox
(Assignee)

Comment 2

12 years ago
Created attachment 235961 [details] [diff] [review]
Move important stuff out of animation callback

This patch makes notificationbox not depend on the completion of the animation.
Assignee: nobody → michael.wu
Status: NEW → ASSIGNED
Attachment #235961 - Flags: first-review?(enndeakin)
(Assignee)

Comment 3

12 years ago
Created attachment 235962 [details] [diff] [review]
Move important stuff out of animation callback, v2

Make sure the notification is visible if we're killing its animation callback to put a new notification in.
Attachment #235961 - Attachment is obsolete: true
Attachment #235962 - Flags: first-review?(enndeakin)
Attachment #235961 - Flags: first-review?(enndeakin)
(Assignee)

Comment 4

12 years ago
Created attachment 236013 [details] [diff] [review]
Move important stuff out of animation callback, v3

Last patch broke the animated closing. This version doesn't.
Attachment #235962 - Attachment is obsolete: true
Attachment #236013 - Flags: first-review?(enndeakin)
Attachment #235962 - Flags: first-review?(enndeakin)

Comment 5

12 years ago
Comment on attachment 236013 [details] [diff] [review]
Move important stuff out of animation callback, v3

This appears to be a branch patch, which doesn't have 343574 in it.

I tested the patch but it didn't seem to work properly when closing a notification when two were open, testing with http://xulplanet.com/ndeakin/tests/xts/notification/notificationbox-browser.xul
Attachment #236013 - Flags: first-review?(enndeakin) → first-review-
(Assignee)

Comment 6

12 years ago
Created attachment 236980 [details] [diff] [review]
Move important stuff out of animation callback, v4

Ported to trunk version of notification.xml, tested/debugged with http://xulplanet.com/ndeakin/tests/xts/notification/notificationbox-browser.xul
Attachment #236013 - Attachment is obsolete: true
Attachment #236980 - Flags: first-review?(enndeakin)

Updated

12 years ago
Attachment #236980 - Flags: first-review?(enndeakin) → first-review+
(Assignee)

Comment 7

12 years ago
Checking in toolkit/content/widgets/notification.xml;
/cvsroot/mozilla/toolkit/content/widgets/notification.xml,v  <--  notification.xml
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

12 years ago
Comment on attachment 236980 [details] [diff] [review]
Move important stuff out of animation callback, v4

This patch is necessary if we want bug 348576. It fixes regressions due to the animation code that is used now and makes it possible to open two notifications at the same time without racing.
Attachment #236980 - Flags: approval1.8.1?
(Assignee)

Comment 9

12 years ago
Created attachment 237775 [details] [diff] [review]
Move important stuff out of animation callback, v4 (branch)
Attachment #237775 - Flags: approval1.8.1?
(Assignee)

Updated

12 years ago
Attachment #236980 - Flags: approval1.8.1?

Comment 10

12 years ago
Comment on attachment 237775 [details] [diff] [review]
Move important stuff out of animation callback, v4 (branch)

a=schrep for regression fix in new functionality.
Attachment #237775 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 11

12 years ago
Branch:
Checking in toolkit/content/widgets/notification.xml;
/cvsroot/mozilla/toolkit/content/widgets/notification.xml,v  <--  notification.xml
new revision: 1.1.2.7; previous revision: 1.1.2.6
done
(Assignee)

Updated

12 years ago
Blocks: 352450
robert, is this the bug you were showing me today?
Hey Sir Seth,
This is for the race condition that mwu found.
bug 348576 is for getting the add-ons mgr. to use notificationbox.
I don't believe there is a bug for the notificationbox displaying full height then animating from a min height back to full height but I haven't searched for one as of yet. I have a couple of ideas of how this can be improved / fixed for 3.0
You need to log in before you can comment on or make changes to this bug.