-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Unbounded TextTrackCue end time #5953
Unbounded TextTrackCue end time #5953
Conversation
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 change looks good, a pretty straightforward expansion of the allowed values that I think ought to work. The setter needs some changes, but that's it.
source
Outdated
a <span>media element</span>'s <span>list of text tracks</span>, and the <span>media | ||
element</span>'s <span>show poster flag</span> is not set, then run the <i data-x="time marches | ||
on">time marches on</i> steps for that <span>media element</span>.</p> | ||
<code>TextTrackCue</code> object represents, in seconds or positive Infinity for an |
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.
Because the IDL type of the attribute is now unrestricted double, the setter here needs to also handle -Infinity and NaN. I would suggest throwing a TypeError, which is what would currently happen. Run new VTTCue(0, 1, '').endTime = Infinity
to see this in action.
https://html.spec.whatwg.org/#dom-input-valueasnumber has an example of this: "On setting, if the new value is infinite, then throw a TypeError exception." (You'll probably need to massage it a bit to fit into this paragraph.)
No reference to "unbounded text track cue" ought to be needed here in the end, because that's defined in terms of what is being set.
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 your feedback and apologies for the delay. I've added a check "on setting" to throw TypeError for -Infinity and NaN as you suggest, and have removed the reference here to "unbounded text track cue".
source
Outdated
in a <span>text track</span>'s <span data-x="text track list of cues">list of cues</span>, and | ||
that <span>text track</span> is in a <span>media element</span>'s <span>list of text | ||
tracks</span>, and the <span>media element</span>'s <span>show poster flag</span> is not set, then | ||
run the <i data-x="time marches on">time marches on</i> steps for that <span>media element</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.
Yep, looks like this is the only change really needed. https://html.spec.whatwg.org/#time-marches-on tests for "whose end times are greater than the current playback position" and that'll work for infinity.
Added support for unbounded TextTrackCue - see whatwg/html#5953 Whitespace removed by Atom
Update for TextTrackCue unbounded endTime proposed in PR whatwg/html#5953
Looks like TextTrackCue has tests in wpt here https://github.com/web-platform-tests/wpt/blob/master/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/endTime.html |
Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493
Added unbounded cue tests in web-platform-tests/wpt#28394 |
@annevk I believe the recent addition of web-platform-tests/wpt#28394 completes the information required for this issue, but I still see a failing check.
Please advise if there are further actions I need to take to resolve this. Thanks. |
Hey @rjksmith, thanks! We'll resolve that when this is ready to merge as per https://github.com/whatwg/participant-data/#maintenance. It does seem like you'll need to rebase this PR and @foolip needs to review. |
Fixes whatwg#5297 <!-- Thank you for contributing to the HTML Standard! Please describe the change you are making and complete the checklist below if your change is not editorial. --> - [ ] At least two implementers are interested (and none opposed): * … * … - [ ] [Tests](https://github.com/web-platform-tests/wpt) are written and can be reviewed and commented upon at: * … - [ ] [Implementation bugs](https://github.com/whatwg/meta/blob/master/MAINTAINERS.md#handling-pull-requests) are filed: * Chrome: … * Firefox: … * Safari: … (See [WHATWG Working Mode: Changes](https://whatwg.org/working-mode#changes) for more details.)
Added type validation for unrestricted TextTrackCue.endTime & removed reference to unbounded text track cue.
d348935
to
17ef7f3
Compare
Rebased to main as requested |
Update for TextTrackCue unbounded endTime proposed in PR whatwg/html#5953
Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493
Also rebased w3c/webvtt#493 and web-platform-tests/wpt#28394 for associated VTTCue and Web Platform Tests changes respectively |
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.
I have but one remaining nit. I can rewrap the result if you don't feel like messing with it. (But feel free to.)
<p>An <dfn data-x="unbounded text track cue">unbounded text track cue</dfn> is a text track cue | ||
with a <span>text track cue end time</span> set to positive Infinity. An active <span>unbounded | ||
text track cue</span> cannot become inactive through the usual monotonic increase of the | ||
<span>current playback position</span> during normal playback (e.g. a metadata cue for a chapter in a live |
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.
Good example!
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 to @chrisn for that. Further use case discussion can be found in w3c/webvtt#496 which has been decoupled from this change.
@rjksmith it turns out that I can't push the rewrap directly to you branch. Since it's a bit tedious, here it is for you to copy-paste directly, if you could push another commit that'd be great:
|
@foolip Thanks for the text. Updated as requested |
Now we just need the WebVTT change. It doesn't have to be merged at the same time, but be approved at least so it's not too far apart. |
w3c/webvtt#493 updated as requested |
@rjksmith I've reviewed that, suggesting some changes. |
Updated implementation bugs for Chrome, Firefox and Safari with recent progress. |
OK, I'm going to bet on w3c/webvtt#493 being merged soon, and merge this + web-platform-tests/wpt#28394 now. @rjksmith can you keep an eye on the WebVTT change, and circle back here if it doesn't end up merged within a week? |
Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493
…only Automatic update from web-platform-tests Tests for unbounded cue end time (#28394) Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493 -- wpt-commits: 786c430165d916bdab740ab491ac541afb3f8bf5 wpt-pr: 28394
@foolip WebVTT change is still not merged. There's an agenda item to resolve this in TTWG Meeting on 29th April 2021.
|
It's mostly unmerged because I haven't had a chance to get back to it yet, unfortunately. I believe there isn't anything that'll block it from being merged on the TTWG side, though, I'll wait for the final resolution from the meeting. |
@gkatsev I can attend the TTWG meeting on 29th April to respond to any concerns raised. If that would be helpful, please let me know how to join. Thanks. |
I'll send you information via email. |
@foolip I can confirm that w3c/webvtt#493 has now been successfully merged to support web-platform-tests/wpt#28394 which concludes the required changes for this PR. I've also updated the implementation bugs for Chrome, Firefox and Safari accordingly. Many thanks to all those who have contributed and especially to @chrisn, @eric-carlson, @annevk, @foolip and @gkatsev for their invaluable support. |
Thanks @rjksmith! Just to know what to expect, will you be doing any work on any implementation here? |
@chrisn and I have been discussing ideas to encourage implementation. I'm considering a code contribution to Mozilla/Firefox. |
Allow unbounded text track cue duration, as discussed in whatwg/html issue #5297.
Summary
It is proposed that a
TextTrackCue.endTime
value ofInfinity
be used to represent an unbounded time, i.e. an unspecified future time. This is a simple extension of the existing HTML standard wheremedia.duration = Infinity
represents the duration of an unbounded stream, which is consistent with the definition of unbounded time.Implementer Details
(See WHATWG Working Mode: Changes for more details.)
cc @chrisn
/acknowledgements.html ( diff )
/media.html ( diff )