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 a way of detecting which actions a UA supports #228

Open
beccahughes opened this issue Aug 19, 2019 · 58 comments
Open

Add a way of detecting which actions a UA supports #228

beccahughes opened this issue Aug 19, 2019 · 58 comments

Comments

@beccahughes
Copy link
Contributor

From a discussion with @beaufortfrancois and @mounirlamouri. We should have a way of detecting what action a UA supports e.g.

if (navigator.mediaSession.supports('pause'))
  navigator.mediaSession.setActionHandler('pause', _ => { /* ... */ });

or

if ('pause' in MediaSessionAction)
  navigator.mediaSession.setActionHandler('pause', _ => { /* ... */ });
if ('stop' in MediaSessionAction)
  navigator.mediaSession.setActionHandler('stop', _ => { /* ... */ });
// TODO: Set other action handlers...
@jernoble
Copy link

I think the discovery is flowing the wrong direction. The UA should be discovering what the page supports, and calling the appropriate handler, and not vice versa.

@beaufortfrancois
Copy link
Contributor

My point is that we need a proper way of detecting which actions a UA supports to avoid code below.

if (!('mediaSession' in navigator)) {
  // Basic media session is supported but some actions might not be supported.
  // You'll have to try/catch some of them specifically for Chrome. Sorry ;(

  // Pause action is ok... I think...
  navigator.mediaSession.setActionHandler('pause', function() { /* ... */ });

  // I'm quite sure this one may fail before Chrome 77 so let's use try/catch.
  try {
    navigator.mediaSession.setActionHandler('stop', function() { /* ... */ });
  } catch(error) {
    // Sorry ;(
  }

  // TODO: Set other action handlers.
  ...
}

@jernoble
Copy link

I don’t understand; where in the spec does it specify that setActionHandler() should throw when it receives an unsupported action? Is it just due to that it takes an enum and that WebIDL requires a throw if the parameter isn’t in the enum? If so, isn’t there a defined way to do this already? Like:

if (‘stop’ in MediaSectionAction) {
    navigator.mediaSession.setActionHandler('stop', function() { /* ... */ });
}

@beaufortfrancois
Copy link
Contributor

Indeed. navigator.mediaSession.setActionHandler('foo', _ => {}) throws a "TypeError" due to WebIDL bindings.

Failed to execute 'setActionHandler' on 'MediaSession': The provided value 'foo' is not a valid enum value of type MediaSessionAction.

And MediaSectionAction enum is not exposed to web developers as well sadly. See https://github.com/web-platform-tests/wpt/blob/master/mediasession/idlharness.window.js

@jernoble
Copy link

How do all other specs that take enums as arguments deal with this? It seems unnecessary and duplicative just to have to add a supports() method in every API that uses an enum. (Also, we couldn’t spec supports(MediaSessionAction action) as that would throw on supports('pause') too!)

@jernoble
Copy link

jernoble commented Aug 21, 2019

If we can’t find a way to make action in MediaSessionAction work, one workaround would be to add a method with no side-effects which takes a MediaSessionAction, like

partial interface MediaSession {
    bool hasActionHandler(MediaSessionAction);
}

Which could be used in a simple function:

bool supportsAction(action){
    try { navigator.mediaSession.hasAction(action); }
    catch(e) { return false; }
    return true;
}

Edit: people have told me this example is confusing. See my comment #228 (comment) for clarification.

@chrisn
Copy link
Member

chrisn commented Aug 21, 2019

We might need to think about the terminology. The spec currently defines a "supported media session action" as an action with an associated handler function, rather than an action that a UA doesn't support.

So would mediaSession.supports(action) be a different query to mediaSession.hasActionHandler(action)? And which of these would action in MediaSessionAction potentially correspond to?

@jernoble
Copy link

Yes that’s a great question. Is the end goal that the “stop” action is “supported” on all instances of (e.g.) Safari >= 15.0, or is the end goal that the ”stop” action is “supported” when the user has a keyboard with a “Stop” key.

Personally, I’m much more supportive of the former than the latter.

But the terminology we come up with will depend on the answer to that question. If we collectively agree on the latter answer, then supports() is a natural fit. However if we decide the former answer is correct, then supports() suffers from a lexical ambiguity that could lead clients to assume we meant the latter.

@jernoble
Copy link

FWIW, this is still being argued about in the WebIDL spec: whatwg/webidl#107

@jernoble
Copy link

jernoble commented Aug 21, 2019

One “solution” suggested by @tabatkins in the OP of that issue, when applied in this context, would look like:

”stop” in navigator.mediaSession.MediaSessionAction

This would be an alternative to just exposing the enum directly in global scope.

