Skip to content
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

Jump to bottom

Checking on the event the relatedTarget exists. fixes #2024 #2025

Closed
wants to merge 2 commits into from
Closed

Checking on the event the relatedTarget exists. fixes #2024 #2025

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e6001e
Checking on the event the relatedTarget exists before setting its pro…
Apr 9, 2015
9a4c700
Removing the test event helper and simply testing against an object i…
Apr 13, 2015
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,5 @@ sandbox/*

test/coverage/*
.coveralls.yml

.idea/
5 changes: 2 additions & 3 deletions .jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@
"_V_",
"goog",
"console",

"require",
Copy link
Member

Choose a reason for hiding this comment

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

The line breaks here provided a bit of categorization since we can't use comments in JSON. It's kind of window vars, node vars, and testing vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem; sorry I'm a little ocd about these things! I'll revert

"define",
"module",
"exports",
"process",

"q",
"asyncTest",
"deepEqual",
Expand All @@ -39,6 +37,7 @@
"stop",
"strictEqual",
"test",
"sinon"
"sinon",
"MouseEvent"
]
}
10 changes: 5 additions & 5 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ module.exports = function(grunt) {
novtt: { src: './build/temp/video.js', dest: './build/temp/alt/video.novtt.js' },
dist: { expand: true, cwd: 'build/temp/', src: ['**/**'], dest: 'dist/', filter: 'isFile' },
examples: { expand: true, cwd: 'build/examples/', src: ['**/**'], dest: 'dist/examples/', filter: 'isFile' },
cdn: { expand: true, cwd: 'dist/', src: ['**/**'], dest: 'dist/cdn/', filter: 'isFile' },
cdn: { expand: true, cwd: 'dist/', src: ['**/**'], dest: 'dist/cdn/', filter: 'isFile' }
},
aws_s3: {
options: {
Expand Down Expand Up @@ -281,7 +281,7 @@ module.exports = function(grunt) {
build: {
options: {},
files: {
'build/temp/video.js.map': ['build/temp/video.js'],
'build/temp/video.js.map': ['build/temp/video.js']
}
}
},
Expand All @@ -293,11 +293,11 @@ module.exports = function(grunt) {
concat: {
vtt: {
options: {
separator: '\n',
separator: '\n'
},
src: ['build/temp/video.js', 'node_modules/vtt.js/dist/vtt.js'],
dest: 'build/temp/video.js',
},
dest: 'build/temp/video.js'
}
}
});

Expand Down
8 changes: 5 additions & 3 deletions src/js/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,11 @@ var fixEvent = function(event) {
}

// Handle which other element the event is related to
event.relatedTarget = event.fromElement === event.target ?
event.toElement :
event.fromElement;
if(!event.relatedTarget) {
event.relatedTarget = event.fromElement === event.target ?
event.toElement :
event.fromElement;
Copy link
Member

Choose a reason for hiding this comment

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

This is a pre-existing condition and I think I noticed this in a few other places when I was checking the classes PR, but multi-line ternary operators....ಠ_ಠ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did see the comments about preferring plain ol if/else over ternary. Alas it is quite the beauty .. I'll attempt a facelift..

Copy link
Member

Choose a reason for hiding this comment

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

Most of the events stuff was from Resig and still has his style, so all of that could be reformatted at some point, but don't feel like you need to in this PR. It's typically best to keep PRs and commits very specific.

Copy link
Member

Choose a reason for hiding this comment

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

kick the can

Nah I'm kidding, @heff's right. this is a pre-existing condition and you shouldn't feel like you need to clean it up here, particularly if it's going to muddy up the diff on this PR. We should have a pre-5.0 cleanup where we actually pick a style guide and make the necessary changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok left it as is; I'm happy with the minimum number pieces of flair.

}

// Stop the default browser action
event.preventDefault = function () {
Expand Down
21 changes: 21 additions & 0 deletions test/unit/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as Events from '../../src/js/events.js';
import document from 'global/document';
import TestHelpers from './test-helpers.js';

q.module('Events');

Expand Down Expand Up @@ -201,3 +202,23 @@ test('should have a defaultPrevented property on an event that was prevent from

Events.trigger(el, 'test');
});

test('should have relatedTarget correctly set on the event', function() {
expect(2);

var el1 = document.createElement('div'),
el2 = document.createElement('div'),
relatedEl = document.createElement('div');

Events.on(el1, 'click', function(e){
equal(e.relatedTarget, relatedEl, 'relatedTarget is set for all browsers when referring element is set on the event');
});

Events.trigger(el1, TestHelpers.makeMouseEvent('click', relatedEl));
Copy link
Member

Choose a reason for hiding this comment

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

If we're using Events.trigger I think we could just use a plain object here instead of creating a MouseEvent. We end up rebuilding it as an object either way. e.g. { type: 'click', relatedTarget: relatedEl }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely simpler; that's actually what I started with.. but then seems to defeat the purpose to test a property that's undefined in some browsers by setting it explicitly in trigger? ie testing it against a 'real' event. But if in the end it doesn't really matter, I'm happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like makeMouseEvent is still manually setting relatedTarget on the created events, so I might be missing a detail but it seems like essentially the same thing. I think to get a true real event we would have to use the element's focus() API or something like that to trigger a real event where the browser sets the related target. But that gets into integration test land, where I think a simple test to make sure we're not dropping relatedTarget in our code could be good enough.


Events.on(el2, 'click', function(e) {
equal(e.relatedTarget, null, 'relatedTarget is null when none is provided');
});

Events.trigger(el2, TestHelpers.makeMouseEvent('click'));
});
19 changes: 18 additions & 1 deletion test/unit/test-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@ var TestHelpers = {
return player = new Player(videoTag, playerOptions);
},

makeMouseEvent: function(type, relatedTarget){
var event;

if(MouseEvent) {
return new MouseEvent(type, {relatedTarget: relatedTarget || null});
}

event = document.createEvent('MouseEvents');

if(event.initMouseEvent) {
event.initMouseEvent(type, true, true, window, 0, 0, 0, 80, 20, false, false, false, false, 0, relatedTarget || null);
} else {
event.initEvent(type, true, false);
}
return event;
},

getComputedStyle: function(el, rule){
var val;

Expand All @@ -39,4 +56,4 @@ var TestHelpers = {
}
};

export default TestHelpers;
export default TestHelpers;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why it's diff'ing here; does my IDE need some kind of formatter on this line?