-
Notifications
You must be signed in to change notification settings - Fork 30
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
Comments
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. |
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.
...
} |
I don’t understand; where in the spec does it specify that
|
Indeed.
And |
How do all other specs that take enums as arguments deal with this? It seems unnecessary and duplicative just to have to add a |
If we can’t find a way to make
Which could be used in a simple function:
Edit: people have told me this example is confusing. See my comment #228 (comment) for clarification. |
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 |
Yes that’s a great question. Is the end goal that the 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 |
FWIW, this is still being argued about in the WebIDL spec: whatwg/webidl#107 |
One “solution” suggested by @tabatkins in the OP of that issue, when applied in this context, would look like:
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. |
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 |
Someone pointed out off-list that my That said, maybe adding a |
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 |
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 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 @jernoble @steimelchrome @chrisn WDYT? |
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 |
WebIDL changes won't be needed. I'll be OOO next week but I'd love if it could be discussed at the WG meeting. |
@chrisn Please add the |
Just a side note that you should now be able to manage issue labels on this repository. |
I'm not sure I quite understand what problem this is solving. Is the main goal to replace |
To expand a bit more:
|
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). |
I agree with @domenic's option 1 here. This is why
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', () => { /* ... */ });
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. |
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.
While contemplating how to write that using function supportsAction(action) {
try {
navigator.mediaSession.getActionHandler(action);
return true;
} catch (e) {
if (e.name != "TypeError") throw e;
return false;
}
} |
It's also worth noting that |
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 |
That's an ergonomics argument, and those have been rejected in the past for all feature detection. |
@jan-ivar Can you share links to similar issues where this was rejected? |
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. |
See #148 to understand why we didn't go with this pattern. |
Having For instance, even though |
I don't mean callback ActionHandler = any ();
[Exposed=Window]
interface MediaSession {
attribute ActionHandler? onpause;
attribute ActionHandler? onstop;
...
}
if ('onpause' in navigator.mediaSession) { /* supported */ } |
Notes from a brief discussion at the 10 Aug 2021 Media WG meeting are here. |
This is the kind of feature creep I would like to avoid. I most definitely don't want to add a 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. |
I still don't understand why you'd add anything here. Did anyone have a rebuttal to my #228 (comment) ? |
I thought #228 (comment) covered it:
|
No, sorry, it did not cover that.
The same is true for a new feature detection API, when developers don't use
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. |
I thought we were converging on silent failure, i.e., non-throwing
We now have two proposals: from @beaufortfrancois in #228 (comment):
and from @jan-ivar in #228 (comment) (which would be a bigger change from what has already shipped). |
I apologize for missing that part of #228 (comment)! |
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 - navigator.mediaSession.onpause = () => { ... };
+ navigator.mediaSession.actions.pause = () => { ... }; Now the two proposal seem to be:
Pros of 1: gives us getters for the previous action handlers set, for free. 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. |
@beaufortfrancois said:
Making that attribute |
@jan-ivar I believe your proposal "2" does not reflect the one at #228 (comment). It should look like this:
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 |
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', () => { ... });
}
It's not just for the sake of feature detection, since we gain a getter. Option 1 is also more idiomatic to JS:
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 |
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? |
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 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? |
Do we need to revive the discussion here? |
Demoting to P2 and tentatively moving to V2 unless we hear other feedback. |
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. |
Here's an example of someone doing the try catch in the wild and using the (this is in no way representative of anything... it's just an example) We found some other examples through GitHub search. |
I do not think this API is required from web developers, the try/catch approach is good enough. |
From a discussion with @beaufortfrancois and @mounirlamouri. We should have a way of detecting what action a UA supports e.g.
or
The text was updated successfully, but these errors were encountered: