Bug 194227 – [WebVTT] Inline WebVTT styles should start with '::cue'

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Inline WebVTT starting with e.g. 'video::cue' should not be allowed.

[Radar WebKit Bug Importer]

<rdar://problem/47791087>

[Per Arne Vollan]

Created attachment 361081 [details]
Patch

[Per Arne Vollan]

Created attachment 361083 [details]
Patch

[Brent Fulgham]

Comment on attachment 361083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361083&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=194227

Please show the radar here.

> Source/WebCore/html/track/WebVTTParser.cpp:374
> +    // Check that stylesheet starts with "::cue".

This comment isn't needed (it just repeats what the code says). Maybe it would be better to say "Inline VTT styles must start with ::cue."

> Source/WebCore/html/track/WebVTTParser.cpp:380
> +    // Check that stylesheet can be parsed.

I don't think this comment is needed.

> LayoutTests/media/track/captions-webvtt/css-styling.vtt:30
> +}

I don't understand these changes. Previously we had a green video::cue style. Now we have a blue inline ::cue style, and an identical video::cue style. Shouldn't these be different so we can see in the test if the right one is being used? And why is "video::cue" now blue, instead of green?

[Per Arne Vollan]

Created attachment 361514 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 361083 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361083&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=194227
> 
> Please show the radar here.
> 
> > Source/WebCore/html/track/WebVTTParser.cpp:374
> > +    // Check that stylesheet starts with "::cue".
> 
> This comment isn't needed (it just repeats what the code says). Maybe it
> would be better to say "Inline VTT styles must start with ::cue."
> 
> > Source/WebCore/html/track/WebVTTParser.cpp:380
> > +    // Check that stylesheet can be parsed.
> 
> I don't think this comment is needed.
> 
> > LayoutTests/media/track/captions-webvtt/css-styling.vtt:30
> > +}
> 
> I don't understand these changes. Previously we had a green video::cue
> style. Now we have a blue inline ::cue style, and an identical video::cue
> style. Shouldn't these be different so we can see in the test if the right
> one is being used? And why is "video::cue" now blue, instead of green?

The original style block was changed from 'video::cue' to '::cue', since 'video::cue' is no longer allowed. The two new style blocks with blue color are testing that we don't allow parse errors and 'video::cue'. There is a syntax error in the first style block. If we see blue text, the test has failed.

Thanks for reviewing!

[Eric Carlson]

