-
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
Add TextTrackCue ctor & cue + cuebackground attributes #9771
base: main
Are you sure you want to change the base?
Conversation
cb842a6
to
e46b5a0
Compare
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 realize this is still a draft, but I thought I'd give some minor editorial feedback that might help with other changes.
47da44c
to
5362a5e
Compare
@zcorpan, Anne suggested I ping you for guidance and review... the proposal doesn't currently talk much about the rendering side of things, so could use some guidance there. I'll take a look at how some other elements define things, but any pointers would be great. |
A thought about rendering from the requirements perspective: typically a TextTrackCue would begin rendering at its begin time, but the target behaviour should be to complete rendering as close as possible to the begin time. If this proposal can be worded to give the UA some flexibility to pre-load the HTML fragment and begin rendering ahead of the begin time so that this goal can be achieved, that would be another accessibility benefit of this proposal. Same applies for clearing the HTML fragment from the DOM and having the presentation update as closely as possible to the TextTrackCue end time, obviously. |
5362a5e
to
0ab61ed
Compare
source
Outdated
<p>When the <code data-x="attr-cuebackground">cuebackground</code> attribute is present, the element serves as cue background | ||
for a <span>text track cue</span>.</p> | ||
|
||
<p>The <code data-x="attr-cue">cue</code> and <code data-x="attr-cuebackground">cuebackground</code> attributes must each be present once.</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.
Does this mean at least once per fragment? Otherwise it is overly restrictive, since a cue fragment may reasonably contain multiple descendant elements that each should be styled as a 'cue' or 'cue background'.
<p>The <code data-x="attr-cue">cue</code> and <code data-x="attr-cuebackground">cuebackground</code> attributes must each be present once.</p> | |
<p>The <code data-x="attr-cue">cue</code> and <code data-x="attr-cuebackground">cuebackground</code> attributes must each be present on at least one element in the <var>cueFragment</var>.</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.
Right, this was intended more user agent requirement (each must appear once... but that's enforce the processing algorithm). I'll rephrase it so it's more clear as per your intent (and yes, can definitely have multiple).
source
Outdated
<p>When the <code data-x="attr-cue">cue</code> attribute is present, the element serves as a <span>text track | ||
cue</span>.</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.
Isn't this circular, since the element only serves as a text track cue if it is attached to a TextTrackCue object? Adding a cue
attribute to some DOM element doesn't inherently make it a text track cue without constructing a TextTrackCue
.
Is there an additional concept needed here, which is to identify somehow the payload text element of a text track cue, i.e. that which should be styled using the ::cue
pseudo-element?
Maybe something conceptually like:
<p>When the <code data-x="attr-cue">cue</code> attribute is present, the element serves as a <span>text track | |
cue</span>.</p> | |
<p>When the <code data-x="attr-cue">cue</code> attribute is present, the element can be styled as the text content of a <span>text track | |
cue</span> using the <span>::cue</span> <span>text track cue | |
pseudo-element</span>.</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.
In the explainer we had an abstract concept a "cue fragment", so might bring that in... something like:
A cue fragment is a DocumentFragment containing cue text, represented in a limited subset of HTML and styled with CSS with ::cue and ::cue-background.
https://bugs.webkit.org/show_bug.cgi?id=262053 rdar://116002871 Reviewed by Eric Carlson. Implements spec changes suggested at: whatwg/html#9771 Specifically: * supports `b` and `i` elements, to match WebVTT * drops support for allowing `img` in the cue fragment * implements more robust error checking to prevent malformed ::cue and :cuebackground hierarchies * As such, ::tagPseudoObjects() can now throw * Switch from InvalidStateError to InvalidNodeTypeError for when required attributes are missing * implements cuebackground and cue global attributes * LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/HTMLElement-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/HTMLElement.html: * LayoutTests/imported/w3c/web-platform-tests/html/dom/elements/global-attributes/cue-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/dom/elements/global-attributes/cue.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/dom/elements/global-attributes/cuebackground-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/dom/elements/global-attributes/cuebackground.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/historical-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/historical.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/constructor-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/constructor.html: * LayoutTests/inspector/model/remote-object/dom-expected.txt: * LayoutTests/media/track/texttrackcue/texttrackcue-constructor-expected.txt: * LayoutTests/media/track/texttrackcue/texttrackcue-constructor.html: * Source/WebCore/html/HTMLAttributeNames.in: * Source/WebCore/html/HTMLElement.idl: * Source/WebCore/html/track/TextTrackCue.cpp: (WebCore::isLegalNode): (WebCore::tagPseudoObjects): (WebCore::removePseudoAttributes): (WebCore::TextTrackCue::create): (WebCore::TextTrackCue::setPauseOnExit): (WebCore::cueAttributName): Deleted. (WebCore::cueBackgroundAttributName): Deleted. Canonical link: https://commits.webkit.org/268644@main
Chrome is generally supportive of this effort, but won't have the bandwidth to implement the changes any time soon. |
0ab61ed
to
0087d58
Compare
I think this goes a little to far in preventing the creation of a I'm not sure the best document change to allow this, whether to explicitly allow the |
4489cb1
to
69001aa
Compare
Right, but you could provide just an empty, for example
Can you help me understand that use case a little bit more?... I'm also leaning towards just allowing the |
There are loads of use cases for this. Generically, anyone wanting to subclass Assuming that the subclass has to instantiate the base class, the base class constructor needs to have the HTML fragment as an optional parameter. It's a small thing, especially in Ecmascript, but I'd lean towards allowing omission of the HTML fragment rather than specifying an empty string for it, just for cleanliness. |
Tagging @zcorpan and @alastor0325 per the discussion in #9836. |
The rendering of WebVTT is currently specified as a custom non-CSS renderer of a non-DOM tree. I believe browsers have mostly implemented the rendering using CSS and DOM. I think they also don't follow the rendering requirements exactly, e.g. Safari has some padding on the background box and rounded corners, whereas the spec requires "uglier" background box without padding. Maybe this could be an opportunity to make the spec more aligned with implementations? |
We discussed internally with WebKit folks, and yes: we should actually fix the interop issues with WebVTT across user agents in addition to this. We still think this proposal might still be worth pursuing, but fixing WebVTT would probably give a higher return on investment. We might let this proposal sit for a while, while we focus on increasing interop of WebVTT. |
Native UA rendering of out-of-band subtitles and captions is currently only possible with WebVTT. If a site can't or won't make its subtitles available in-band, and can't or won't publish its subtitles and captions in WebVTT, the site must render its subtitles and captions itself.
This pull request aim to:
This pull requests makes the following changes to HTML:
TextTrackCue
which takes startTime, endTime, and cueNode arguments.cue
andcuebackground
global attributes.::cue
and::cue-background
pseudo elements, which can then be styles by the CSS or the system.This pull request is based on:
https://github.com/WebKit/explainers/tree/main/texttracks
(See WHATWG Working Mode: Changes for more details.)
/dom.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/media.html ( diff )