Bug Tracker

Ticket #8982 (closed bug: fixed)

Opened 4 years ago

Last modified 3 years ago

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
Blocking: Blocked by:

Description

Change History

comment:1 Changed 4 years ago by rkatic

Using specail events?

comment:2 Changed 4 years ago by rwaldron

  • Keywords needsreview added
  • Component changed from unfiled to event

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 4 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 4 years ago by rkatic (previous) (diff)

comment:4 Changed 3 years ago by timmywil

  • Priority changed from undecided to low
  • Status changed from new to open

comment:5 Changed 3 years ago by rwaldron

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 3 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 3 years ago by rwaldron

  • 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 3 years ago by dmethvin

  • Keywords needsdocs added; needsreview removed
  • Status changed from open to closed
  • Resolution set to wontfix

@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 3 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution wontfix deleted

comment:10 Changed 3 years ago by dmethvin

  • Keywords needsdocs removed
  • Owner set to dmethvin
  • Status changed from reopened to assigned
  • Milestone changed from 1.next to 1.7

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.

comment:11 Changed 3 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.