Bug 104043

Summary: Implement general ::cue pseudo element for the <video>
Product: WebKit Reporter: Dima Gorbik <dgorbik>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, dglazkov, eric.carlson, feature-media-reviews, koivisto, macpherson, menard, ojan.autocc, silviapf, silviapfeiffer1, vcarbune, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed fix 0.1
none
Proposed fix 0.2
eric.carlson: review+
Proposed fix 0.3
eric.carlson: review+
Proposed fix 0.4
none
Proposed fix 0.5
webkit.review.bot: commit-queue-
Proposed fix 0.6 none

Description Dima Gorbik 2012-12-04 14:14:10 PST
::cue pseudo element should style all the cues.
Comment 1 Dima Gorbik 2012-12-04 14:23:02 PST
Created attachment 177561 [details]
Proposed fix 0.1
Comment 2 Dima Gorbik 2012-12-04 14:25:56 PST
A test case will be added for this.
Comment 3 Dima Gorbik 2012-12-04 17:35:19 PST
Created attachment 177621 [details]
Proposed fix 0.2
Comment 4 Eric Carlson 2012-12-04 17:51:49 PST
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 5 Antti Koivisto 2012-12-04 17:56:00 PST
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?
Comment 6 Dima Gorbik 2012-12-04 18:29:13 PST
Created attachment 177639 [details]
Proposed fix 0.3
Comment 7 Dima Gorbik 2012-12-05 17:10:30 PST
Thanks for the review, Eric!
Comment 8 Silvia Pfeiffer 2012-12-05 17:16:58 PST
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.
Comment 9 Dima Gorbik 2012-12-05 17:23:11 PST
(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 10 Victor Carbune 2012-12-05 17:33:18 PST
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)
Comment 11 Dima Gorbik 2012-12-05 17:40:08 PST
(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.
Comment 12 Victor Carbune 2012-12-05 17:59:11 PST
(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.
Comment 13 Dima Gorbik 2012-12-05 19:24:48 PST
(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?
Comment 14 Eric Carlson 2012-12-05 19:31:24 PST
(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.
Comment 15 Dima Gorbik 2012-12-06 20:01:11 PST
Created attachment 178141 [details]
Proposed fix 0.4
Comment 16 Dima Gorbik 2012-12-06 20:03:45 PST
Created attachment 178142 [details]
Proposed fix 0.5
Comment 17 WebKit Review Bot 2012-12-06 23:57:38 PST
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
Comment 18 Dima Gorbik 2012-12-07 13:58:22 PST
Created attachment 178268 [details]
Proposed fix 0.6
Comment 19 WebKit Review Bot 2012-12-07 15:11:59 PST
Comment on attachment 178268 [details]
Proposed fix 0.6

Clearing flags on attachment: 178268

Committed r136991: <http://trac.webkit.org/changeset/136991>
Comment 20 WebKit Review Bot 2012-12-07 15:12:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2012-12-14 16:17:37 PST
<rdar://problem/12887273>