Bug 110705 - Not all properties apply to the '::cue' pseudo-element
Summary: Not all properties apply to the '::cue' pseudo-element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dima Gorbik
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-23 20:27 PST by Eric Carlson
Modified: 2013-03-11 13:47 PDT (History)
13 users (show)

See Also:


Attachments
Patch (17.66 KB, patch)
2013-02-25 19:41 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Patch (18.56 KB, patch)
2013-02-26 13:12 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Patch (27.58 KB, patch)
2013-02-26 17:41 PST, Dima Gorbik
eric.carlson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (27.61 KB, patch)
2013-03-06 15:06 PST, Dima Gorbik
eric.carlson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-02-23 20:27:07 PST
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.
Comment 1 Radar WebKit Bug Importer 2013-02-23 20:27:37 PST
<rdar://problem/13281074>
Comment 2 Dima Gorbik 2013-02-25 13:34:28 PST
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.
Comment 3 Dima Gorbik 2013-02-25 14:42:49 PST
(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.
Comment 4 Dima Gorbik 2013-02-25 19:41:57 PST
Created attachment 190185 [details]
Patch
Comment 5 Eric Carlson 2013-02-26 06:05:34 PST
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.
Comment 6 Dima Gorbik 2013-02-26 13:01:33 PST
(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
Comment 7 Dima Gorbik 2013-02-26 13:12:02 PST
Created attachment 190348 [details]
Patch
Comment 8 Eric Carlson 2013-02-26 13:28:39 PST
Comment on attachment 190348 [details]
Patch

These changes look correct to me, but I don't know enough about StyleResolver to r+ it.
Comment 9 Dima Gorbik 2013-02-26 13:51:14 PST
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?
Comment 10 Antti Koivisto 2013-02-26 13:55:29 PST
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
Comment 11 Antti Koivisto 2013-02-26 13:57:24 PST
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?
Comment 12 Antti Koivisto 2013-02-26 13:59:14 PST
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 ?
Comment 13 Dima Gorbik 2013-02-26 14:27:16 PST
(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).
Comment 14 Eric Carlson 2013-02-26 14:38:32 PST
(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()?
Comment 15 Antti Koivisto 2013-02-26 14:56:29 PST
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.
Comment 16 WebKit Review Bot 2013-02-26 16:20:20 PST
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
Comment 17 Dima Gorbik 2013-02-26 17:41:50 PST
Created attachment 190403 [details]
Patch
Comment 18 WebKit Review Bot 2013-02-26 19:47:53 PST
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
Comment 19 Eric Seidel (no email) 2013-03-01 10:37:33 PST
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.
Comment 20 Dima Gorbik 2013-03-01 13:27:32 PST
(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.
Comment 21 WebKit Review Bot 2013-03-06 13:58:27 PST
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
Comment 22 Dima Gorbik 2013-03-06 15:06:21 PST
Created attachment 191842 [details]
Patch
Comment 23 WebKit Review Bot 2013-03-06 16:34:07 PST
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
Comment 24 WebKit Review Bot 2013-03-06 17:07:58 PST
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
Comment 25 Dima Gorbik 2013-03-11 13:47:16 PDT
Committed r145397 (http://trac.webkit.org/changeset/145397)