prevent default action for simple html5 media events. fixes #573, fixes #620 (duplicate bug) #630
Conversation
| // Mediafaker doesn't support play/pause, so dispatch an event manually. | ||
| var event = document.createEvent('CustomEvent'); | ||
| event.initCustomEvent('play', false /*bubbles*/, true /*cancelable*/, null); | ||
| tech.el_.dispatchEvent(event); | ||
| }); |
There was a problem hiding this comment.
Thanks for writing a test!
There's a PlayerTest.makePlayer() function that could make this simpler. https://github.com/videojs/video.js/blob/master/test/unit/test-helpers.js#L8
createEvent is apparently deprecated.
https://developer.mozilla.org/en-US/docs/Web/API/document.createEvent
Also not supported in ie8, but I guess that's not really an issue in the HTML5 tests. Probably better to use a constructor like MDN says.
It seems like at the core of this you're just testing that preventDefault works, does that sound right?
There was a problem hiding this comment.
Okay - I've incorporated PlayerTest.makePlayer().
I should have mentioned this before submitting, but I did try to use event constructors and ran into an issue in phantomjs - ariya/phantomjs#11289. Okay to keep createElement for now?
And yeah - the point is just a regression test to verify that events bubbled from the html5 tech have preventDefault called on them before trigger is called.
|
Thinking about it more, I agree with your thought that it should support event.bubbles, and that might be the thing to focus on. I think we could handle this updating this line: https://github.com/videojs/video.js/blob/v4.1.0/src/js/events.js#L281 To: if (parent && !event.isPropagationStopped() && event.bubbles !== false) { |
|
Sure - I can make that change to events.js. My only concern was that my observations were biased towards chrome, and wasn't sure if all browsers turned off bubbling for media events, since I'm unable to find that rule in the spec. Though there seems to be pretty consistent language from other browsers: Would you rather me scrap this pull request and make that change to events.js? |
|
Yeah, you can use this PR or a new one, but the bubble part should be enough. AFAIK, certain events bubble so you can disambiguate where they happened, e.g. clicks. It's pretty clear where media events happen, so I at least don't see a benefit in bubbling them. Steve Heffernan (mobile) On Jul 23, 2013, at 4:05 PM, cameron notifications@github.com wrote:
|
|
All done with the suggested changes. |
|
Thanks! And very nice test. |
Close GH-630: prevent default action for simple html5 media events. fixe...
|
FYI, didn't realize this was against the stable branch. Most new changes should go into master. I merged into master and reverted stable, so if you're currently working from stable you won't see your changes until the next release. Thanks again. |
|
Gotcha. I tried to use contribflow to see how that process worked, so it might have automatically branched from stable? Or I might not have understood what I was doing -- but thanks for the heads up either way. |
|
Oh cool. Yeah, we're still polishing up contribflow, but hopefully it was a good experience. Let me know if you have any feedback. We also had some updates to the CONTRIBUTING.md in the last month which talk about contribflow if that helps at all. |
Looking for advice/input on whether this is a problem for flash, too.