Bug 111262 - Cue line-height property shouldn't be inherited from the video element
Summary: Cue line-height property shouldn't be inherited from the video 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: Victor Carbune
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-03 00:37 PST by Victor Carbune
Modified: 2013-03-05 14:04 PST (History)
11 users (show)

See Also:


Attachments
Fix (5.40 KB, patch)
2013-03-05 13:27 PST, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2013-03-03 00:37:26 PST
This can be reproduced with any captioned video.
Comment 1 Radar WebKit Bug Importer 2013-03-04 07:47:58 PST
<rdar://problem/13336907>
Comment 2 Victor Carbune 2013-03-04 13:28:47 PST
It looks like the problem appear each time the video is scaled (not neccessarily
fullscreen, e.g. through height and width properties)

Before I start digging even more into this, it looks now that WebKit supports
viewport values (such as the 5vh which is the default for cues) and also using
this when scaling the video seems to fix a part of the problem (no overlapping 
lines)

Firstly, I'm thinking of dropping the manual CSS font-size adjustment of cues 
that happens in TextTrackContainer::updateSizes(). Is there anything that might
block this?
Comment 3 Eric Carlson 2013-03-04 13:48:08 PST
(In reply to comment #2)
> It looks like the problem appear each time the video is scaled (not neccessarily
> fullscreen, e.g. through height and width properties)
> 
> Before I start digging even more into this, it looks now that WebKit supports
> viewport values (such as the 5vh which is the default for cues) and also using
> this when scaling the video seems to fix a part of the problem (no overlapping 
> lines)
> 
> Firstly, I'm thinking of dropping the manual CSS font-size adjustment of cues 
> that happens in TextTrackContainer::updateSizes(). Is there anything that might
> block this?

It is also possible for user preferences to override font size, eg. CaptionUserPreferencesMac::captionFontSizeScale, but there is no reason that we couldn't change the size by adding a font size override to the style sheet generated by CaptionUserPreferencesMac::captionsStyleSheetOverride. 

This would actually be simpler because it could be done only when the user preferences change (as we do with the other overrides) instead of every time the size changes.

Note, I originally tried to use vh but it did not work
Comment 4 Victor Carbune 2013-03-05 13:20:48 PST
(In reply to comment #3)
> Note, I originally tried to use vh but it did not work
You are right, it still doesn't work correctly.

Further debugging shows that this doesn't happen all the time, but particularly
when the line-height on the video element is inherited from containing blocks.

I think this is a problem with the shadow tree, and such properties shouldn't
be inherited (I'm not sure if there's a bug already for this).
Comment 5 Victor Carbune 2013-03-05 13:27:56 PST
Created attachment 191549 [details]
Fix
Comment 6 Eric Carlson 2013-03-05 13:30:38 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Note, I originally tried to use vh but it did not work
> You are right, it still doesn't work correctly.
> 
  Darn :-(

> Further debugging shows that this doesn't happen all the time, but particularly
> when the line-height on the video element is inherited from containing blocks.
> 
> I think this is a problem with the shadow tree, and such properties shouldn't
> be inherited (I'm not sure if there's a bug already for this).
>
  I agree that this looks like a shadow tree problem, the existing code that sets inline style sometimes works and sometimes does not.
Comment 7 WebKit Review Bot 2013-03-05 14:04:29 PST
Comment on attachment 191549 [details]
Fix

Clearing flags on attachment: 191549

Committed r144814: <http://trac.webkit.org/changeset/144814>
Comment 8 WebKit Review Bot 2013-03-05 14:04:34 PST
All reviewed patches have been landed.  Closing bug.