Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New ATTRIBUTES block #523
base: main
Are you sure you want to change the base?
New ATTRIBUTES block #523
Changes from 1 commit
18b3b37
665bc57
ae202da
f4f4e31
992f234
7824b46
5423996
3011cc4
f4fd5f8
840e17e
d6ca24e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 example isn't a great place to put definitional content. The WebVTT spec does not currently distinguish between subtitles and captions and that's probably a good thing. If there's an intended linkage between the
kind
attribute and the HTMLtrack
element'skind
attribute then that should be explicit in the spec text.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 included that in the "WebVTT attributes key/value parsing rules."
Did you miss the above text, or is there some additional place where you'd like it added? Thanks for clarifying.
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 may have missed it, but I can see why: that's appearing as part of §6.6 "WebVTT rules for extracting the chapter title" - shouldn't it be in its own numbered section?
Aside from that, what does "process the value as ..." mean? Does it mean that it takes precedence over the same attribute on the
<track>
element? Seems like we need to say something about precedence in this scenario.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 agree, it should be a separate section.
I was wondering the same. Are these purely metadata or do they interact with the HTML/DOM in any way? And what is the order of precedence if both values are used? The track element wins? That might need a change in HTML?
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.
Section mismatch was probably due to my unfamiliarty with Bikeshed. I will correct it and look at your other question at the same time.
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.
My feeling here is that the number of examples enumerating different values of
kind
is unnecessary given that this specification is not defining that attribute's value set. Better to have one example and, somewhere, link the value set to its canonical definition.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'm not sure I understand... Are you suggesting the value parsing algo cross reference the HTML Infra values in
track[kind]
?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.
Oh, upon re-reading, it sounds like you're suggesting the examples be removed.
FWIW, as a reader of the spec (this was my first PR to VTT), I found the pre-existing examples helpful. Perhaps others will too? The ATTRIBUTES block is defined here, so seeing how it's used with different key/value pairs seems useful to me, even if those values are defined by HTML.
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.
There is one of each:
I'd prefer to leave all 4 examples, but perhaps we can compromise on just removing the subtitle 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.
I still think there are too many but it's not a major complaint; as you say, many folk find examples an easy way into understanding the specification.
Also I just noticed that Example 14 is appearing in the Chapters section, but its about descriptions, so it seems to be misplaced.
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 tend to like a lot of examples.
Given how similar the subtitles and captions examples are, maybe we only keep one?
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.
we probably want this in a separate section on descriptions, alternatively, should move to section 1.4 Other caption and subtitling features rather than being in the chapters section.
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 am not sure, but this item seems to be out from this
<ol>
, since it divides theATTRIBUTES
line but not in key/value pairsThere 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 intended that as the line termination char on the same line as ATTRIBUTES
But I'd be happy to combine it with the previous line, or specify some other way at the editors' direction.
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 think we can remove a level here. So, at the top say "attributes block consists of the following components, in the given order:"
This would match the webvtt region block definition too: https://pr-preview.s3.amazonaws.com/cookiecrook/webvtt/pull/523.html#webvtt-region-definition-block https://github.com/w3c/webvtt/blob/main/index.bs#L1500-L1511
The "attributes block body" (current lines 1774-1797) could either be inline or could move into its own subsection.