Checking on the event the relatedTarget exists. fixes #2024 #2025
Conversation
mischizzle
commented
Apr 9, 2015
mischizzle
commented
- Checking the relatedTarget exists on the event,
- .idea to .gitignore,
- added tests for the fix and a corresponding test helper to create mouseevent.
| 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)); |
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the help! Made a few comments but otherwise this looks good to me. |
| if(!event.relatedTarget) { | ||
| event.relatedTarget = event.fromElement === event.target ? | ||
| event.toElement : | ||
| event.fromElement; |
There was a problem hiding this comment.
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....ಠ_ಠ
There was a problem hiding this comment.
I did see the comments about preferring plain ol if/else over ternary. Alas it is quite the beauty .. I'll attempt a facelift..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok left it as is; I'm happy with the minimum number pieces of flair.
…n Events.trigger. fixes videojs#2024
| export default TestHelpers; |
There was a problem hiding this comment.
Not sure why it's diff'ing here; does my IDE need some kind of formatter on this line?
|
Got this merged in (finally). Thanks again! |
|
Hey no problem! Kind of forgot about it to be honest .. no conflicts from being stale, that i can see |