(In reply to Per Arne Vollan from comment #6)
> 
> Thanks for reviewing!

I also missed the syntax error (missing semi-colon). Please add a comment or make the error more obvious.

[Per Arne Vollan]

Created attachment 361516 [details]
Patch

[Per Arne Vollan]

(In reply to Eric Carlson from comment #7)
> (In reply to Per Arne Vollan from comment #6)
> > 
> > Thanks for reviewing!
> 
> I also missed the syntax error (missing semi-colon). Please add a comment or
> make the error more obvious.

Yes, I think I hid that pretty well :) I have added a comment. Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 361516 [details]
Patch

Clearing flags on attachment: 361516

Committed r241203: <https://trac.webkit.org/changeset/241203>

[Per Arne Vollan]

Created attachment 361956 [details]
Patch

[Per Arne Vollan]

Created attachment 361959 [details]
Patch

[Eric Carlson]

Comment on attachment 361959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361959&action=review

> Source/WebCore/html/track/WebVTTParser.cpp:397
> +        auto listSize = selectorList.listSize();
> +        if (!listSize)
> +            return true;

Do we want to reject the entire style block if any of the selectors is empty?

[Per Arne Vollan]

Created attachment 362030 [details]
Patch

[Per Arne Vollan]

(In reply to Eric Carlson from comment #13)
> Comment on attachment 361959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361959&action=review
> 
> > Source/WebCore/html/track/WebVTTParser.cpp:397
> > +        auto listSize = selectorList.listSize();
> > +        if (!listSize)
> > +            return true;
> 
> Do we want to reject the entire style block if any of the selectors is empty?

That is a good point. I agree that it would be good to allow the valid selectors in a STYLE block. However, I cannot find a method to remove rules from the stylesheet, maybe we can look closer into this in a follow-up patch?

Thanks for reviewing!

[Eric Carlson]

(In reply to Per Arne Vollan from comment #15)
> (In reply to Eric Carlson from comment #13)
> > Comment on attachment 361959 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361959&action=review
> > 
> > > Source/WebCore/html/track/WebVTTParser.cpp:397
> > > +        auto listSize = selectorList.listSize();
> > > +        if (!listSize)
> > > +            return true;
> > 
> > Do we want to reject the entire style block if any of the selectors is empty?
> 
> That is a good point. I agree that it would be good to allow the valid
> selectors in a STYLE block. However, I cannot find a method to remove rules
> from the stylesheet, maybe we can look closer into this in a follow-up patch?
> 
Do we need to remove empty selectors? What happens if you just leave it?

[Eric Carlson]

Comment on attachment 362030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362030&action=review

> LayoutTests/media/track/captions-webvtt/css-styling.vtt:44
> +STYLE
> +::cue {
> +color: blue
> +font-size: 25px;
> +}
> +video::cue {
> +color: blue;
> +font-size: 25px;
> +}

The test should have style blocks that contain the invalid rule types rejected by WebVTTParser::checkAndStoreStyleSheet (namespace, etc).

[Per Arne Vollan]

Created attachment 362045 [details]
Patch

[Per Arne Vollan]

(In reply to Eric Carlson from comment #13)
> Comment on attachment 361959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361959&action=review
> 
> > Source/WebCore/html/track/WebVTTParser.cpp:397
> > +        auto listSize = selectorList.listSize();
> > +        if (!listSize)
> > +            return true;
> 
> Do we want to reject the entire style block if any of the selectors is empty?

That is a good point. I agree that it would be good to allow the valid selectors in a STYLE block. However, I cannot find a method to remove rules from the stylesheet, maybe we can look closer into this in a follow-up patch?

Thanks for reviewing!(In reply to Eric Carlson from comment #16)
> (In reply to Per Arne Vollan from comment #15)
> > (In reply to Eric Carlson from comment #13)
> > > Comment on attachment 361959 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=361959&action=review
> > > 
> > > > Source/WebCore/html/track/WebVTTParser.cpp:397
> > > > +        auto listSize = selectorList.listSize();
> > > > +        if (!listSize)
> > > > +            return true;
> > > 
> > > Do we want to reject the entire style block if any of the selectors is empty?
> > 
> > That is a good point. I agree that it would be good to allow the valid
> > selectors in a STYLE block. However, I cannot find a method to remove rules
> > from the stylesheet, maybe we can look closer into this in a follow-up patch?
> > 
> Do we need to remove empty selectors? What happens if you just leave it?

I think there should be only one selector (::cue) for each rule. I have updated the patch accordingly.

[Jon Lee]

rdar://problem/48088044

[EWS Watchlist]

Comment on attachment 362045 [details]
Patch

Attachment 362045 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11151241

New failing tests:
inspector/css/modify-inline-style.html

[EWS Watchlist]

Created attachment 362069 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Per Arne Vollan]

(In reply to Eric Carlson from comment #17)
> Comment on attachment 362030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362030&action=review
> 
> > LayoutTests/media/track/captions-webvtt/css-styling.vtt:44
> > +STYLE
> > +::cue {
> > +color: blue
> > +font-size: 25px;
> > +}
> > +video::cue {
> > +color: blue;
> > +font-size: 25px;
> > +}
> 
> The test should have style blocks that contain the invalid rule types
> rejected by WebVTTParser::checkAndStoreStyleSheet (namespace, etc).

Unfortunately, I am not able to apply styles to the '::cue' element by using @namespace or @import rules, possibly because it is a pseudo element. I have filed https://bugs.webkit.org/show_bug.cgi?id=194708 to look closer into this.

[Per Arne Vollan]

Created attachment 362120 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362120 [details]
Patch

Clearing flags on attachment: 362120

Committed r241608: <https://trac.webkit.org/changeset/241608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.