Bug 194227 - [WebVTT] Inline WebVTT styles should start with '::cue'
Summary: [WebVTT] Inline WebVTT styles should start with '::cue'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh All
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-04 10:46 PST by Per Arne Vollan
Modified: 2019-02-15 13:13 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.61 KB, patch)
2019-02-04 11:56 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2019-02-04 11:58 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2019-02-08 10:07 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2019-02-08 10:52 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2019-02-13 16:23 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2019-02-13 16:32 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2019-02-14 09:51 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2019-02-14 12:12 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.86 MB, application/zip)
2019-02-14 15:23 PST, Build Bot
no flags Details
Patch (4.53 KB, patch)
2019-02-15 09:45 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-02-04 10:46:26 PST
Inline WebVTT starting with e.g. 'video::cue' should not be allowed.
Comment 1 Radar WebKit Bug Importer 2019-02-04 10:47:03 PST
<rdar://problem/47791087>
Comment 2 Per Arne Vollan 2019-02-04 11:56:37 PST
Created attachment 361081 [details]
Patch
Comment 3 Per Arne Vollan 2019-02-04 11:58:01 PST
Created attachment 361083 [details]
Patch
Comment 4 Brent Fulgham 2019-02-08 09:14:16 PST
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?
Comment 5 Per Arne Vollan 2019-02-08 10:07:14 PST
Created attachment 361514 [details]
Patch
Comment 6 Per Arne Vollan 2019-02-08 10:11:19 PST
(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!
Comment 7 Eric Carlson 2019-02-08 10:26:12 PST
(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.
Comment 8 Per Arne Vollan 2019-02-08 10:52:31 PST
Created attachment 361516 [details]
Patch
Comment 9 Per Arne Vollan 2019-02-08 10:54:28 PST
(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!
Comment 10 WebKit Commit Bot 2019-02-08 11:48:10 PST
Comment on attachment 361516 [details]
Patch

Clearing flags on attachment: 361516

Committed r241203: <https://trac.webkit.org/changeset/241203>
Comment 11 Per Arne Vollan 2019-02-13 16:23:24 PST
Created attachment 361956 [details]
Patch
Comment 12 Per Arne Vollan 2019-02-13 16:32:20 PST
Created attachment 361959 [details]
Patch
Comment 13 Eric Carlson 2019-02-13 18:12:15 PST
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?
Comment 14 Per Arne Vollan 2019-02-14 09:51:20 PST
Created attachment 362030 [details]
Patch
Comment 15 Per Arne Vollan 2019-02-14 09:55:01 PST
(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!
Comment 16 Eric Carlson 2019-02-14 11:01:11 PST
(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?
Comment 17 Eric Carlson 2019-02-14 11:03:27 PST
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).
Comment 18 Per Arne Vollan 2019-02-14 12:12:36 PST
Created attachment 362045 [details]
Patch
Comment 19 Per Arne Vollan 2019-02-14 12:15:00 PST
(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.
Comment 20 Jon Lee 2019-02-14 14:19:54 PST
rdar://problem/48088044
Comment 21 Build Bot 2019-02-14 15:23:12 PST
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
Comment 22 Build Bot 2019-02-14 15:23:14 PST
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
Comment 23 Per Arne Vollan 2019-02-15 09:35:55 PST
(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.
Comment 24 Per Arne Vollan 2019-02-15 09:45:31 PST
Created attachment 362120 [details]
Patch
Comment 25 WebKit Commit Bot 2019-02-15 13:13:12 PST
Comment on attachment 362120 [details]
Patch

Clearing flags on attachment: 362120

Committed r241608: <https://trac.webkit.org/changeset/241608>
Comment 26 WebKit Commit Bot 2019-02-15 13:13:14 PST
All reviewed patches have been landed.  Closing bug.