Bug Tracker

Modify

Ticket #5274 (closed bug: fixed)

Opened 5 years ago

Last modified 4 years ago

Toggle cycles incorrectly with multiple toggle handlers

Reported by: mrspeaker Owned by: brandon
Priority: major Milestone: 1.4
Component: event Version: 1.3.2
Keywords: toggle Cc:
Blocking: Blocked by:

Description

The toggle event stores the current cycle position on the DOM element with this.lastToggle. If you attach multiple toggle handlers to the same event then they use the same state variable, so fire in an unexpected order.

I have attached a possible fix for the issue, which uses the function's guid in conjunction with the state variable - so each handler tracks it's own state.

Attachments

toggleTest.html Download (1.5 KB) - added by mrspeaker 5 years ago.
Example of toggle issue
toggle_state_patch.diff Download (711 bytes) - added by mrspeaker 5 years ago.
Store toggle state in data store using function guid

Change History

Changed 5 years ago by mrspeaker

Example of toggle issue

comment:1 Changed 5 years ago by mrspeaker

Ah, my original patch used the function guid to help store state (instead of the DOM node: $(this).data( 'lastToggle' + fn.guid ) instead of this.lastToggle ) but it doesn't need to be stored in data - it can just go in the function scope as a closure. Much nicer.

Changed 5 years ago by mrspeaker

Store toggle state in data store using function guid

comment:2 Changed 5 years ago by mrspeaker

Ok. re-uploaded the original patch, as that was correct, and the revised patch was not - I had my tests set up incorrectly. The revised patch suffered the same issue as the current toggle when attached to multiple elements.

comment:3 Changed 5 years ago by dmethvin

  • Owner set to brandon
  • Component changed from unfilled to event

comment:4 Changed 5 years ago by mrspeaker

This demo might be more useful to see the issue:  Toggle issue demo. If you click on the boxes you'll see the first does not toggle correctly. With the  patched version it works as expected.

comment:5 Changed 4 years ago by john

  • Status changed from new to closed
  • Resolution set to fixed

The patch worked, although I had to make some tweaks (namely, using jQuery instead of $ and jQuery.data instead of jQuery().data). I also added some tests to verified that the patch worked (in the future - having tests together with the patch makes it a lot easier to land stuff like this).

Thanks a ton for your patch - it looks like it works great!

Landed here:  http://github.com/jquery/jquery/commit/5cb1163469fb2c9cd80712cd5481b29055821022

comment:6 Changed 4 years ago by john

Oh, also, as far as I can tell the fn.guid technique you used is safe. Since the function is going through the proxy handler it'll have a guide by the time that it's used (which was my major concern).

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.