Summary: | [WebVTT] Inline WebVTT styles should start with '::cue' | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, eric.carlson, ews-watchlist, jonlee, koivisto, rniwa, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2019-02-04 10:46:26 PST
Created attachment 361081 [details]
Patch
Created attachment 361083 [details]
Patch
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? Created attachment 361514 [details]
Patch
(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! (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. Created attachment 361516 [details]
Patch
(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 on attachment 361516 [details] Patch Clearing flags on attachment: 361516 Committed r241203: <https://trac.webkit.org/changeset/241203> Created attachment 361956 [details]
Patch
Created attachment 361959 [details]
Patch
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? Created attachment 362030 [details]
Patch
(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 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 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). Created attachment 362045 [details]
Patch
(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 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 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
(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. Created attachment 362120 [details]
Patch
Comment on attachment 362120 [details] Patch Clearing flags on attachment: 362120 Committed r241608: <https://trac.webkit.org/changeset/241608> All reviewed patches have been landed. Closing bug. |