Closed Bug 609930 Opened 9 years ago Closed 6 years ago

Stop dispatching the unused AlertClose event


(Firefox for Metro Graveyard :: General, defect, minor)

Windows 8.1
Not set


(Not tracked)



(Reporter: wesj, Unassigned)



(1 obsolete file)

Fennec implemented an AlertClose event, fired when notification boxes are closed. It would be nice to have it in toolkit instead of mb so that all our apps can use it (they already have an AlertOpen event).
I second this. And instead of stating "New in Mobile 2" on page it's more helpful to state "Only available on Mobile 2".
I found this bug while researching a patch I'm working on for Bug 752988 - Chrome Scratchpad focus bug ...

For the solution there, I'm having to add an AlertClose to the toolkit/notification.xml button ... how much of this bug does that change satisfy?
Assignee: nobody → markcapella
Attached patch Patch (v1) (obsolete) — Splinter Review
I added the new AlertClose event to the notification box à la Fennec, and a quickie mochitest for both AlertActive and AlertClose events. Can you give this a look?
Attachment #647436 - Flags: review?(dao)
Comment on attachment 647436 [details] [diff] [review]
Patch (v1)

I don't understand why we should add this. The event appears to be unused in Fennec, so it seems that we should just remove it there.
Attachment #647436 - Flags: review?(dao) → review-
Comment on attachment 647436 [details] [diff] [review]
Patch (v1)

Yes I see. The AlertClose event was fired and listened for in Bug 509408 - when getting notification asking to allow popup, it covers the url bar ...

The EventListener was removed somewhere along the line, but the dispatch was left hanging.

I'm asking :wesj in Fennec if I can remove it for code clean up and to complete this bug, as he was involved here back when.
Attachment #647436 - Flags: feedback?(wjohnston)
Component: XP Toolkit/Widgets: XUL → General
Keywords: dev-doc-needed
Product: Core → Fennec
Summary: Add AlertClosed event to notification boxes → Stop dispatching the unused AlertClosed event
Version: unspecified → Trunk
We don't use notifications.xml in Native Fennec anymore. I don't think doing code cleanup on XUL Fennec is worth it. We risk breaking something and having no idea we broke it because no one is using/testing it anymore.

Note Metro Firefox is also still using this code though:

Removing it there might be better cleanup. Sorry to keep bumping you around :(
Interesting! I've never seen that code area before "This is the home for incomplete mozilla-central projects." ...

Maybe this patch should just be WONTFIX'ed, as Widgets doesn't want to add the AlertClose, and Fennec doesn't want to drop their AlertActive ?
Sorry, "Fennec / doesn't want to drop their AlertClose."
Dropping off this one until further clarification.
Assignee: markcapella → nobody
Pushing this over to metro firefox and cc'ing bbondy just so you guys know this code is there... doing nothing.
OS: All → Windows 8 Metro
Product: Fennec → Firefox
Attachment #647436 - Flags: feedback?(wjohnston)
Adding Matt since he's back in this code!
Product: Firefox → Firefox for Metro
I am interested in AlertClose so I can write mochitests against global-notificationbox for FHR. IMO an explicit event is much more useful than sniffing lower-level DOM events. Although poking around, it does appear there is an undocumented "Removed" event in notification.xml:169. Maybe this is part of some DOM event magic. I don't know DOM that well. I dunno.
"removed" isn't a DOM event. It's just a call to a function that you may add as appendNotification's last argument.
This is no longer unused on Metro.
Closed: 6 years ago
Keywords: dev-doc-needed
Resolution: --- → WONTFIX
I don't understand comment 14. shows that AlertClose is still around. Even if it wasn't, wouldn't that make this bug FIXED/WORKSFORME?
Resolution: WONTFIX → ---
Attachment #647436 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #15)
> I don't understand comment 14.
> shows
> that AlertClose is still around. Even if it wasn't, wouldn't that make this

What I said in comment 14 is that it is "no longer unused" -- i.e. it is still around, but we actually use it now, so we don't want to remove it.  I guess INVALID is a better resolution.
Closed: 6 years ago6 years ago
Resolution: --- → INVALID
Summary: Stop dispatching the unused AlertClosed event → Stop dispatching the unused AlertClose event
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.