1321488 - [webvtt] Restrict css attributes in ::cue.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Benjamin Chen [:bechen]]

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

[Cameron McCormack (:heycam)]

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=

Comment 2

[Blake Wu [:bwu][:blakewu]]

Cameron, 
Great to have your help on this!

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1321488 - Restrict css attributes for ::cue.

Review commit: https://reviewboard.mozilla.org/r/105070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105070/

Comment 4

[Benjamin Chen [:bechen]]

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

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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.

Comment 7

[Benjamin Chen [:bechen]]

(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

[Cameron McCormack (:heycam)]

OK, even better. :-)

Comment 9

[Benjamin Chen [:bechen]]

(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

Comment 10

[Alastor Wu [:alwu]]

It seems there are already some work left, I will continue to finish this bug.

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1321488 - restrict CSS properties for '::cue'

According to the spec [1], only those CSS properties listed on the spec can be applied on the ::cue.

[1] https://www.w3.org/TR/webvtt1/#the-cue-pseudo-element

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49799d898958
restrict CSS properties for '::cue' r=emilio

Comment 13

[Dorel Luca [:dluca]]

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

Comment 14

[Alastor Wu [:alwu]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93b6bc89bf32
restrict CSS properties for '::cue' r=emilio

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/93b6bc89bf32

Comment 17

[Eric Shepherd [:sheppy]]

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