Benjamin Chen [:bechen] Reporter
	Description • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 1 • 8 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&regexp=true&path=

	Blake Wu [:bwu][:blakewu]
	Comment 2 • 8 years ago

Cameron, 
Great to have your help on this!

	Benjamin Chen [:bechen] 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.

	Cameron McCormack (:heycam)
	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.

	Cameron McCormack (:heycam)
	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.

	Benjamin Chen [:bechen] 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.

	Cameron McCormack (:heycam)
	Comment 8 • 7 years ago

OK, even better. :-)

	Benjamin Chen [:bechen] 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

	Pulsebot
	Comment 12 • 5 years ago

Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49799d898958
restrict CSS properties for '::cue' r=emilio

	Pulsebot
	Comment 15 • 5 years ago

Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93b6bc89bf32
restrict CSS properties for '::cue' r=emilio