[webvtt] Restrict css attributes in ::cue.

NEW
Assigned to

Status

()

Core
Audio/Video: Playback
P3
normal
2 years ago
a year ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 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
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=
Cameron, 
Great to have your help on this!
(Assignee)

Updated

2 years ago
Assignee: nobody → bechen
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 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

a year 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.
Attachment #8827375 - Flags: review?(cam) → review+
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.
(Assignee)

Comment 7

a year 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.
Depends on: 1318542
OK, even better. :-)
(Assignee)

Comment 9

a year 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
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.