Not all properties apply to the '::cue' pseudo-element

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The WebVTT spec says the following '::cue' properties may be styled:

'color'
'opacity'
'visibility'
'text-decoration'
'text-outline'
'text-shadow'
the properties corresponding to the 'background' shorthand
the properties corresponding to the 'outline' shorthand
the properties corresponding to the 'font' shorthand, including 'line-height'
'white-space'
properties relating to the transition and animation features

Styling at least the 'background' properties don't really work because the element we use for '::cue' is a 0 x 0 <span>, so the background is not rendered. I wonder if “cue” should be what we now call -webkit-media-text-track-all-nodes?

Our layout tests only check some of these properties, we should have tests for all of them.

[Radar WebKit Bug Importer]

<rdar://problem/13281074>

[Dima Gorbik]

This is expected. Specs require background properties to apply to cueBox instead of the cue itself (only for ::cue without an argument, if there is an argument we just skip background properties).
I have few ideas on how to implement this.

[Dima Gorbik]

(In reply to comment #0)
>I wonder if “cue” should be what we now call -webkit-media-text-track-all-nodes?

So the hierarchy is:

-webkit-media-text-track-container (MediaControlTextTrackContainerElement)
| - ::cue (m_cueContainer)
  | - TextTrackCueBox (WebVTT cue background box according to specs)
    |- -webkit-media-text-track-all-nodes (m_allDocumentNodes)

It should be fine to apply all properties to TextTrackCueBox as there are no differences.

[Dima Gorbik]

Created attachment 190185 [details]
Patch

[Eric Carlson]

Comment on attachment 190185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190185&action=review

CaptionUserPreferencesMac::captionsStyleSheetOverride also needs to be updated so the user preference overrides work.

> LayoutTests/media/track/track-css-all-cues.html:21
> +            testExpected("getComputedStyle(textTrackDisplayElement(video, 'cue')).color", "rgb(128, 0, 128)");
> +            testExpected("getComputedStyle(textTrackDisplayElement(video, 'cue')).backgroundColor", "rgb(0, 255, 0)");

We should have tests for all of the white-listed properties.

[Dima Gorbik]

(In reply to comment #5)
> We should have tests for all of the white-listed properties.

I think this should be done separately and for other test (whitelists related probably), track-css-all-cues.html checks if ::cue matches the right container in general. Also it would be good to have DRT enabled for this. I've created a bug report: https://bugs.webkit.org/show_bug.cgi?id=110902

[Dima Gorbik]

Created attachment 190348 [details]
Patch

[Eric Carlson]

Comment on attachment 190348 [details]
Patch

These changes look correct to me, but I don't know enough about StyleResolver to r+ it.

[Dima Gorbik]

Would it be better in terms of performance to save MatchingUARulesScope::isMatchingUARules() result to an intermediate variable or is this being optimized by the compiler itself?

[Antti Koivisto]

Comment on attachment 190348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190348&action=review

> Source/WebCore/html/track/TextTrackCue.cpp:745
> -    m_allDocumentNodes->setPseudo(allNodesShadowPseudoId());
> +    m_allDocumentNodes->setPseudo(cueShadowPseudoId());

m_allDocumentNodes could still use a less confusing name

[Antti Koivisto]

Comment on attachment 190348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190348&action=review

> Source/WebCore/css/StyleResolver.cpp:539
> -        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), matchedRules[i]->propertyWhitelistType());
> +        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), MatchingUARulesScope::isMatchingUARules() ?  PropertyWhitelistNone : matchedRules[i]->propertyWhitelistType());

This seems hackish. Couldn't the propertyWhitelistType() just be correct?

[Antti Koivisto]

Comment on attachment 190348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190348&action=review

>> Source/WebCore/css/StyleResolver.cpp:539
>> +        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), MatchingUARulesScope::isMatchingUARules() ?  PropertyWhitelistNone : matchedRules[i]->propertyWhitelistType());
> 
> This seems hackish. Couldn't the propertyWhitelistType() just be correct?

Extra space after ?

[Dima Gorbik]

(In reply to comment #11)
> (From update of attachment 190348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190348&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:539
> > -        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), matchedRules[i]->propertyWhitelistType());
> > +        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), MatchingUARulesScope::isMatchingUARules() ?  PropertyWhitelistNone : matchedRules[i]->propertyWhitelistType());
> 
> This seems hackish. Couldn't the propertyWhitelistType() just be correct?

This is the earliest possible point to check MatchingUARulesScope. The problem is that RuleSets are being populated (and whitelist type determined) outside of MatchingUARulesScope visibility. I consider this piece of code as customizing matched properties for a particular type of rules (UA in this case).

[Eric Carlson]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 190348 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190348&action=review
> > 
> > > Source/WebCore/css/StyleResolver.cpp:539
> > > -        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), matchedRules[i]->propertyWhitelistType());
> > > +        addMatchedProperties(result, matchedRules[i]->rule()->properties(), matchedRules[i]->rule(), matchedRules[i]->linkMatchType(), MatchingUARulesScope::isMatchingUARules() ?  PropertyWhitelistNone : matchedRules[i]->propertyWhitelistType());
> > 
> > This seems hackish. Couldn't the propertyWhitelistType() just be correct?
> 
> This is the earliest possible point to check MatchingUARulesScope. The problem is that RuleSets are being populated (and whitelist type determined) outside of MatchingUARulesScope visibility. I consider this piece of code as customizing matched properties for a particular type of rules (UA in this case).

Could you pass MatchingUARulesScope::isMatchingUARules() to propertyWhitelistType()?

[Antti Koivisto]

MatchingUARulesScope is out-of-band signaling and shouldn't be used at all if possible. It has only one special case client atm. It think you can just pass the information about RuleData being on a UA sheet in AddRuleFlags and set the whitelist appropriately.

[WebKit Review Bot]

Comment on attachment 190348 [details]
Patch

Attachment 190348 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16807018

New failing tests:
media/track/track-cue-rendering-horizontal.html

[Dima Gorbik]

Created attachment 190403 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190403 [details]
Patch

Attachment 190403 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16794019

New failing tests:
media/track/track-cue-rendering-horizontal.html

[Eric Seidel (no email)]

Comment on attachment 190348 [details]
Patch

Cleared Antti Koivisto's review+ from obsolete attachment 190348 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dima Gorbik]

(In reply to comment #19)
> (From update of attachment 190348 [details])
> Cleared Antti Koivisto's review+ from obsolete attachment 190348 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Actually the last one needs to be landed. Though there are some chromium-specific pixel/layout tests that will need a rebaseline after changes land.

[WebKit Review Bot]

Comment on attachment 190403 [details]
Patch

Rejecting attachment 190403 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 190403, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
Core/html/shadow/MediaControlsGtk.cpp.rej
patching file Source/WebCore/html/track/TextTrackCue.cpp
Hunk #4 FAILED at 741.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/html/track/TextTrackCue.cpp.rej
patching file Source/WebCore/html/track/TextTrackCue.h
patching file Source/WebCore/page/CaptionUserPreferencesMac.mm

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Eric Carlson']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/16987181

[Dima Gorbik]

Created attachment 191842 [details]
Patch

[WebKit Review Bot]

Comment on attachment 191842 [details]
Patch

Attachment 191842 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17001110

New failing tests:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

[WebKit Review Bot]

Comment on attachment 191842 [details]
Patch

Attachment 191842 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17060068

New failing tests:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

[Dima Gorbik]

Committed r145397 (http://trac.webkit.org/changeset/145397)