Bug 134821 - Use a separate backdrop element to allow cues to have highlight and background color
Summary: Use a separate backdrop element to allow cues to have highlight and backgroun...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-10 18:40 PDT by Brent Fulgham
Modified: 2014-07-16 09:32 PDT (History)
17 users (show)

See Also:


Attachments
Patch (9.60 KB, patch)
2014-07-10 21:05 PDT, Brent Fulgham
eric.carlson: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (610.33 KB, application/zip)
2014-07-10 22:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (583.31 KB, application/zip)
2014-07-11 06:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-07-10 18:40:45 PDT
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.
Comment 1 Brent Fulgham 2014-07-10 18:41:06 PDT
<rdar://problem/15999721>
Comment 2 Brent Fulgham 2014-07-10 21:05:15 PDT
Created attachment 234743 [details]
Patch
Comment 3 Build Bot 2014-07-10 22:35:15 PDT
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
Comment 4 Build Bot 2014-07-10 22:35:20 PDT
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
Comment 5 Build Bot 2014-07-11 06:04:26 PDT
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
Comment 6 Build Bot 2014-07-11 06:04:33 PDT
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
Comment 7 Eric Carlson 2014-07-11 09:01:14 PDT
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"
Comment 8 Brent Fulgham 2014-07-11 09:31:20 PDT
(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.
Comment 9 Brent Fulgham 2014-07-11 10:08:04 PDT
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.
Comment 10 Brent Fulgham 2014-07-11 10:12:09 PDT
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!
Comment 11 Brent Fulgham 2014-07-11 10:16:30 PDT
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.
Comment 12 Brent Fulgham 2014-07-11 10:32:43 PDT
Committed r171004: <http://trac.webkit.org/changeset/171004>
Comment 13 Carlos Alberto Lopez Perez 2014-07-16 09:32:52 PDT
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