Bug 134821 – Use a separate backdrop element to allow cues to have highlight and background color

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

To achieve a specific style goal, we want to be able to have cues with a background color plus a highlight color. To do so, we need to add a <div> around the cue's <span> element so that the <span> background can contain the highlight color, and the <div> wrapping the <span> can hold the desired background color.

[Brent Fulgham]

<rdar://problem/15999721>

[Brent Fulgham]

Created attachment 234743 [details]
Patch

[Build Bot]

Comment on attachment 234743 [details]
Patch

Attachment 234743 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5446361667338240

New failing tests:
media/track/track-cue-rendering-snap-to-lines-not-set.html
media/track/track-cue-rendering-horizontal.html

[Build Bot]

Created attachment 234745 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234743 [details]
Patch

Attachment 234743 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6455020509724672

New failing tests:
media/track/track-cue-rendering-snap-to-lines-not-set.html
media/track/track-cue-rendering-horizontal.html

[Build Bot]

Created attachment 234761 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Eric Carlson]

Comment on attachment 234743 [details]
Patch

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

It looks like a couple of test results need to be updated.

Thanks!

> Source/WebCore/rendering/RenderVTTCue.cpp:343
> +    // First child is our wrapping <div>. We need to get ITS first child.

Nit: "We need to get ITS" -> "The cue object is the <div>'s"

[Brent Fulgham]

(In reply to comment #5)
> (From update of attachment 234743 [details])
> Attachment 234743 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/6455020509724672
> 
> New failing tests:
> media/track/track-cue-rendering-snap-to-lines-not-set.html
> media/track/track-cue-rendering-horizontal.html

The change to 'track-cue-rendering-snap-to-lines-not-set.html" seems like a progression. The difference here is that "Random Cue 2" appears above the "You may even have multiple cues in the same time.", allowing "Random Cue 3" to move higher on the page. Since "Random Cue 2" was encoded to be above the center of the video frame, the fact that it now appears above the "You may .... time" is correct.

[Brent Fulgham]

Comment on attachment 234743 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:734
> +    -webkit-box-sizing: border-box;

These two lines break the tests because we do no want this added padding when using the W3C caption style. In fact, the padding isn't even needed since a 0.4em pad is added when we set background color, so I can just remove these.

[Brent Fulgham]

Comment on attachment 234743 [details]
Patch

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

Landing with fixes based on Eric's review.

>> Source/WebCore/rendering/RenderVTTCue.cpp:343
>> +    // First child is our wrapping <div>. We need to get ITS first child.
> 
> Nit: "We need to get ITS" -> "The cue object is the <div>'s"

Fixed!

[Brent Fulgham]

Comment on attachment 234743 [details]
Patch

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

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:734
>> +    -webkit-box-sizing: border-box;
> 
> These two lines break the tests because we do no want this added padding when using the W3C caption style. In fact, the padding isn't even needed since a 0.4em pad is added when we set background color, so I can just remove these.

Also, this same code needs to be in mediaControlsiOS.css.

[Brent Fulgham]

Committed r171004: <http://trac.webkit.org/changeset/171004>

[Carlos Alberto Lopez Perez]

This caused 6 Layout test on media/track/track* fail on the GTK port. Reported here: https://bugs.webkit.org/show_bug.cgi?id=134979