Bug 135820

Summary: More adjustments to css width calculations of cue boxes
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, gyuyoung.kim, philipj, roger_fong
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch bfulgham: review+, bfulgham: commit-queue-

Description Roger Fong 2014-08-11 16:29:51 PDT
<rdar://problem/17961164>

Two more issues I neglected to fix:

First off the multiplier calculations are wrong since they are based off a default font size of 10px which is not actually what we use. We use a percentage of the video size as specified in CaptionUserPreferences.

The second issue is that for center aligned text, the cue box is offset from center after adjusting the width.
This is because when we specify center alignment, what happens is that we actually calculate the left/top css value such that the cue looks centered.
Thus we also need to adjust the left/top property value (by the difference in width / 2) if we're not centered such that the box remained in the center.
Comment 1 Roger Fong 2014-08-11 16:37:25 PDT
Created attachment 236413 [details]
patch
Comment 2 Brent Fulgham 2014-08-11 16:59:49 PDT
Comment on attachment 236413 [details]
patch

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

r=me, but please address the possible zero-base-font-size issue I mentioned.

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:82
> +        double authorFontSize = videoSize.height() * cue->baseFontSizeRelativeToVideoHeight() / 100.0;

Can baseFontSizeRelativeToVideoHeight ever be zero (I think it can be)? If the author could arbitrarily set this to zero, I think we should check and fall back to that minimum (10 pt) value from the CSS.
Comment 3 Roger Fong 2014-08-11 17:14:39 PDT
committed with review commented applied:
http://trac.webkit.org/changeset/172421