[webvtt] Restrict css attributes in ::cue.
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: bechen, Assigned: alwu)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
The ::cue only supports these css attributes: color opacity visibility text-decoration text-shadow background outline font including line-height white-space text-combine-upright ruby-position
Comment 1•7 years ago
|
||
To implement this, I would follow the example of how we restrict properties to, for example, ::first-letter. http://searchfox.org/mozilla-central/search?q=CSS_PROPERTY_APPLIES_TO_FIRST_LETTER%5Cb&case=false®exp=true&path=
Comment 2•7 years ago
|
||
Cameron, Great to have your help on this!
Reporter | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8827375 [details] Bug 1321488 - Restrict css attributes for ::cue. https://reviewboard.mozilla.org/r/105068/#review105884 In the patch, except the allow attributes in spec, we allow "top bottom left right width height text-align position" attributes set by vtt.jsm.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8827375 [details] Bug 1321488 - Restrict css attributes for ::cue. https://reviewboard.mozilla.org/r/105070/#review114066 r=me with these things addressed. ::: layout/style/nsCSSPropList.h:497 (Diff revision 1) > BackgroundAttachment, > CSS_PROPERTY_PARSE_VALUE_LIST | > CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE | > CSS_PROPERTY_APPLIES_TO_PLACEHOLDER | > - CSS_PROPERTY_VALUE_LIST_USES_COMMAS, > + CSS_PROPERTY_VALUE_LIST_USES_COMMAS | > + CSS_PROPERTY_APPLIES_TO_CUE, Nit: please keep the CSS_PROPERTY_APPLIES_TO_CUE value next to the other CSS_PROPERTY_APPLIES_TO_* values (i.e., move it up a few lines). Same throughout the file. ::: layout/style/nsCSSPropList.h:3606 (Diff revision 1) > CSS_PROP_TEXT( > ruby-position, > ruby_position, > RubyPosition, > - CSS_PROPERTY_PARSE_VALUE, > + CSS_PROPERTY_PARSE_VALUE | > + CSS_PROPERTY_APPLIES_TO_CUE, I have no idea why the spec allows ruby-position but not the other two ruby formatting properties, ruby-merge and ruby-align. Can you ask the spec authors whether there is a reason behind this? ::: layout/style/nsCSSPropList.h:4093 (Diff revision 1) > CSS_PROP_VISIBILITY( > text-orientation, > text_orientation, > TextOrientation, > - CSS_PROPERTY_PARSE_VALUE, > + CSS_PROPERTY_PARSE_VALUE | > + CSS_PROPERTY_APPLIES_TO_CUE, Did you mean to include text-orientation? I don't see it mentioned in the spec. ::: layout/style/nsCSSProps.h:284 (Diff revision 1) > +#define CSS_PROPERTY_APPLIES_TO_CUE (1U<<31) > + Please add a comment above here describing what CSS_PROPERTY_APPLIES_TO_CUE means. Also, it would be good in that comment to mention how we allow {top, right, bottom, left, text-align, position} in ::cue but that through the rules the UA sets, these can't ever have an effect when specified by the author.
Comment 6•7 years ago
|
||
Also, it would be great to have a test that {top, right, bottom, left, text-align, position} have no effect when set by the author.
Reporter | ||
Comment 7•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #6) > Also, it would be great to have a test that {top, right, bottom, left, > text-align, position} have no effect when set by the author. The patch Attachment 8818222 [details] that I try to apply the ::cue to cueStyleBox is not correct. That makes me to allow {top, right, bottom, left, text-align, position} here. There are 2 divs, the outer one is for moving the cue, and the inner one is the cueStyleBox(cue background box ), so I should apply ::cue to the inner one, not the outer div. And then, maybe not allow {top, right, bottom, left, text-align, position} here.
Comment 8•7 years ago
|
||
OK, even better. :-)
Reporter | ||
Comment 9•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away 25 Feb–5 Mar) from comment #5) > Comment on attachment 8827375 [details] > Bug 1321488 - Restrict css attributes for ::cue. > > ::: layout/style/nsCSSPropList.h:3606 > (Diff revision 1) > > CSS_PROP_TEXT( > > ruby-position, > > ruby_position, > > RubyPosition, > > - CSS_PROPERTY_PARSE_VALUE, > > + CSS_PROPERTY_PARSE_VALUE | > > + CSS_PROPERTY_APPLIES_TO_CUE, > > I have no idea why the spec allows ruby-position but not the other two ruby > formatting properties, ruby-merge and ruby-align. Can you ask the spec > authors whether there is a reason behind this? Make a note: https://github.com/w3c/webvtt/issues/265
Updated•6 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 10•4 years ago
|
||
It seems there are already some work left, I will continue to finish this bug.
Assignee | ||
Comment 11•4 years ago
|
||
According to the spec [1], only those CSS properties listed on the spec can be applied on the ::cue
.
Comment 12•4 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49799d898958 restrict CSS properties for '::cue' r=emilio
Comment 13•4 years ago
|
||
Backed out changeset 49799d898958 (bug 1321488) for Mochitest failures in dom/base/test/test_audioNotificationSilent_audioFile.html
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253193265&repo=autoland&lineNumber=3500
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=49799d8989589e9efc34091979ab77a5d2ff53c1
Backout:
https://hg.mozilla.org/integration/autoland/rev/cb3f98079c964432d93b386362548a795e50ec36
Assignee | ||
Comment 14•4 years ago
|
||
That failure is 100% impossible caused by my changes, my patches doesn't include any media changes. I also checked other pushed after my commit, the failure didn't happen on every pushes, which means it's intermittent and there might be a real culprit behind.
Comment 15•4 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/93b6bc89bf32 restrict CSS properties for '::cue' r=emilio
Comment 16•4 years ago
|
||
bugherder |
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 17•4 years ago
|
||
Documentation updates:
- Minor cleanup to the ::cue page to improve flow of the content and to alphabetize the names of the permitted CSS properties.
- Submitted BCD PR 4571 to add indication to
::cue
that Firefox 69 now enforces the spec-defined limits on which CSS properties can be used in cues (bug 1321488). - Updated Firefox 69 for developers
Description
•