-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[HTMLMediaElement] Add a controlsList/controlslist attribute. #2426
Conversation
Adds a controlslist content attribute reflected by a DOMTokenList dom attribute named controlsList to the HTMLMediaElement and <audio>/<video> tags. Specifies three supported keywords: nodownload, nofullscreen, and noremoteplayback. Documents the change proposed in whatwg#2293
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! This is indeed the maximally clear form for providing an interoperable spec others could implement, even if currently there is no other implementer interest. Great stuff.
So, despite this not likely being merged soon, I went through and did a quick review, which I hope is helpful.
@@ -31033,6 +31035,9 @@ interface <dfn>HTMLMediaElement</dfn> : <span>HTMLElement</span> { | |||
|
|||
// controls | |||
[<span>CEReactions</span>] attribute boolean <span data-x="dom-media-controls">controls</span>; | |||
[<span>CEReactions</span>, SameObject, PutForwards=<span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out we recently discovered CEReactions is not necessary for DOMTokenList attributes: #2378. (It's actually disallowed these days.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
data-x="attr-media-controlslist-nofullscreen">nofullscreen</code></dfn>, and | ||
<dfn><code data-x="attr-media-controlslist-noremoteplayback">noremoteplayback</code></dfn>. | ||
|
||
<p>The <code data-x="attr-media-controlslist-nodownload">nodownload</code> keyword causes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be "causes ... to be hidden", or maybe instead "hints that ... should be hidden"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
values defined in the <code data-x="attr-media-controlslist">controlslist</code> attribute and | ||
supported by the user agent.</p> | ||
|
||
<p>The user agent might ignore the author's preference depending on the environment, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: this seems like a normative "may", not a "might". I'd then move the example to
<p class="example">A user agent might ignore the <code data-x="attr-media-controlslist-nofullscreen">nofullscreen</code>
keyword if the content area containing the video is small, such as on a mobile device.</p>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
<td> <code data-x="attr-media-controlslist">audio</code>; | ||
<code data-x="attr-media-controlslist">video</code> | ||
<td> Show/hide specific user agent controls | ||
<td> <span>DOMTokenList</span> attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ideally follow the example of iframe sandbox in the index, i.e.
Unordered set of unique space-separate tokens, ASCII case-insensitive, consisting of ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
data-x="attr-media-controlslist-noremoteplayback">noremoteplayback</code> keyword causes the remote | ||
playback control to be hidden when using the user agent's own set of controls for the media | ||
element.</p> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding something like
<p class="note">Hiding these aspects of the user agent's own controls does not necessarily disable
the related functionality. For example, the user agent might present the same functionality through a
context menu or keyboard shortcut.</p>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Adds a DOMTokenList backed controlsList/controlslist attribute to HTMLMediaElement with three keywords: nodownload, nofullscreen and noremoteplayback. Spec change is discussed here: whatwg/html#2293 Spec change PR is here: whatwg/html#2426 WICG repo for the API is here: https://github.com/WICG/controls-list Intent to ship is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/discussion BUG=650174,685018 TEST=manual+layout tests Review-Url: https://codereview.chromium.org/2657723002 Cr-Commit-Position: refs/heads/master@{#455926}
Adds a DOMTokenList backed controlsList/controlslist attribute to HTMLMediaElement with three keywords: nodownload, nofullscreen and noremoteplayback. Spec change is discussed here: whatwg/html#2293 Spec change PR is here: whatwg/html#2426 WICG repo for the API is here: https://github.com/WICG/controls-list Intent to ship is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tFuQd3AcsIQ/discussion BUG=650174,685018 TEST=manual+layout tests Review-Url: https://codereview.chromium.org/2657723002 Cr-Commit-Position: refs/heads/master@{#455926} (cherry picked from commit caca068) Review-Url: https://codereview.chromium.org/2741933004 . Cr-Commit-Position: refs/branch-heads/3029@{#132} Cr-Branched-From: 939b32e-refs/heads/master@{#454471}
Closing as this seems abandoned. |
I believe this is shipping in Chrome so I think it's best to leave open as a record of such? If we want to start closing single-vendor PRs then I at least would want to work on some sort of archive... |
Agreed! |
Can't "a record of such" be in Chrome's documentation? I don't think we should leave PRs open for years and years if it's pretty clear it's not going to get the multi-implementer interest it needs to land. |
Alternately, a closed PR is literally "a record of such". PRs don't stop existing when they get closed. |
For info, the Chrome team is thinking about adding another value |
Yes, that plus a rebase would be ideal. |
@domenic Is it even possible for me to update this PR as I'm not the original owner? |
You would need to get write access to @avayvod's fork. Otherwise creating a new PR would make the most sense. |
Rolling forward to #6715. |
Adds a controlslist content attribute reflected by a DOMTokenList dom
attribute named controlsList to the HTMLMediaElement and
<audio>
/<video>
tags.
Specifies three supported keywords: nodownload, nofullscreen, and
noremoteplayback.
Documents the change proposed in #2293 (unlikely to be merged).
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Apr 28, 2021, 3:29 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.