Skip to content

Mouse: reset click event suppression on next mousedown. Fixes #6946 - Mouse: click event suppressed after drag in Gecko #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
2 commits merged into from
Mar 7, 2011

Conversation

joshvarner
Copy link
Contributor

It seems that Firefox and Opera (may be others) will not fire a click event if the element's DOM position is altered by a mouseup handler, while Webkit will. If you have a .live('click', func) listening on the <li> elements of a sortable <ul>:

  1. Drag an item to reorder it.
  2. Click the same item to trigger the .live(). (Works in Webkit, not in others)
  3. Click the same item again (Works in all)

This is caused by the 'preventClickEvent' flag added to the element in mouse._mouseUp(). If the click event never fires, it never resets. This pull request checks for and clears the flag during mousedown, in case the browser didn't fire a click event.

@scottgonzalez
Copy link
Member

It doesn't really make sense for the simulate plugin to have an option to suppress the click event. That's not something a user can manually do.

@joshvarner
Copy link
Contributor Author

It's necessary to be able to test the differences in browser behavior that I described, where WebKit sends the click event while Gecko does not. Otherwise, you wouldn't be able to test the change to ui.mouse.js.

@rdworth
Copy link
Contributor

rdworth commented Feb 5, 2011

This may be a point at which we have to recognize a design constraint of the simulate plugin. It's not designed to simulate what each browser will differently do. For example, some browsers trigger different numbers of events, at times in different orders. Where such differences exist, we have to pick a reasonable sub- or super-set based on what's most common or most sane or most close to spec or least problematic or most helpful in finding bugs, etc. As much as possible, simulate should behave the same in every browser, even if that means not exactly matching what the browser might natively do. While that may leave some holes in terms of testing, and especially automated testing, it may only be possible to fill those whole with actual genuine events, which for automated testing means remote controlling the input device(s) or browser.

In short, simulate only gets us so far and should not grow to contain every event difference in all supported browsers.

@joshvarner
Copy link
Contributor Author

My goal was just to be able to write a test to cover the fix I applied to jquery.ui.mouse.js. But I see the point about drawing a line in regards to the simulate plugin. I can imagine it would be a much different beast if you tried to account for all of the different browsers and JavaScript engines. Should I submit another pull request with the jquery.ui.mouse.js fix only?

@scottgonzalez
Copy link
Member

That would be good. Can you also file a bug at http://bugs.jqueryui.com/newticket and reference the ticket in the commit message? See http://wiki.jqueryui.com/w/page/25941597/Commit-Message-Style-Guide for details.

… Mouse: click event suppressed after drag in Gecko
@joshvarner
Copy link
Contributor Author

GitHub automatically adjusted this pull request to include the new commit instead of the old one, so I'll just leave this one open if that's okay. Also, I created the requested ticket: http://bugs.jqueryui.com/ticket/6946

@rdworth
Copy link
Contributor

rdworth commented Feb 5, 2011

As long as we recognize this fix can't be tested using simulate, how about a visual test ( /tests/visual/mouse/mouse_ticket_6946.html ) that contains a test and steps on how to manual test in each relevant browser. This will be especially helpful as mouse (with all interactions) gets refactored, to guard against regression.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants