Enable options by using e.dispatchEvent

[kaicataldo]

The current implementation allows developers to include an optional options parameter but doesn't actually use it. As such, this change should not affect any pre-existing use cases, but opens up the option for those who might want to have more granular control over the event they are simulating.

[jquerybot]

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

---

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

[keik]

👍

[scottgonzalez]

This is an extremely confusing condition, since options are meaningless at this point. Do you know why this condition event exists instead of just always going through dispatchEvent()?

[dmethvin]

It was added in 206bbf8 but that seems way too broad for the bug it's trying to fix. For example it's not passing the event object. The current case will run not only click but also focus and blur which don't seem to need the workaround. I think this may also be just an older IE problem (the YUI ticket link is dead now but gh-1 says it is) so it might be good to isolate this case via a feature detect and not make other browsers do it.

[kaicataldo]

Ah, you're right about that. Definitely needs to me more granular (i.e. options only apply to mouse and key events).

[kaicataldo]

Ah, realized I misunderstood @dmethvin's comment. Seems like it's really just for click events. I'm happy to make the change re: feature detection, but am not sure what that looks like. Will look into that a bit and update the PR.

[dmethvin]

The original bug says this only affects old IE (probably IE8 and lower). Those browsers don't have dispatchEvent so that test should probably be first followed by a test for type === "click" && elem[ type ] or something similar. Try removing the fix but not the tests from 206bbf8 and see what fails, that tell you a lot.

[kaicataldo]

Thanks for the explanation, @dmethvin.

I can't check this in IE8 myself, unfortunately, but the MDN docs corroborate that dispatchEvent is not compatible with versions of IE previous to 9.

Generally though, don't we want to use dispatchEvent() by default and use click() as the fallback? In other words, should we change the order of these checks? Something like:

dispatchEvent: function( elem, type, event ) {
    if ( elem.dispatchEvent ) {
        elem.dispatchEvent( event );
    } else if (type === 'click' && elem[ type ] ) {
        elem[ type ]();
    } else if ( elem.fireEvent ) {
        elem.fireEvent( "on" + type, event );
    }
}

@scottgonzalez The event is being created with supplied options at line 73 and 74:

var event = this.createEvent( type, options );
this.dispatchEvent( elem, type, event, options );

It's just defaulting to click(), etc. because of the current order of the checks. Looks like options used to work and stopped working when the changes @dmethvin pointed to above were merged.

[dmethvin]

Generally though, don't we want to use dispatchEvent() by default and use click() as the fallback? In other words, should we change the order of those tests?

Yes, I think that is right but just haven't done the research to be sure. So something like this, along with some more extensive unit tests:

    dispatchEvent: function( elem, type, event, options ) {

        // Standard W3C event dispatch
        if ( elem.dispatchEvent ) {
            elem.dispatchEvent( event );

        // Ensure the checkbox/radio state changes on IE<=8 (Issue #1)
        } else if ( type === "click" && elem.click && elem.nodeName.toLowerCase() === "input" )
            elem.click(); 

        // IE<=8 event dispatch
        } else if ( elem.fireEvent ) {
            elem.fireEvent( "on" + type, event );
        }
    },

[kaicataldo]

Updated as per our discussion. Thoughts?

[kaicataldo]

Separate variables for clickOptions and keyOptions might be unnecessary since they're currently the same, but thought it would be nice for any potential future options testing if they were split up.

[dmethvin]

The changes look good as far as the logic goes. Tests don't run on IE8 because they check for event.button which isn't supported (you'd need to use event.which). Also the use of [].forEach in the unit tests doesn't work, you'd need to use either an explicit loop or jQuery.each instead. Welcome to the world of browser compat!

Maybe someone else can chime in on how important it is to support IE<=8 in this plugin. Microsoft no longer supports it, so perhaps it would make sense to just skip these tests for IE8 using a simple indirect feature check like window.attachEvent && !window.addEventListener.

Do you have an account on SauceLabs or BrowserStack? I think they have free accounts and you can test IE8 there. Here's a screen shot of the failure on IE8.

[scottgonzalez]

I'm fine with dropping IE8 support.

[kaicataldo]

Thanks for all the helpful guidance! I'll make an update when I get home tonight.

[kaicataldo]

Thanks again for all the helpful comments - this has been a great learning experience for me 👍. Updated the tests to be IE8 compatible and tested with BrowserStack.

Turns out event.button is IE8 compatible, but the values are different!

[dmethvin]

Given that IE8 is a lame duck this LGTM. We're not fixing the button value here for IE8 so it's just passing through the inconsistent value. When a non-old-IE version of jquery-simulate is released this can be pulled out anyway, but since @kaicataldo has done this work we should just land it IMO.

Any other opinions?

[kaicataldo]

If there's anything else I can change I'm happy to do it. Thanks again for the input and help!

[kaicataldo]

Anything else I can do to land this?

[kaicataldo]

Pinging to see if there's anything I can do to land this