Edit: “solution” in quotes only because it’s not a WebIDL solution, not that it wouldn’t solve the issue.

@mounirlamouri
Copy link
Member

Is the end goal that the “stop” action is “supported” on all instances of (e.g.) Safari >= 15.0, or is the end goal that the ”stop” action is “supported” when the user has a keyboard with a “Stop” key.

In my opinion, we should do the former, not the latter. The issue with the latter is that in some cases, we wouldn't know: for example, a keyboard may or may not have a next track button. In some rare cases, we know that an action will never be executed because of some specificity with an OS version but I don't think that's worth putting in spec.

I think originally the issue raised because @beaufortfrancois was thinking of how to write backward compatible code that uses the new stop action. Is that correct Francois?

@jernoble
Copy link

Someone pointed out off-list that my bool hasActionHandler(MediaSessionAction) example is confusing. In my straw man example, if we added a hasActionHandler() function, it would return true if someone had previously called addActionHandler() with a valid action and handler, return false if no handlers had been added for that action, or if all handlers had been removed, or would throw an exception if the action was not in the MediaSessionAction enum. But since there are no side-effects to calling hasActionHandler(), it could be used to detect whether a given action string was a valid MediaSessionAction.

That said, maybe adding a readonly attribute sequence<MediaSessionAction> MediaSessionAction to MediaSession is a much more direct solution to the problem at hand.

@chrisn
Copy link
Member

chrisn commented Aug 21, 2019

I think originally the issue raised because @beaufortfrancois was thinking of how to write backward compatible code that uses the new stop action.

