Bug 104043

Created attachment 177561 [details] Proposed fix 0.1

A test case will be added for this.

Created attachment 177621 [details] Proposed fix 0.2

Comment on attachment 177621 [details] Proposed fix 0.2 View in context: https://bugs.webkit.org/attachment.cgi?id=177621&action=review > LayoutTests/media/track/track-css-all-cues.html:14 > + <style> > + > + video::cue { color:red } > + > + </style> It looks like you have a mixture of spaces and tabs in this file. Spaces only please. > Source/WebCore/ChangeLog:13 > + (WebCore): This doesn't add anything and can be removed. > Source/WebCore/ChangeLog:15 > + (WebCore::MediaControlTextTrackContainerElement::createSubtrees): > + (WebCore::MediaControlTextTrackContainerElement::updateDisplay): I prefer to see a comment about what changed in each method. Not everyone does this, but I think it makes it easier for people looking for information about changes later. > Source/WebCore/ChangeLog:19 > + (WebCore::MediaControlRootElement::createTextTrackDisplay): Ditto.

Comment on attachment 177621 [details] Proposed fix 0.2 View in context: https://bugs.webkit.org/attachment.cgi?id=177621&action=review Looks good except for the ChangeLog. > Source/WebCore/ChangeLog:10 > +2012-12-04 Dima Gorbik <dgorbik@apple.com> > + > + Implement general ::cue pseudo element for the <video> > + https://bugs.webkit.org/show_bug.cgi?id=104043 > + > + Reviewed by NOBODY (OOPS!). > + > + Implemented the ::cue pseudo element to be able to style all WebVTT cues. > + > + Test: media/track/track-css-all-cues.html Please make the ChangeLog bit more descriptive. A pointer to the relevant standard section would be good. What part is exactly implemented by this patch and what is still left to implement? What is the plan going forward? >> Source/WebCore/ChangeLog:19 >> + * html/shadow/MediaControlElements.cpp: >> + (WebCore): >> + (WebCore::MediaControlTextTrackContainerElement::createSubtrees): >> + (WebCore::MediaControlTextTrackContainerElement::updateDisplay): >> + * html/shadow/MediaControlElements.h: >> + (MediaControlTextTrackContainerElement): >> + * html/shadow/MediaControlRootElement.cpp: >> + (WebCore::MediaControlRootElement::createTextTrackDisplay): > > Ditto. Some implementation comments here would be good too. Why is cueContainer subtree needed?

Created attachment 177639 [details] Proposed fix 0.3

Thanks for the review, Eric!

Code looks good to me. Very glad that this is happening! Can I suggest adding a layout test result for your platform and the appropriate changes to test expectation files? It would be nice to see the results of the ::cue CSS. Also note that you'll need to rebase on the latest codebase, because the media controls code has been refactored.

