Bug Tracker

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#8982 closed bug (fixed)

bind("unload someOther") => on unload, handler is not executed only once.

Reported by: rkatic Owned by: dmethvin
Priority: low Milestone: 1.7
Component: event Version: 1.5.2
Keywords: Cc: dmethvin
Blocked by: Blocking:


Change History (11)

comment:1 Changed 6 years ago by rkatic

Using specail events?

comment:2 Changed 6 years ago by Rick Waldron

Component: unfiledevent
Keywords: needsreview added

While this bug is valid, it's also quite irksome because the code to achieve it is simply bad practice and bad code.

When would you ever manually trigger unload twice like: .trigger("unload").trigger("unload"); ?

Additionally, if you read the docs, unload isn't reliable: http://api.jquery.com/unload

This is the result of my further attempts at reduction: http://jsfiddle.net/rwaldron/9Apxs/

comment:3 Changed 6 years ago by rkatic

Sorry, I realized only now that I couldn't be more unclear. The bug is how the "unload" case is handled in .bind()/.one(). Currently, if type === "unload" then .one() is used to register the event, but it ignores the fact that type can also be "unload foo" or "unload.boo", in which cases the test will not pass.

Now, for consistency, or we remove the .one() thing, or we have to move such workaround in another place (special events seams the right way to do that).

The reason why I have not explain myself in the first place, was because on opening the ticket I had also the intention to write a patch that would make clear the problem. Later I decided to wait your opinion if such workaround is really needed at all...

Last edited 6 years ago by rkatic (previous) (diff)

comment:4 Changed 6 years ago by timmywil

Priority: undecidedlow
Status: newopen

comment:5 Changed 6 years ago by Rick Waldron

Can you provide some kind of wide reaching, real-world use case where an unload event would need to be namespaced (not hard to imagine) or manually triggered twice (this is a stretch)?

It still seems to me that "fixing" to benefit something that just seems incorrect or edge-case at best is not something jQuery should be in the business of.

This is just to note that nothing is wrong when unload is only manually triggered once: http://jsfiddle.net/rwaldron/9Apxs/8/

comment:6 Changed 6 years ago by rkatic

The problem is not how the event is triggered, but how the "unload" workaround is implemented on binding the handler.

In my opinion the unload event would not be triggered manually neither once, and for me the only real-world example would be the one where that event is fired more then once by the browser itself. If such bug exists in any browser, the workaround is probably needed, but if there is no such bug, then I think we would remove it. You tell me.

This ticket is primarily about the inconsistency introduced by the current workaround, and not about the necessity of it. Maybe the title of the ticket is misleading?

comment:7 Changed 6 years ago by Rick Waldron

Cc: dmethvin added

One thing I've learned is that everything that happens internally in jQuery does so for a reason. Maybe Dave Methvin can shed some historical light on this one.

comment:8 Changed 6 years ago by dmethvin

Keywords: needsdocs added; needsreview removed
Resolution: wontfix
Status: openclosed

@rkatic I agree that this is a bug, as I recall it is required by a bug in IE6/7 that will leak memory if the event is not removed. To me the easiest way to fix it is to simply document that the unload event must be bound by itself and not in combination with other events. In the event rewrite I looked at other solutions and they are simply not worth the waste of bytes for such a rare and non-recommended case. (Binding to unload defeats the bfcache in Firefox last I looked for example, so it's not a good idea to do it if there is any other way to accomplish the task.)

comment:9 Changed 6 years ago by dmethvin

Resolution: wontfix
Status: closedreopened

comment:10 Changed 6 years ago by dmethvin

Keywords: needsdocs removed
Milestone: 1.next1.7
Owner: set to dmethvin
Status: reopenedassigned

While walking the dog (when I do my best thinking) I realized we don't need the .one() anymore:


At least I am pretty sure that is why it was there originally. So I'll take it out for 1.7.

comment:11 Changed 6 years ago by dmethvin

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.