Bug 104043 – Implement general ::cue pseudo element for the <video>

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dima Gorbik]

::cue pseudo element should style all the cues.

[Dima Gorbik]

Created attachment 177561 [details]
Proposed fix 0.1

[Dima Gorbik]

A test case will be added for this.

[Dima Gorbik]

Created attachment 177621 [details]
Proposed fix 0.2

[Eric Carlson]

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.

[Antti Koivisto]

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
> [phone]  Dima Gorbik  <[e-mail]>
> +
> +        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?

[Dima Gorbik]

Created attachment 177639 [details]
Proposed fix 0.3

[Dima Gorbik]

Thanks for the review, Eric!

[Silvia Pfeiffer]

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.

[Dima Gorbik]

(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.

[Victor Carbune]

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)

[Dima Gorbik]

(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.

[Victor Carbune]

(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.

[Dima Gorbik]

(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?

[Eric Carlson]

(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.

[Dima Gorbik]

Created attachment 178141 [details]
Proposed fix 0.4

[Dima Gorbik]

Created attachment 178142 [details]
Proposed fix 0.5

[WebKit Review Bot]

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

[Dima Gorbik]

Created attachment 178268 [details]
Proposed fix 0.6

[WebKit Review Bot]

Comment on attachment 178268 [details]
Proposed fix 0.6

Clearing flags on attachment: 178268

Committed r136991: <http://trac.webkit.org/changeset/136991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/12887273>