I certainly have this use case, experimenting with the Chrome for Android implementation which only partially implements the spec so far (e.g., adding a seekto handler currently throws in the Chrome version I'm using). I guess it would also be useful as a mechanism for future extensibility.

@beaufortfrancois
Copy link
Contributor

Following up on https://groups.google.com/a/chromium.org/g/blink-dev/c/sXhlwO_R9To/m/c1ZG4qx8AgAJ discussion to figure out a safer way to enable feature detection for actions, I'd like to propose adding a new actions readonly attribute to the MediaSession interface that would contain an array of MediaSessionAction supported by the UA.

partial interface MediaSession {
  readonly attribute FrozenArray<MediaSessionAction> actions;
};
console.log(navigator.mediaSession.actions);
// ["play", "pause", "previoustrack", "nexttrack", "seekbackward", "seekforward",
// "skipad", "stop", "seekto", "togglemicrophone", "togglecamera", "hangup"]
if (navigator.mediaSession.actions.includes('hangup')) {
  navigator.mediaSession.setActionHandler('hangup', () => { ... }
}

Note that a sequence of enums is not supported in WebIDL, hence the use of a FrozenArray.

@jernoble @steimelchrome @chrisn WDYT?

@chrisn
Copy link
Member

chrisn commented Jul 8, 2021

This seems reasonable to me, although I'd want to know if there's any change planned in WebIDL or other pattern specs should use for this kind of feature.

We have a WG meeting coming up next week, please add the agenda label if you'd like time in the meeting to discuss.

@beaufortfrancois
Copy link
Contributor

WebIDL changes won't be needed.
https://w3ctag.github.io/design-principles/#feature-detect is probably the closed we have from a spec design principle point of view.

I'll be OOO next week but I'd love if it could be discussed at the WG meeting.

@beaufortfrancois
Copy link
Contributor

@chrisn Please add the agenda label, I don't have access.

@tidoust tidoust added the agenda Topic should be discussed in a group call label Jul 8, 2021
@tidoust
Copy link
Member

tidoust commented Jul 8, 2021

@chrisn Please add the agenda label, I don't have access.

Just a side note that you should now be able to manage issue labels on this repository.

@domenic
Copy link

domenic commented Jul 8, 2021

I'm not sure I quite understand what problem this is solving. Is the main goal to replace try/catch with if?

@domenic
Copy link

domenic commented Jul 8, 2021

To expand a bit more:

  • If the goal is that developers need to write custom code to handle an action being unsupported, either try or if seem like reasonable tests for that. And try has the advantage of not adding new API.

  • If the goal is that developers do no need to write custom code to handle an action being unsupported, then a simpler solution would be to just not use Web IDL enums here, and use a string instead so that you avoid the throwing behavior. That way adding a handler for hangup on browsers that don't implement hangup would silently do nothing, similar to adding a handler for play on a desktop with no play key.

@chrisn
Copy link
Member

chrisn commented Jul 13, 2021

Discussion from April Media WG meeting: https://www.w3.org/2021/04/13-mediawg-minutes.html#t04. The context was adding new action handlers (#264).

@jan-ivar
Copy link
Member

jan-ivar commented Jul 13, 2021

I agree with @domenic's option 1 here. This is why enums throw.

My point is that we need a proper way of detecting which actions a UA supports to avoid code below.

I think we have that. If a site doesn't care whether actions are supported or not, they can use the following pattern:

function setActionHandlerIfSupported(action, func) {
  try {
    navigator.mediaSession.setActionHandler(action, func);
  } catch (e) {
    if (e.name != "TypeError") throw e;
  }
}

// Basic media session is supported but some actions might not be supported.
setActionHandlerIfSupported('pause', () => { /* ... */ });
setActionHandlerIfSupported('stop', () => { /* ... */ });

FWIW, this is still being argued about in the WebIDL spec: whatwg/webidl#107

I believe that issue is instead about cases where no other means of feature detection exists (dictionary members). That's not the case here. We have an API that throws, i.e. we already have feature detection.

@jan-ivar
Copy link
Member

jan-ivar commented Jul 13, 2021

I think it's rare to need pure feature detection, so while I think it needs to be possible, I don't think it needs to be ergonomic.

one workaround would be to add a method with no side-effects

While contemplating how to write that using setActionHandler (by removing its potential side-effects), the lack of a getActionHandler function became apparent (a way to set, but not get?) — If we had that, then it would also be that method:

function supportsAction(action) {
  try {
    navigator.mediaSession.getActionHandler(action);
    return true;
  } catch (e) {
    if (e.name != "TypeError") throw e;
    return false;
  }
}

@jan-ivar
Copy link
Member

It's also worth noting that navigator.mediaSession.setActionHandler(action, null) has no side-effects initially. This becomes a problem the same time the fact that setActionHandler is last-come-last-serve becomes one.

@yoavweiss
Copy link

My concern with using an enum and throwing as a feature detection method, is that developers MUST use try and catch for any new actions we add. Otherwise, they are running a risk of completely breaking their app in non-supporting browsers, unless they explicitly test in those browsers.

A supports() API coupled with silent failure would alleviate that concern.

@jan-ivar
Copy link
Member

That's an ergonomics argument, and those have been rejected in the past for all feature detection.

@beaufortfrancois
Copy link
Contributor

@jan-ivar Can you share links to similar issues where this was rejected?

@yoavweiss
Copy link

That's an ergonomics argument, and those have been rejected in the past for all feature detection.

The argument is not that it's hard for devs to perform such feature detection, but that it's a potential foot gun that can cause interop issues when introducing new values.

@beaufortfrancois
Copy link
Contributor

But if we prefer silent failure, trivial feature detection, and last-come-only-served behavior, did anyone consider:

navigator.mediaSession.onpause = () => { ... };
navigator.mediaSession.onstop = () => { ... };
navigator.mediaSession.onseekbackward = () => { ... };
navigator.mediaSession.onseekforward = () => { ... };
navigator.mediaSession.onprevioustrack = () => { ... };
navigator.mediaSession.onnexttrack = () => { ... };
navigator.mediaSession.onskipad = () => { ... };
navigator.mediaSession.onstop = () => { ... };
navigator.mediaSession.onseekto = () => { ... };
navigator.mediaSession.ontogglemicrophone = () => { ... };
navigator.mediaSession.ontogglecamera = () => { ... };
navigator.mediaSession.onhangup = () => { ... };

? This seems like the most idiomatic semantics for these 3 behaviors. And it won't throw on Chrome 77.

See #148 to understand why we didn't go with this pattern.

@beaufortfrancois
Copy link
Contributor

Having navigator.mediasession.actions to return a list of actions supported by the browser in this platform would also help websites to know whether an action is supported.

For instance, even though skipad is a valid enum, navigator.mediaSession.setActionHandler('skipad', _ => { /* ... */ }); doesn't throw but doesn't do anything on Safari.

@jan-ivar
Copy link
Member

See #148 to understand why we didn't go with this pattern.

I don't mean EventHandlers. Just attributes. E.g.

callback ActionHandler = any ();

[Exposed=Window]
interface MediaSession {
  attribute ActionHandler? onpause;
  attribute ActionHandler? onstop;
  ...
}

would also help websites to know whether an action is supported.

if ('onpause' in navigator.mediaSession) { /* supported */ }

@chrisn
Copy link
Member

chrisn commented Aug 10, 2021

Notes from a brief discussion at the 10 Aug 2021 Media WG meeting are here.

@jernoble
Copy link

Having navigator.mediasession.actions to return a list of actions supported by the browser in this platform would also help websites to know whether an action is supported.

For instance, even though skipad is a valid enum, navigator.mediaSession.setActionHandler('skipad', _ => { /* ... */ }); doesn't throw but doesn't do anything on Safari.

This is the kind of feature creep I would like to avoid. I most definitely don't want to add a .actions API that queries for whether the UA will ever call that action. Hypothetically speaking, "skipad" might never be called for a user with a USB keyboard, but might be called for a user with a Touch Bar. I wouldn't want the .actions to become a stalking horse for detecting what kind of keyboard a user is using.

The current use case for this feature is to avoid throwing an exception when setting a handler, not for detecting the circumstances under which a UA will call the handler.

@beaufortfrancois
Copy link
Contributor

The current use case for this feature is to avoid throwing an exception when setting a handler, not for detecting the circumstances under which a UA will call the handler.

Let's focus on that then.

Does #228 (comment) proposal looks good to you? I'm happy to send a spec PR.

@domenic
Copy link

domenic commented Aug 11, 2021

I still don't understand why you'd add anything here. Did anyone have a rebuttal to my #228 (comment) ?

@yoavweiss
Copy link

I thought #228 (comment) covered it:

  • Using an enum that throws breaks less-tested browsers in case developers don't actually use try/catch when using the API with new additions they don't yet support.
  • An explicit feature detection mechanism coupled with silent failures for non-supporting browsers doesn't suffer from the same issue, and is therefore more resilient to future additions to the API.

@domenic
Copy link

domenic commented Aug 16, 2021

No, sorry, it did not cover that.

Using an enum that throws breaks less-tested browsers in case developers don't actually use try/catch when using the API with new additions they don't yet support.

The same is true for a new feature detection API, when developers don't use if. There's an explicit isomorphism here between if and try which you can use to show that the same argument applies to both.

An explicit feature detection mechanism coupled with silent failures for non-supporting browsers doesn't suffer from the same issue, and is therefore more resilient to future additions to the API.

Yes, but nobody (except you maybe?) is proposing a silent failure.

To be clear, my objection is specifically to a new API as long as failures continue being loud. If failures are silent, then a new feature detection API makes some sense (although IMO it's not clear what the use case would be). But there's no WG agreement for silent failure, and every time that's been proposed so far it's been rejected, from what I can see.

@chrisn
Copy link
Member

chrisn commented Aug 20, 2021

I thought we were converging on silent failure, i.e., non-throwing setActionHandler(), based on @jan-ivar's #228 (comment):

With that understanding, I agree silent failure seems appropriate.

We now have two proposals: from @beaufortfrancois in #228 (comment):

  • add actions which exposes the list of supported actions, where "supported" means implemented on all instances a particular browser version and does not expose information about device input capabilities

  • change setActionHandler() to take action as a string so no longer throws

and from @jan-ivar in #228 (comment) (which would be a bigger change from what has already shipped).

@domenic
Copy link

domenic commented Aug 20, 2021

I apologize for missing that part of #228 (comment)!

@chrisn
Copy link
Member

chrisn commented Aug 27, 2021

Thanks @domenic. What are your thoughts, @jernoble and @jan-ivar?

@jan-ivar
Copy link
Member

jan-ivar commented Sep 9, 2021

and from @jan-ivar in #228 (comment) (which would be a bigger change from what has already shipped).

Define bigger. Let me modify my (non-event-handler) proposal slightly to avoid it looking like event-handlers (which shouldn't have side-effects), so users don't see onplay and think addEventListener also works:

- navigator.mediaSession.onpause = () => { ... };
+ navigator.mediaSession.actions.pause = () => { ... };

Now the two proposal seem to be:

  1. if (!("pause" in navigator.mediaSession.actions)) console.log("not supported");
    
    navigator.mediaSession.actions.pause = () => { ... }); // silent
  2. if (navigator.mediaSession.actions?.pause != "supported") console.log("not supported");
    
    if (navigator.mediaSession.setActionHandler) {
      navigator.mediaSession.setActionHandler(pause, () => { ... }); // silent
    }

Pros of 1: gives us getters for the previous action handlers set, for free.
Cons of 1: Not entirely free: feature detection only works ahead of setting an action (which is when you should feature detect).
Cons of 2: A lot of surface dedicated to feature detection we no longer need to use the API, yet no getter?

Web compat: both proposals will fail to detect support in Chrome today.

I vote for 1 since feature detection is no longer really necessary to use the API.

@jernoble
Copy link

jernoble commented Sep 9, 2021

@beaufortfrancois said:

...I'd like to propose adding a new actions readonly attribute to the MediaSession interface that would contain an array of MediaSessionAction supported by the UA.

Making that attribute readonly would prevent polyfills from adding support for new actions in existing UAs. Not a blocker as such, but this would be a reason to prefer @jan-yvar's "Option 1" vs. "Option 2".

@beaufortfrancois
Copy link
Contributor

beaufortfrancois commented Sep 13, 2021

@jan-ivar I believe your proposal "2" does not reflect the one at #228 (comment). It should look like this:

  1. if (navigator.mediaSession.actions.includes('pause')) {
      navigator.mediaSession.setActionHandler('pause', () => { ... }
    }

Since Media Session shipped in Chrome, Firefox, and Safari already, I believe changing drastically the shape of action handlers for the sake of bringing better feature detection is not worth it. That's me wearing my web developer hat here ;)

@jernoble We can remove readonly if that helps. I don't feel strongly about it. Your point is valid.

@jan-ivar
Copy link
Member

jan-ivar commented Sep 14, 2021

@jan-ivar I believe your proposal "2" does not reflect the one at #228 (comment). It should look like this:

Thanks and sorry for getting that wrong. But that would throw on older browsers, so you'd need this I think:

if (navigator.mediaSession.actions?.includes('pause')) {
  navigator.mediaSession.setActionHandler('pause', () => { ... });
}

Since Media Session shipped in Chrome, Firefox, and Safari already, I believe changing drastically the shape of action handlers for the sake of bringing better feature detection is not worth it.

It's not just for the sake of feature detection, since we gain a getter. Option 1 is also more idiomatic to JS:

  1. It's an attribute instead of a silent setter method (which I believe DOM folks would say should be an attribute)
  2. It makes it obvious you can only have one action handler per action, which isn't clear with setActionHandler.
  3. It makes it obvious that setting subsequent handlers on an action silently overrides earlier handlers of that action

On the latter: setActionHandler lets me set an action handler, but I cannot see whether one has already been set, and the act of me setting an action handler overrides what was set before, which means I cannot see what I'm overriding, or if I'm overriding anything. That seems like a usability issue.

Also, changing the behavior without changing the shape isn't web compatible either. Old browsers will throw on sites calling setActionHandler with an unknown action. So sites need to update their code anyway for web compat. Getting sites to update might be simpler if the API is changed (for the better). The new API is silent even on old browsers (because "pause" in undefined is falsy).

@yoavweiss
Copy link

IIUC, Option 1 would not be backwards compatible. Is that correct?

If so, we should be cautious about chaging the API shape to it, as it's likely we'd need to keep the current shape as well for quite some time. Deprecating the old API shape and moving to the new one would require strong justification, at least in order to ship the new API shape in Chromium. Is there developer demand for a getter here?

@chrisn
Copy link
Member

chrisn commented Nov 3, 2021

Yes, that seems correct to me - with Option 1 there would be a new API shape and so there would have to be a migration plan from the old API.

But, as Jan-Ivar points out, changing setActionHandler to be non-throwing for actions that are not supported also isn't backwards compatible.

So, how to progress this issue? Do we need additional developer feedback? Should we run a call for consensus with these two options? There's a WG meeting coming up next week, should we use that for further discussion?

@youennf
Copy link
Contributor

youennf commented Mar 6, 2023

Do we need to revive the discussion here?
It seems this should be resolved sooner rather than later, marking as P1.

@youennf youennf added the P1 label Mar 6, 2023
@youennf
Copy link
Contributor

youennf commented Mar 14, 2023

Demoting to P2 and tentatively moving to V2 unless we hear other feedback.

@marcoscaceres
Copy link
Member

marcoscaceres commented May 30, 2024

How pressing is this? Is this needed by developers to add UI elements to a page by feature detection? If so, we should prioritize this again as it potentially affects the user experiences that can be created on a website.

At the same time, setActionHandler() already throws, so there is a means to check for what's supported.

@marcoscaceres
Copy link
Member

marcoscaceres commented May 30, 2024

Here's an example of someone doing the try catch in the wild and using the actions.includes type pattern:
https://github.com/blackcandy-org/blackcandy/blob/fc2d680b8e652a31a797294c0a81e63953967af8/app/javascript/controllers/media_session_controller.js#L16

(this is in no way representative of anything... it's just an example)

We found some other examples through GitHub search.

@youennf
Copy link
Contributor

youennf commented May 30, 2024

I do not think this API is required from web developers, the try/catch approach is good enough.
It would be nice to transition to an API with explicit feature detection and a not throwing setActionHandler. But it is unclear whether we could ever go there.
Doing a cost/benefit evaluation, I would tend to close this issue with no action.

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

No branches or pull requests