Skip to content

Preserve originalEvent property (for compat) #2336

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.

Sign up for GitHub

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

Closed
markelog wants to merge 1 commit into from
Closed

Preserve originalEvent property (for compat) #2336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,17 +648,26 @@ jQuery.event = {
}
},

// Piggyback on a donor event to simulate a different one
simulate: function( type, elem, event, bubble ) {
// Piggyback on a donor event to simulate a different one.
// Fake originalEvent to avoid donor's stopPropagation, but if the
// simulated event prevents default then we do the same on the donor.
var e = jQuery.extend(
new jQuery.Event(),
Copy link
Member

@dmethvin dmethvin May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of this comment still applies and should be kept. Perhaps you could change "Fake originalEvent" to "Don't set originalEvent" instead?

Copy link
Member Author

@markelog markelog May 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

event,
{
type: type,
isSimulated: true,
originalEvent: {}
isSimulated: true
// Previously, `originalEvent: {}` was set here, so stopPropagation call
// would not be triggered on donor event, since in our own
// jQuery.event.stopPropagation function we had a check for existence of
// originalEvent.stopPropagation method, so, consequently it would be a noop.
//
// But now, this "simulate" function is used only for events
// for whom stopPropagation() is noop, so there is no need for that anymore.
Copy link
Member

@mgol mgol May 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for whom -> for which

//
// For the compat branch though, guard for "click" and "submit"
// events is still used, but was moved to jQuery.event.stopPropagation function
// because `originalEvent` should point to the original event for the constancy
// with other events and for more focused logic
}
);
if ( bubble ) {
Expand Down Expand Up @@ -763,7 +772,8 @@ jQuery.Event.prototype = {
var e = this.originalEvent;

this.isPropagationStopped = returnTrue;
if ( !e ) {

if ( !e || this.isSimulated ) {
return;
}
// If stopPropagation exists, run it on the original event
Expand Down
129 changes: 129 additions & 0 deletions test/unit/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2686,6 +2686,135 @@ test( "Inline event result is returned (#13993)", function() {
equal( result, 42, "inline handler returned value" );
});

test( "preventDefault() on focusin does not throw exception", function( assert ) {
expect( 1 );

var done = assert.async(),
input = jQuery( "<input/>" ).appendTo( "#form" );

input.on( "focusin", function( event ) {
var exceptionCaught;

try {
event.preventDefault();
} catch ( theException ) {
exceptionCaught = theException;
}

assert.strictEqual( exceptionCaught, undefined,
"Preventing default on focusin throws no exception" );

done();
} ).trigger( "focus" );
} );
Copy link
Member

@dmethvin dmethvin May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still really dislike this space.

Copy link
Member

@timmywil timmywil May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

@markelog markelog May 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big +1 from me

Copy link
Member

@arthurvr arthurvr May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still dislike the whole jQuery style guide, but this space looks really wrong IMHO. Though it's consistent so I don't think we should do exceptions (like PEP did for example).

Copy link
Member Author

@markelog markelog May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one likes this, but we all force to use it, sounds like an absurd situation to me.


test( "Donor event interference", function( assert ) {
assert.expect( 10 );

var html = "<div id='donor-outer'>" +
"<form id='donor-form'>" +
"<input id='donor-input' type='radio' />" +
"</form>" +
"</div>";

jQuery( "#qunit-fixture" ).append( html );

jQuery( "#donor-outer" ).on( "click", function( event ) {
assert.ok( true, "click bubbled to outer div" );
assert.equal( typeof event.originalEvent, "object", "make sure originalEvent exist" );
assert.equal( event.type, "click", "make sure event type is correct" );
} );
jQuery( "#donor-input" ).on( "click", function( event ) {
assert.ok( true, "got a click event from the input" );
assert.ok( !event.isPropagationStopped(), "propagation says it's not stopped" );
assert.equal( event.type, "click", "make sure event type is correct" );
assert.equal( typeof event.originalEvent, "object", "make sure originalEvent exist" );
} );
jQuery( "#donor-input" ).on( "change", function( event ) {
assert.equal( typeof event.originalEvent, "object", "make sure originalEvent exist" );
assert.equal( event.type, "change", "make sure event type is correct" );
assert.ok( true, "got a change event from the input" );
event.stopPropagation();
} );

jQuery( "#donor-input" )[ 0 ].click();
} );

test( "originalEvent property for IE8", function( assert ) {
if ( !(/msie 8\.0/i.test( window.navigator.userAgent )) ) {
assert.expect( 1 );
assert.ok( true, "Assertions should run only in IE" );
return;
}

assert.expect( 12 );

var html = "<div id='donor-outer'>" +
"<form id='donor-form'>" +
"<input id='donor-input' type='radio' />" +
"</form>" +
"</div>";

jQuery( "#qunit-fixture" ).append( html );

jQuery( "#donor-outer" ).on( "change", function( event ) {
assert.ok( true, "click bubbled to outer div" );
assert.equal( event.originalEvent.type, "click", "make sure simulated event is a click" );
assert.equal( event.type, "change", "make sure event type is correct" );
} );

jQuery( "#donor-outer" ).on( "click", function( event ) {
assert.ok( true, "click bubbled to outer div" );
assert.equal( event.originalEvent.type, "click", "make sure originalEvent exist" );
} );
jQuery( "#donor-input" ).on( "click", function( event ) {
assert.ok( true, "got a click event from the input" );
assert.ok( !event.isPropagationStopped(), "propagation says it's not stopped" );
assert.equal( event.originalEvent.type, "click", "make sure originalEvent exist" );
assert.equal( event.type, "click", "make sure event type is correct" );
} );
jQuery( "#donor-input" ).on( "change", function( event ) {
assert.equal( event.originalEvent.type, "click", "make sure originalEvent exist" );
assert.equal( event.type, "change", "make sure event type is correct" );
assert.ok( true, "got a change event from the input" );
} );

jQuery( "#donor-input" ).trigger( "click" );
} );

test( "originalEvent property for Chrome, Safari and FF of simulated event", function( assert ) {
var userAgent = window.navigator.userAgent;

if ( !(/chrome/i.test( userAgent ) ||
/firefox/i.test( userAgent ) ||
/safari/i.test( userAgent ) ) ) {
assert.expect( 1 );
assert.ok( true, "Assertions should run only in Chrome, Safari and FF" );
Copy link
Member

@dmethvin dmethvin May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UA string for MS Edge says it's both Chrome and Safari, I don't know if you want it to run there or not but we'll need to deal with it soon.

Copy link
Member Author

@markelog markelog May 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to change this for support tests too, anyway created - #2357

Copy link
Member

@mgol mgol Jun 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2390 for the summary of where we're staying with MS Edge.

return;
}

assert.expect( 4 );

var html = "<div id='donor-outer'>" +
"<form id='donor-form'>" +
"<input id='donor-input' type='radio' />" +
"</form>" +
"</div>";
Copy link
Member Author

@markelog markelog May 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To all - i believe this is a best way for creating fixtures, not in the actual qunit html.

Copy link
Member

@timmywil timmywil May 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

@markelog markelog May 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For improved atomicity and encapsulation.

  1. If this test would become obsolete and you would need to remove it you might forget to remove them too, this happened couple times before.
  2. If there would be different assertion like equal( $( "div" ).length, 2 ) you would break it
  3. Other tests might start use those fixtures too and then when you would need to change it, for regression testing or such, you might break other tests.

And stuff like that

Copy link
Member

@timmywil timmywil May 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure, I thought you were saying this was better in all cases.

Copy link
Member

@dmethvin dmethvin May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like having the test markup in the test. When I was rewriting events back in 1.7 the big index.html was a nightmare to follow so I tended to do what @markelog did here for new tests. For selector tests I think it's good to have a reasonably complex document to query against though, so I guess it depends.

Copy link
Member

@gibson042 gibson042 May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To all - i believe this is a best way for creating fixtures, not in the actual qunit html.

I agree, but we should be creating and injecting content with supportjQuery instead of jQuery (i.e., a fixed version distinct from the code under test).

Copy link
Member Author

@markelog markelog May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of supportjQuery would help you create an isolated env for you, that is cool, but that is a unit-test which means you testing smallest possible action and change should be atomic, so really, you would need to use this entity only when you create an environment through the thing you test - only in if you expect that your changes could affect other parts of library.

And even if your change has effect on assertion output it probably be false negative then false positive which would additionally indicate that something is wrong.

In other words, if you test jQuery.get you could use jQuery#html in your tests, because your changes on jQuery.get shouldn't influence entirely different module (at least in most cases) since it should be atomic and even if it does, then jQuery.get assertions would fail then give right output. But if you need to use jQuery.get to create fixtures for jQuery.get assertions then you should use supportjQuery.

I'm not sure what is right thing to do, best action might be to go from the real-life example, although it seems supportjQuery is the most correct way. But then there is a consistency issue, we are not gonna rewrite 6 thousands tests because of it, do we?

Now, tests with wrong kinds of fixtures is another story, we probably should start rewriting at least some of them, since we need it to use more flexible testing tools like karma.


jQuery( "#qunit-fixture" ).append( html );

jQuery( "#donor-outer" ).on( "focusin", function( event ) {
assert.ok( true, "focusin bubbled to outer div" );
assert.equal( event.originalEvent.type, "focus",
"make sure originalEvent type is correct" );
assert.equal( event.type, "focusin", "make sure type is correct" );
} );
jQuery( "#donor-input" ).on( "focus", function() {
assert.ok( true, "got a focus event from the input" );
} );
jQuery( "#donor-input" ).trigger( "focus" );
} );

// This tests are unreliable in Firefox
if ( !(/firefox/i.test( window.navigator.userAgent )) ) {
test( "Check order of focusin/focusout events", 2, function() {
Expand Down