Bug 110703 – Support padding, margin and border for internal UA cue styling

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

[Eric Carlson]

Only the first line of a default positioned, multi-line cue is visible because the cue is placed too far down the page. I noticed this in the styling demo at http://www.longtailvideo.com/html5/webvtt/, where the cue at 46 seconds is:

00:00:46.000 --> 00:00:50.000
<c.myclass>This cue contains the class "myclass". 
Browsers that support ::cue CSS should make it red.</c>

but only 'This cue contains the class "myclass". ' is visible.

[Radar WebKit Bug Importer]

<rdar://problem/13281078>

[Eric Carlson]

This works correctly with cues with default styles, but if the TextTrackCueBox element (-webkit-media-text-track-display) has style that increases it size, eg. -webkit-border-radius, the code that calculates the position is incorrect.

[Victor Carbune]

Created attachment 189966 [details]
Screenshots

(In reply to comment #2)
> This works correctly with cues with default styles, but if the TextTrackCueBox element (-webkit-media-text-track-display) has style that increases it size, eg. -webkit-border-radius, the code that calculates the position is incorrect.

I can't reproduce this properly, meaning that whatever I change I still get both
lines displayed. I attached the screenshots.

* Adding "font-size: 150%" to ::cue(.myclass) (works properly)
* Adding "font-size: 150%" to -webkit-media-text-track-display (overlaps backgrounds)
* Setting -webkit-border-radius on -webkit-media-text-tracks-all-nodes doesn't 
generate any visible differences (other than the cue background corners)

It looks to me like the way a developer should change the font-size works (through ::cue),
but it indeed looks like a bit of a problem for internally increasing the font-size of
the cue.

[Eric Carlson]

Setting 'padding' on -webkit-media-text-track-display causes the problem I am seeing. Try adding this to a page with a <track>:

        video::-webkit-media-text-track-display {
            background-color:rgba(0, 0, 0, 0.5);
            padding: .4em !important;
        }

I suspect that other CSS properties will also cause problems.

[Victor Carbune]

(In reply to comment #4)
> Setting 'padding' on -webkit-media-text-track-display causes the problem I am seeing. Try adding this to a page with a <track>:

Thanks! Took me a while to understand this (fairly simple) problem - it lies in
RenderTextTrackCue::isOutside(), because padding (or border, margin), increases
both horizontal and vertical sizes of the cue (thus the width is 100% +
padding pixels) and isOutside always returns true with such properties.

Due to the nature of the algorithm, if we allow padding and check only horizontal 
fitting in the parent container, the cue will be shifted upwards with multiples of line height (depending on the exact extra size besides the cue lines).

Given that padding isn't really supported as VTT selector property, but we can
set it via the pseudo-id, is the above described fix good enough? If there's a
specific use-case for this, maybe a bug should be filed against the spec.

[Eric Carlson]

(In reply to comment #5)
> (In reply to comment #4)
> > Setting 'padding' on -webkit-media-text-track-display causes the problem I am seeing. Try adding this to a page with a <track>:
> 
> Thanks! Took me a while to understand this (fairly simple) problem - it lies in
> RenderTextTrackCue::isOutside(), because padding (or border, margin), increases
> both horizontal and vertical sizes of the cue (thus the width is 100% +
> padding pixels) and isOutside always returns true with such properties.
> 
> Due to the nature of the algorithm, if we allow padding and check only horizontal 
> fitting in the parent container, the cue will be shifted upwards with multiples of line height (depending on the exact extra size besides the cue lines).
> 
> Given that padding isn't really supported as VTT selector property, but we can
> set it via the pseudo-id, is the above described fix good enough? If there's a
> specific use-case for this, maybe a bug should be filed against the spec.

I think the only use case is for UA specific styling. I found this because I am using 'padding' to make the "window" around the cue text (-webkit-media-text-track-display ) bigger to match a user style. See CaptionUserPreferencesMac::captionsStyleSheetOverride. 

I agree that we don't need to pages overriding private styles, so I don't this we need to request a spec change, but if we can support this without an enormous amount of work it will allow us to be more flexible internally.

[Victor Carbune]

Created attachment 190795 [details]
Patch

I think it's the simplest way to accomodate this

[Eric Carlson]

Comment on attachment 190795 [details]
Patch

Thank you Victor!

[WebKit Review Bot]

Comment on attachment 190795 [details]
Patch

Clearing flags on attachment: 190795

Committed r144443: <http://trac.webkit.org/changeset/144443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111169

[Victor Carbune]

Created attachment 191003 [details]
Resubmit patch

The failing tests leading to the rollback aren't related to the changes

[WebKit Review Bot]

Comment on attachment 191003 [details]
Resubmit patch

Clearing flags on attachment: 191003

Committed r144508: <http://trac.webkit.org/changeset/144508>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.