Stop dispatching the unused AlertClose event

RESOLVED INVALID

Status

--
minor
RESOLVED INVALID
8 years ago
4 years ago

People

(Reporter: wesj, Unassigned)

Tracking

Trunk
All
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

8 years ago
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).

Comment 1

8 years ago
I second this. And instead of stating "New in Mobile 2" on page https://developer.mozilla.org/en/XUL/notificationbox 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
Status: NEW → ASSIGNED
Created attachment 647436 [details] [diff] [review]
Patch (v1)

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)

Updated

6 years ago
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
(Reporter)

Comment 6

6 years ago
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:
https://mxr.mozilla.org/projects-central/source/elm/browser/metro/base/content/bindings/notification.xml#81

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
Status: ASSIGNED → NEW
(Reporter)

Comment 10

6 years ago
Pushing this over to metro firefox and cc'ing bbondy just so you guys know this code is there... doing nothing.
Component: General → General
OS: All → Windows 8 Metro
Product: Fennec → Firefox
(Reporter)

Updated

6 years ago
Attachment #647436 - Flags: feedback?(wjohnston)
(Reporter)

Comment 11

6 years ago
Adding Matt since he's back in this code!

Updated

6 years ago
Component: General → General
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: dev-doc-needed
Resolution: --- → WONTFIX
I don't understand comment 14. http://mxr.mozilla.org/mozilla-central/search?string=%22AlertClose%22 shows that AlertClose is still around. Even if it wasn't, wouldn't that make this bug FIXED/WORKSFORME?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Updated

5 years ago
Attachment #647436 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #15)
> I don't understand comment 14.
> http://mxr.mozilla.org/mozilla-central/search?string=%22AlertClose%22 shows
> that AlertClose is still around. Even if it wasn't, wouldn't that make this
> bug FIXED/WORKSFORME?

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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → INVALID
Summary: Stop dispatching the unused AlertClosed event → Stop dispatching the unused AlertClose event
(Assignee)

Updated

4 years ago
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.