-
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
Add a section that explains the CSS extensions to authors. #317
Conversation
Hmm, I rebased on the gh-pages branch - not sure why the two commits of Simon snuck in... ignore for now and just provide feedback on my patches. |
Marked as non-substantive for IPR from ash-nazg. |
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 working on this! Let me know if you would like me to fix my requested changes myself, that's OK with me.
|
||
<p>The ''::cue'' pseudo-element describes the content of all cues of a video element.</p> | ||
|
||
<pre> |
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 be <div class="example">
<pre> | ||
CSS example: | ||
|
||
video.upper ::cue { text-transform: uppercase } |
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.
No space in the selector (a space is a descendant combinator)
</pre> | ||
|
||
<p>The selector ''::cue'' does not match any real document element. It does match a pseudo-element | ||
that conforming user agents will insert into the shadow dom of the video element to render 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.
I would prefer to not mention "shadow dom" -- that is an implementation detail.
<pre> | ||
CSS example: cue span component matching | ||
|
||
video::cue(v) { text-transform: uppercase} |
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.
Space before }
|
||
video::cue(v) { text-transform: uppercase} | ||
|
||
This rule means "for all video elements, change the letters of all the voice tags in all the cues to |
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.
"WebVTT Voice Objects" instead of "voice tags"?
<pre> | ||
CSS example: | ||
|
||
video.upper ::cue { text-transform: uppercase } |
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.
'text-transform' is not a safelisted property, it does not apply for WebVTT. Maybe use 'color' instead?
<pre> | ||
CSS example: | ||
|
||
video.upper ::cue-region { text-transform: uppercase } |
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.
Remove space in selector
</pre> | ||
|
||
<p>The selector ''::cue-region'' does not match any real document element. It does match a pseudo-element | ||
that conforming user agents will insert into the shadow dom of the video element to render 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.
Same as earlier about "shadow dom"
@@ -4592,7 +4718,7 @@ object">WebVTT region objects</a> must take their initial values.</p> | |||
then they must be interpreted as defined in the next section.</p> | |||
|
|||
|
|||
<h4 id=css-extensions>CSS extensions</h4> | |||
<h3 id=css-extensions>Processing CSS extensions</h3> |
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 this should be a subsection of the "CSS extensions" section above, so the authoring intro and the processing model are together.
Thanks for the review - would love for you to take this over - I just made it to help. |
which it did enormously. Simon, can you fix it up and apply the patch? Many many thanks for the review and (prospectively) the action…
David Singer |
Marked as non-substantive for IPR from ash-nazg. |
Finally got around to this. Please review the new text. |
looking good, but you need to resolve the conflict. Thanks! |
Thank you! I've cleaned up the branch for landing. |
Thanks! |
No substantial changes are necessary to the rendering sections, since the new section is merely informative.
Some small cleanup of the rendering section title and subsectioning is included.
Closes #313
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28408