-
Notifications
You must be signed in to change notification settings - Fork 40
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
Timeranges #419
base: gh-pages
Are you sure you want to change the base?
Timeranges #419
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.
Although this change is fine in its own right, it only partially addresses the issue. In particular see #415 (comment) - I think further edits are needed to clarify the scope of inheritance of Text Track Cue so it can not be interpreted as only applying to the data model.
Also remove "other playback mechanism" where that is inconcrete.
@nigelmegitt added as requested, please re-review |
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.
Almost there...
index.bs
Outdated
implemented.</p> | ||
<p>This section describes how to visually render WebVTT cues in a user agent. The processing model | ||
is tightly linked to media elements in HTML and follows the handling of cues when <a>playing the | ||
media resource</a>. When supporting WebVTT in media players that don't support CSS, equivalent |
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.
"don't support CSS" now needs to be expanded to "don't support CSS and HTML5" I think
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.
HTML5 is being used to define semantics of cue times; do I need to "support HTML5" to get that?
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.
@nigelmegitt how about "don't support HTML nor CSS" - I just want to make sure players that support CSS but not HTML are covered.
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.
Sorry @dwsinger I didn't spot your response a week ago. I think that you do not need to support HTML5 to match the semantics of cue times, in theory, though you might in practice!
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.
@silviapfeiffer the correct wording would be "that support neither HTML nor CSS" I think, or alternatively "that do not support HTML and do not support CSS" to be super clear.
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.
@silviapfeiffer sorry for the confusion, but I think we would need to clarify what implementations that support HTML but not CSS, or CSS but not HTML would do with my proposed wording at #419 (comment) and also in the more recent update. I suspect the better option is to say "that do not support both HTML and CSS".
Please re-review |
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.
Small grammar change to recent fix - I've made a couple of proposals to choose from for how to resolve.
index.bs
Outdated
with a full CSS engine would render.</p> | ||
<p>This section describes how to visually render WebVTT cues in a user agent. The processing model | ||
is tightly linked to media elements in HTML and follows the handling of cues when <a>playing the | ||
media resource</a>. When supporting WebVTT in media players that don't support HTML nor CSS, |
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.
Grammar issue with the recent update, "don't support X nor Y" doesn't work, sadly. I think the correct wording here 'is don't support both HTML and CSS" in place of "don't support HTML nor CSS".
"Support neither CSS nor HTML" would be normal english. |
@dwsinger I had to think about the logic of what this statement is saying too. I believe the sentence applies to all processors that do not support both CSS and HTML, since if either one or both of those is not (fully) supported then the implementation is still expected to behave equivalently. |
I think I'll go back to "don't support CSS" because it doesn't matter whether it supports HTML or not - this is about the rendering. "neither" makes it sound like if you support CSS, but not HTML, it doesn't apply to you, which isn't the case. |
@silviapfeiffer the Rendering section includes the processing model, which is dependent on HTML time marches on, so it is appropriate to refer to HTML support as well as CSS support here. |
@nigelmegitt yes, indeed. That's why the first sentence addresses the lack of HTML: The processing model is tightly linked to media elements in HTML and follows the handling of cues when playing the media resource. The second sentence is just about the visual rendering, which is about CSS. But I can more explicitly refer to the conformance classes "User agents that do not support CSS" and "User agents that do not support a full HTML CSS engine" to clarify this. |
@silviapfeiffer sounds good to me. Just one more thought: are HTML and CSS completely separable in this case, so that there may be an HTML engine that does not support CSS, or is that so niche that it is not worth considering? |
@silviapfeiffer wrote
This works for me. @silviapfeiffer wrote
This work for me too. As the second edit would also work for @nigelmegitt: @silviapfeiffer could you do the edit so this PR can be closed. |
@nigelmegitt yes, there are HTML renderers that do not support a full CSS engine (or even: none) - think of command-line browsers as an example. I've updated the patch, so please re-review. |
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.
Happy with the structure of the recent change - just a few nits.
with a full CSS engine would render.</p> | ||
<p>This section describes how to visually render WebVTT cues in a user agent. The processing model | ||
is tightly linked to media elements in HTML and follows the handling of cues when <a>playing the | ||
media resource</a>. When supporting WebVTT in media players that falls into the <a>User agents that |
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.
Grammar nit: s/in media players/in a media player
<p>This section describes how to visually render WebVTT cues in a user agent. The processing model | ||
is tightly linked to media elements in HTML and follows the handling of cues when <a>playing the | ||
media resource</a>. When supporting WebVTT in media players that falls into the <a>User agents that | ||
do not support CSS</a> or <a>User agents that do not support a full HTML CSS engine</a> conformace |
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.
typo: s/conformace/conformance
is tightly linked to media elements in HTML and follows the handling of cues when <a>playing the | ||
media resource</a>. When supporting WebVTT in media players that falls into the <a>User agents that | ||
do not support CSS</a> or <a>User agents that do not support a full HTML CSS engine</a> conformace | ||
classes, equivalent visual rendering will need to be implemented.</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.
Another nit: s/will need/needs
Closes #415