(In reply to comment #8) > Code looks good to me. Very glad that this is happening! > > Can I suggest adding a layout test result for your platform and the appropriate changes to test expectation files? It would be nice to see the results of the ::cue CSS. > > Also note that you'll need to rebase on the latest codebase, because the media controls code has been refactored. That sounds like a good idea. May I add those tests a little bit later since I am rolling out other patches related to the ::cue now? (and they will most likely change the shadow tree). Also I am expecting specs for the ::cue to change a little bit really soon.

Comment on attachment 177639 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=177639&action=review > LayoutTests/ChangeLog:3 > + Implement general ::cue pseudo element for the <video> Would you have thoughts / knowledge about parsing / filter so that only some CSS properties can be applied? > LayoutTests/media/track/track-css-all-cues-expected.txt:4 > +EXPECTED (getComputedStyle(textTrackDisplayElement(video).firstChild).color == 'rgb(255, 0, 0)') OK Just nice to have would be tests for each allowed CSS properties mentioned in the spec. > Source/WebCore/html/shadow/MediaControlElements.cpp:1307 > + m_cueContainer = HTMLElement::create(spanTag, document); Couldn't the same be achieved by overriding shadowPseudoId() in TextTrackCueBox? It would avoid creating a container within a container and it would match individually each list of WebVTT node objects. > Source/WebCore/html/shadow/MediaControlElements.cpp:1308 > + m_cueContainer->setShadowPseudoId(cue); As Silvia mentioned you probably need to sync, as I think this should be setPseudo. (looks to me that setShadowPseudoId was removed in http://trac.webkit.org/changeset/133749)

(In reply to comment #10) > (From update of attachment 177639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177639&action=review > > > LayoutTests/ChangeLog:3 > > + Implement general ::cue pseudo element for the <video> > > Would you have thoughts / knowledge about parsing / filter so that only some CSS properties can be applied? Yes, that was implemented and I will roll out the patch in a separate bug. > Couldn't the same be achieved by overriding shadowPseudoId() in TextTrackCueBox? > > It would avoid creating a container within a container and it would match > individually each list of WebVTT node objects. I guess it is not possible to have several pseudoIDs for the same element. And I can't use the same pseudoID because the default properties may be not in the whitelist.

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 177639 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=177639&action=review > > > > > LayoutTests/ChangeLog:3 > > > + Implement general ::cue pseudo element for the <video> > > > > Would you have thoughts / knowledge about parsing / filter so that only some CSS properties can be applied? > > Yes, that was implemented and I will roll out the patch in a separate bug. Nice! > > Couldn't the same be achieved by overriding shadowPseudoId() in TextTrackCueBox? > > > > It would avoid creating a container within a container and it would match > > individually each list of WebVTT node objects. > > I guess it is not possible to have several pseudoIDs for the same element. And I can't use the same pseudoID because the default properties may be not in the whitelist. Oups, sorry for the rush. You're right.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > I guess it is not possible to have several pseudoIDs for the same element. And I can't use the same pseudoID because the default properties may be not in the whitelist. > Oups, sorry for the rush. You're right. There is still a possibility to use ::cue for both cases, and we could check where the ruleset comes from — from the user agent stylesheet or the author stylesheet. In this case we have to get rid of the -text-track prefixed pseudoID which may be hard. Is it already been used by someone?

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > I guess it is not possible to have several pseudoIDs for the same element. And I can't use the same pseudoID because the default properties may be not in the whitelist. > > Oups, sorry for the rush. You're right. > > There is still a possibility to use ::cue for both cases, and we could check where the ruleset comes from — from the user agent stylesheet or the author stylesheet. In this case we have to get rid of the -text-track prefixed pseudoID which may be hard. Is it already been used by someone? The pseudoID is an implementation detail and should only be used inside of WebKit, so we can change it as needed.

Created attachment 178141 [details] Proposed fix 0.4

Created attachment 178142 [details] Proposed fix 0.5

Comment on attachment 178142 [details] Proposed fix 0.5 Attachment 178142 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15187193 New failing tests: media/track/track-cues-seeking.html media/track/track-mode.html media/track/track-cue-rendering-inner-timestamps.html media/track/track-cues-pause-on-exit.html media/track/track-cue-mutable-fragment.html media/track/track-cues-enter-exit.html media/track/track-cue-mutable-text.html media/track/track-cue-rendering.html media/track/track-cues-sorted-before-dispatch.html media/track/track-active-cues.html media/track/track-cues-missed.html media/track/track-cue-nothing-to-render.html media/track/track-css-all-cues.html media/track/track-mode-disabled-crash.html media/video-controls-captions.html media/track/track-cue-container-rendering-position.html media/track/track-kind.html media/track/track-cues-cuechange.html media/track/track-mode-not-changed-by-new-track.html

Created attachment 178268 [details] Proposed fix 0.6

Comment on attachment 178268 [details] Proposed fix 0.6 Clearing flags on attachment: 178268 Committed r136991: <http://trac.webkit.org/changeset/136991>

All reviewed patches have been landed. Closing bug.

<rdar://problem/12887273>