Core: register validateDelegate also for checkables on focusin, focusout... #1065
Conversation
|
@jzaefferer any feedback? edit: never mind, disussion takes places in the linked issue #1062 |
|
Yeah, might as well have asked that here. Let's stick with the PR for less confusion. Based on your feedback, could you add a test that involves the elements and events you're interested in? Will help prevent future regressions. Thanks. |
|
@jzaefferer added tests. hardest part was getting the whitespaces right... my editor and I are more used to spaces, instead of tabs ;-) |
|
(off-topic) the codestyle checker on travis did not throw an error because of the tabs / spaces mix I had at first.. |
| var input, i, events, triggeredEvents = 0; | ||
|
|
||
| $("#form").validate({ | ||
| debug: true, |
There was a problem hiding this comment.
Does this have to be here?
There was a problem hiding this comment.
copied from a test in this file, was not sure if it is required or why it was actually their.
|
@jzaefferer adjusted per your feedback. everything still green commit messages adjusted, hopefully in a correct way ;-). |
| "focusin focusout keyup", delegate) | ||
| .validateDelegate("[type='radio'], [type='checkbox'], select, option", "click", delegate); | ||
| .validateDelegate("option", "click", delegate); |
There was a problem hiding this comment.
Did you test that handling the click on option elements actually works consistently? If not, this might cause a regression.
There was a problem hiding this comment.
@jzaefferer I did not change handling of option elements or the even listeners on them... do you think it didn't work correctly before my change?
There was a problem hiding this comment.
You removed click events on selects. I don't know if handling clicks on options is good enough to justify that change. I may have known before, since I likely wrote the code, but its been between 4 to 8 years ;)
There was a problem hiding this comment.
@jzaefferer had a few minutes to test it... select with click is required for IE <= 8 (tested by switching document-mode in the dev-tools of IE11). Even in Chrome 35 it is required.
In FF it works without it.
|
Tests are looking good now, thanks. |
|
adjusted the PR accordingly. ready to land. |
|
@jzaefferer any news here? |
|
Thanks for the reminder. Landed with an updated commit message. |
... and keyup. fixes #1062.