Skip to main content

Bug Tracker

Side navigation

#8982 closed bug (fixed)

Opened April 27, 2011 12:19AM UTC

Closed September 20, 2011 12:59PM UTC

Last modified March 08, 2012 07:15PM UTC

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:
Description
Attachments (0)
Change History (11)

Changed April 27, 2011 01:23AM UTC by rkatic comment:1

Using specail events?

Changed April 27, 2011 02:21AM UTC by rwaldron comment:2

component: unfiledevent
keywords: → needsreview

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/

Changed April 27, 2011 03:41AM UTC by rkatic comment:3

_comment0: 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).1303877738970840

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...

Changed April 27, 2011 04:01PM UTC by timmywil comment:4

priority: undecidedlow
status: newopen

Changed April 27, 2011 06:59PM UTC by rwaldron comment:5

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/

Changed April 28, 2011 01:07AM UTC by rkatic comment:6

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?

Changed April 28, 2011 01:36AM UTC by rwaldron comment:7

cc: → dmethvin

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.

Changed September 01, 2011 05:14PM UTC by dmethvin comment:8

keywords: needsreviewneedsdocs
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.)

Changed September 01, 2011 09:44PM UTC by dmethvin comment:9

resolution: wontfix
status: closedreopened

Changed September 01, 2011 09:46PM UTC by dmethvin comment:10

keywords: needsdocs
milestone: 1.next1.7
owner: → dmethvin
status: reopenedassigned

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

https://github.com/jquery/jquery/commit/37d297c67fe3569a5a9d81586e14d4a887df7879

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

Changed September 20, 2011 12:59PM UTC by dmethvin comment:11

resolution: → fixed
status: assignedclosed