Bug 135820 - More adjustments to css width calculations of cue boxes
Summary: More adjustments to css width calculations of cue boxes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-11 16:29 PDT by Roger Fong
Modified: 2014-08-11 17:14 PDT (History)
8 users (show)

See Also:


Attachments
patch (7.71 KB, patch)
2014-08-11 16:37 PDT, Roger Fong
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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