Add Video/Audio Media Events #4370
Conversation
|
I don't see any reason not to add them, might as well include all the events. cc @zpao |
| topDragOver: eventTypes.dragOver, | ||
| topDragStart: eventTypes.dragStart, | ||
| topDrop: eventTypes.drop, | ||
| topPause: eventTypes.pause, |
There was a problem hiding this comment.
Please keep all lists sorted.
|
@jimfb I added the rest of them and updated the title. |
| @@ -322,6 +322,70 @@ function trapBubbledEventsLocal() { | |||
| ), | |||
| ]; | |||
| break; | |||
| case 'video': | |||
| inst._wrapperState.listeners = [ | |||
There was a problem hiding this comment.
I'm not entirely sure about this, but this is superfluent I believe. This is only for events that explicitly can emit events the instant the node is mounted into the DOM (and otherwise risk getting lost). Perhaps it applies to some of these onLoadStart (etc) perhaps, but I'm sure it does not apply to all of them.
There was a problem hiding this comment.
My understanding was that this function was for events that can't be caught at the global level and need to be attached to the actual dom node. For example, media events don't bubble and can only be received on the media node.
There was a problem hiding this comment.
Oooh, yeah that too. All good then :)
|
One thing that I didn't do is make an extension to the SyntheticEvent for media stuff. Looking into the specs it doesn't seem that the MediaEvents do anything particularly special. I'm assuming the answer to this will be to follow the spec, but we could add some useful helper information to a SyntheticMediaEvent. Things like: Float percentLoaded
Bool muted
Float volumeLevel
Object metadata
...cc @zpao |
| topEnded: null, | ||
| topSeeking: null, | ||
| topSeeked: null, | ||
| topVolumeChange: null, |
There was a problem hiding this comment.
Shouold probably keep this ABC order too
|
Sorry for the delay! A few things... What about As for Synthetic events, yea, we should stick to spec. |
|
@zpao no worries! I did some research, the events do overlap 100%, so i included audio in the event handling. I think this PR should be good to go. I didn't see any obvious/good place to put tests in. Maybe I missed that? Or maybe this is something you guys just don't worry about testing? |
| @@ -322,6 +322,49 @@ function trapBubbledEventsLocal() { | |||
| ), | |||
| ]; | |||
| break; | |||
| case 'video': | |||
| case 'audio': | |||
| var mediaEvents = { | |||
There was a problem hiding this comment.
This shouldn't be inline (to avoid unnecessary overhead).
There was a problem hiding this comment.
You mean I should just copy the code for audio and video? What's the benefit in that?
There was a problem hiding this comment.
Ah, no I meant to put it outside the function since it's a static object, so it isn't recreated every time it's called.
There was a problem hiding this comment.
gotcha. Donezies
|
Last call for feedback on this PR. Otherwise, looks good to me. @blainekasten There are 12 commits in this PR; can you squash them together so we can merge? |
blainekasten
force-pushed
the
video-events
branch
2 times, most recently
from
9bf3c84 to
bdf377f
Compare
|
That was surprisingly difficult. But it's done. @jimfb Thanks! |
|
Great, thanks @blainekasten! |
|
Woo hoo! @jimfb, do I assume that this will be included in 0.14 then? |
|
@blainekasten Correct, that's the plan! |
@see https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Media_events
This adds the following event options to video and audio dom components:
onPause onLoadStart onPlay onPlaying onCanPlay onCanPlayThrough onAbort onEnded onSeeking onSeeked onVolumeChange onError onDurationChange onEmptied onLoadedData onLoadedMetadata onEncrypted onProgress onRateChange onStalled onSuspend onTimeUpdate onWaiting