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.

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

Add support for audio/video attributes #2094

Closed
wants to merge 3 commits into from

Conversation

yocontra
Copy link
Contributor

  • adds currentTime property for controlling position in a video or audio element
  • adds volume property for controlling volume in a video or audio element
  • adds paused mutation method for controlling play/pause state in a video or audio element
  • adds playbackRate property for controlling playback speed in a video or audio element
  • adds srcObject property for setting a WebRTC stream on a video element
  • adds kind property/attribute for track element
  • adds srcLang property/attribute for track element

Related: #1128 #1474 #2090

srcSet: 'srcset'
},
DOMMutationMethods: {
playing: function(node, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to support an API like this, it should be paused to mirror the DOM API – but this is tricky because when the media reaches the end, it'll stop playing causing the element to get out of sync with the specified prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spicyj I agree - paused makes more sense than playing

@sophiebits
Copy link
Collaborator

Does react automatically lowercase attribute names?

It doesn't, but I believe HTML attributes (unlike SVG, for example) are always case-insensitive so the mapping isn't necessary.

@yocontra
Copy link
Contributor Author

Perhaps the paused state should be handled in a DOM wrapper to make sure the state stays in sync

@daniellewissc
Copy link

does this add support for media events (canplaythrough, durationchange) as synthetic events?

@yocontra
Copy link
Contributor Author

@daniellewissc No, this only adds support for attributes. When I have time I'll work on adding full event support as well

@syranide
Copy link
Contributor

currentTime is not a DOM markup attribute and should not be supported by React (see #2140), I believe the same applies to some of the others too including playbackRate.

@yocontra
Copy link
Contributor Author

@syranide There are other non DOM markup attributes that are supported. There seems to be a discrepancy between implementation and reality that should be ironed out either by adding all attributes or documenting why they shouldn't be supported.

@syranide
Copy link
Contributor

@contra Hmm? Which ones. The only ones I know of are scrollLeft and scrollTop and no one seems to think they should be there. Nor do I see why we should add more when the ones we have are broken and shouldn't be used.

@zpao
Copy link
Member

zpao commented Sep 19, 2014

scrollLeft and scrollTop are there but shouldn't be (artifact of times forgotten). If you see others, please let us know.

@zpao
Copy link
Member

zpao commented Sep 23, 2014

Let's get the non-markup attributes removed and then we can move forward.

},
DOMMutationMethods: {
paused: function(node, value) {
if (value === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe node[value ? 'pause' : 'play']() or value ? node.pause() : node.play()

Copy link
Contributor

Choose a reason for hiding this comment

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

As I assume this is a supported attribute (or else it should be removed), not using DOMMutationMethods at all and setting the attribute (as implemented by the standard) instead seems favorable.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs aren't great but as far as I'm aware the only way to set the play state is through the play and pause methods. There's no attributes for it. [1] [2] [3]

Copy link
Contributor

Choose a reason for hiding this comment

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

http://www.w3.org/TR/html-markup/video.html says it doesn't exist, so it shouldn't in React either.

@syranide
Copy link
Contributor

http://www.w3.org/TR/2012/WD-html5-20121025/media-elements.html#mediaevents

Are there new events we should support? (I'm not really familiar with the new media elements)

@sophiebits
Copy link
Collaborator

We can't take this as-is so I'm closing this out – if you do want to remove the non-attribute property names and rebase, I think we can take this though.

@sophiebits sophiebits closed this Feb 26, 2015
@yocontra
Copy link
Contributor Author

Seems like a step in the wrong direction IMO I hope this get rethought in the future

@syranide
Copy link
Contributor

@contra Ambigious behavior is best handled by community components, put audio/video in a component of your own and you're free to do what you want with the properties. Anything React could do that you can too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants