explainer for media controls #139

xxyzzzq · 2016-10-05T12:42:42Z

No description provided.

mounirlamouri · 2016-10-05T13:56:33Z

explainer.md

+
+    attribute EventHandler onplay;
+    attribute EventHandler onpause;
+    attribute EventHandler onplaypause;


Hmm, do we need to support this? Android will automatically send play or pause if playpause is fired. iOS supports this?

Actually the opposite. Both Android and iOS have "toggle play/pause". I think they are usually used for headset buttons.

Another issue related to play/pause is that how the user agent detects whether the page is playing or not. If the UA falsely detects a page is playing while the page thinks not, the page will always fire pause instead of play

Now MediaSession has a list of events it wants to handle.

Should we just leave onplaypause event then?
Would we fire both onplay/onpause and onplaypause at the same time? This might get confusing to the page.
I'd argue that the browser has its own playing/paused state anyway (to show the right button on the notification or in the media controls) so if the browser detects it wrong, we're in trouble even without the MediaSession API.
We can always add onplaypause later if it's really needed.

Did you mean only have onplaypause? Your first and last sentence seems contradicting. 😄
I agree with you, otherwise the page needs to specify the play/pause state.

What's the example where UA can't detect the playback state correctly? My argument that the UA likely depends on detecting the playback state correctly already (it shows play/pause button on the media element controls and in the notification) so either we can assume it's correct or we need an API for the page to tell the UA to adjust the UI bits I mention above.

Keeping all three seems confusing. I imagine the page do something like:

var v = document.getElementById('video1'); navigator.mediaSession.onplay = function() { v.play(); }; navigator.mediaSession.onpause = function() { v.pause(); }; navigator.mediaSession.onplaypause = function() { if (v.paused) v.play(); else v.pause(); };

Now if the UA fires both onplay and onplaypause, the page may play() and then pause() the element depending on the order and timing of the events.

If the UA fires onplay in some cases and onplaypause in some others, I wonder what the cases are and if they can be listed in the spec so that the page knows when it needs to add one or the other event listener.

If the UA only fires onplay and onpause, it seems pretty straightforward for the page what to do.

If the UA fires only onplaypause, the page might actually do the opposite of what the user expects: if the page maintains the state itself and it's not reflecting what the user sees in the browser UI. At the least, the page has to do some extra logic to check the state and it has no idea what the UA thinks about the playback state.

I think if the page wants to use MediaSession to control some paint-to-canvas animation (let's say with no sound), the UA can't reliably detect the playback state and we need some API for that (so we know when playback starts in the first place).

OK. Realized that onplaypause is identical to onplay/onpause + a playback state, while the difference is the if statement on the playback state is in the page or the UA.

Maybe we need to add attribute bool playbackState in the future. If it is specified, the UA uses this info, otherwise the UA uses its best guess.

Is this better?

Thanks! Done.

I think the need for a playbackState is much more immediate. Both YouTube and Netflix have user-displayed playback state that does not match the

So, I would either encourage adding a playbackstate attribute immediately, or adding the onplaypause command back.

mounirlamouri · 2016-10-05T13:56:55Z

explainer.md

+    attribute EventHandler onprevioustrack;
+    attribute EventHandler onnexttrack;
+    attribute EventHandler onseekforward;
+    attribute EventHandler onseedbackward;


s/onseed/onseek/

mounirlamouri · 2016-10-05T13:58:03Z

explainer.md

@@ -96,20 +113,43 @@ interface MediaMetadata {

 ### Media controls

-**Note** The spec will handle these use cases but not as part of the first
-iteration.
+**Note** The API for media controls are still under development and may not be


s/ are still/ is still/

mounirlamouri · 2016-10-05T13:59:14Z

explainer.md

+    readonly attribute supportsPreviousTrack;
+    readonly attribute supportsNextTrack;
+    readonly attribute supportsSeekForward;
+    readonly attribute supportsSeekBackward;


All of these are readonly, is it on purpose?

These attributes are removed per our offline discussion

mounirlamouri · 2016-10-05T14:00:11Z

explainer.md

+When pages registers event handlers to MediaSession, the page declares it can
+and want to accept these events. However, it is up to the user agent to decide
+which controls to show for in the UI and register callbacks to the
+platform. This may depend on UI concerns and platform capabilities.


s/This may depend/This may vary depending/

mounirlamouri · 2016-10-05T14:01:16Z

explainer.md

+
+```
+// Suppose |audio| is an audio element and |session| is a MediaSession object.
+if (!!session.supportsPlayPause) {


Hmm. Is supportsFoo meant for the UA to expose that it supports some action? I was expecting the opposite.

I was thinking if it would be useful if we expose the UA media controls capability to the page, but I haven't found a good use case yet. Maybe now we just remove all supports*?

I think registering event handlers should be enough for the page to show its intention to handle the event?

xxyzzzq · 2016-10-06T11:28:42Z

PTAL

mounirlamouri

A lot of comments are on the spec and API shape side of things (and nits). The gist of the change looks good :)

mounirlamouri · 2016-10-08T15:46:27Z

explainer.md

+  facilities, and forward the control event to the page by calling the
+  corresponding event listener callbacks.
+
+Pages declare what events it wants to handle by setting the `eventList`


I think we should use the term action instead of events here.

mounirlamouri · 2016-10-08T15:46:50Z

explainer.md

+    attribute EventHandler onseekbackward;
+    ...
+
+    attribute DOMString eventList = "";


Maybe this should be called supportedControls?

Actually, what about Promise<> setSupportedControls(sequence<DOMString>)

You can also name this "setSupportedActions" if the naming sounds better. I'm not certain what to pick :)

I'm using supportedActions since we are using the term action consistently.

Maybe just throw an exception if the argument is illegal? I'm not sure what's the benefit of returning a Promise<>, which makes the API asynchronous.

Also, I think we made metadata as an attribute of MediaSession for the race condition of a page setting metadata in multiple places. Maybe supportedActions also has the problem?

I disagree with mounirlamouri here. There's no need to have a separate call to setSupportedControls(). Adding event handlers implies that those events are supported.

Secondly, throwing an exception if one of the "controls" is unsupported is not future-proof. If the spec is changed to add (or remove) specific commands, page authors will be forced to try-and-retry various combinations of commands until they get one that doesn't throw an exception. The end result would be that pages would only ever support the intersection of all supported commands across both evergreen and legacy UAs.

The feature detection story of using the event handlers to show support for various commands is much better:

if (typeof(audio.onseekbackward) !== 'undefined') audio.onseekbackward = mySeekHandler; if (typeof(audio.onnewlyspecifiedcommand) !== 'undefined') audio.onnewlyspecifiedcommand = myNewCommandHandler;

Hmm, I did not think about throwing and forward compatibility. Good point.

For the events, this is something we did consider. To be explicit, something that might not be clear is that we want websites to express interest for a control/action that would allow the browser to show a UI for it. In addition of that, we want to offer a way for the websites to control this. For example, a website might want the UA to show seek backward/forward buttons but wouldn't need to do anything with the events. If we consider events to be the way for websites to express support/interest, it would require them to attach events doing nothing.

Maybe this isn't a strong use case and we shouldn't design the API around it?

Maybe we could go back to have a DOMString/sequence attribute and let it be totally a hint? Pages can set the value to whatever it want, and the UA will ignore unexpected values.

As you pointed, I'm also starting to wonder if this will be useful.

mounirlamouri: If you do want to support the use case of the page declaring support for a command but not wanting to override the default behavior for a command, the specification could say that the event handler must call preventDefault() to prevent the UA's default behavior for a given event (e.g., calling play() due to onplay(), etc.).

👍 That was my alternative design. Let's run with this for now and see if everyone is happy.

mounirlamouri · 2016-10-08T15:55:15Z

explainer.md

+event handler, the user agent may have fallback behaviors (such as handling
+"seekforward" internally).
+
+Please be aware that, it is up to the user agent to decide which controls to


nit: remove "Please be aware that". The audience for this document is not web developers.

mounirlamouri · 2016-10-08T15:57:03Z

explainer.md

+    attribute EventHandler onseekbackward;
+    ...
+
+    attribute DOMString eventList = "";
 };


We need to have a list of supported actions/controls. It can be expressed via an enum. The spec will have to require the supported controls/actions passed to be from that list, otherwise throw an exception.

Done.

enum MediaSessionActions { "play", "pause", ... };

mounirlamouri · 2016-10-08T15:57:36Z

explainer.md

+The API for media controls should work as follows:
+
+* The page tells which media controls it would like to receive, and register
+  corresponding callbacks to `MediaSession`.


"Which media controls it supports and would like the user agent to expose in its UI."

I guess it's already in the text bullet?

mounirlamouri · 2016-10-08T15:59:43Z

explainer.md

+the event name in `eventList`, and register the corresponding event handler. In
+case of the page specifying event name in `eventList` while not registering an
+event handler, the user agent may have fallback behaviors (such as handling
+"seekforward" internally).


Maybe you should mention somewhere that an action should be marked as supported in order to receive an event because the UA might not expose the control otherwise. Also add that it's okay to mark an action as supported but not handle it, in which case the UA will do the default behaviour.

Done. Rephrased the paragraphs around this location.

mounirlamouri · 2016-10-08T16:00:28Z

explainer.md

+session.eventList = "playpause";
+session.onplaypause = function() {
+    if (audio.paused)
+        play();


nit: audio.play();

mounirlamouri · 2016-10-08T16:00:42Z

explainer.md

+    if (audio.paused)
+        play();
+    else
+        pause();


nit: audio.pause();

xxyzzzq · 2016-10-13T12:24:30Z

PTAL. Now using the preventDefault() solution. I'm not quite convinced of this solution though. Left some notes for a possible issue in the explainer.

mounirlamouri

lgtm with comments

mounirlamouri · 2016-10-14T17:00:02Z

explainer.md

+
+The user agent may have fallback behavior for some actions. There might be
+useful in several use cases where the page wants to use or override the default
+fallback behavior. Here are some possible solutions:


I would not have the two bullet points and just say that preventDefault() should be called to avoid the fallback behaviour.

mounirlamouri · 2016-10-14T17:02:42Z

explainer.md

+attribute `playbackState` to let the page tell the playback state correctly. In
+many cases, `play` and `pause` share the same media button. When the button is
+pressed, the user agent should fire `pause` action when the page is playing and
+fire `play` when the page is paused.


I would prefer to put back playpause and open an issue to discuss whether we should drop it.

mounirlamouri · 2016-10-14T17:31:05Z

@jernoble playpause event was added back. @xxyzzzq opened issue #141 to see if we should do something else. I prefer to expose playpause than playbackState. The former is more flexible than the later. Let's move the discussion to #141 :)

mounirlamouri reviewed Oct 5, 2016

View reviewed changes

xxyzzzq force-pushed the media_controls_explainer branch from ed6bf77 to d937245 Compare October 6, 2016 11:27

mounirlamouri reviewed Oct 8, 2016

View reviewed changes

xxyzzzq force-pushed the media_controls_explainer branch 2 times, most recently from 4e7b227 to 91668ed Compare October 13, 2016 12:20

xxyzzzq force-pushed the media_controls_explainer branch from 91668ed to e729152 Compare October 14, 2016 09:46

mounirlamouri reviewed Oct 14, 2016

View reviewed changes

explainer for media controls

8209617

xxyzzzq force-pushed the media_controls_explainer branch from e729152 to 8209617 Compare October 14, 2016 17:21

xxyzzzq merged commit 1d6eeb7 into w3c:master Oct 14, 2016

xxyzzzq deleted the media_controls_explainer branch October 25, 2016 19:14

xxyzzzq mentioned this pull request Nov 17, 2016

Whether to allow media control actions be default-handled by the UA, and how? #145

Closed

xxyzzzq mentioned this pull request Jan 12, 2017

Define in greater detail what effect setting playbackState has #177

Closed

explainer for media controls #139

explainer for media controls #139

Conversation

xxyzzzq commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxyzzzq Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

xxyzzzq commented Oct 6, 2016

mounirlamouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxyzzzq commented Oct 13, 2016

mounirlamouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mounirlamouri commented Oct 14, 2016

xxyzzzq Oct 5, 2016 •

edited

Loading