Bug 135823

Summary: Adjust max-width of cues based on text alignment when cue size is expanded
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, eric.carlson, roger_fong
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch bfulgham: review+

Description Roger Fong 2014-08-11 17:40:58 PDT
<rdar://problem/17954473>

The maxwidth calculations for cues right now do not take into account alignment.

All units are in percentages.
If we are left aligned the max cue width is the 100-the cue position
If we are right aligned the max cue width is the cue position
If we are centered the max cue width is just 100
Comment 1 Roger Fong 2014-08-11 17:44:57 PDT
Created attachment 236418 [details]
patch
Comment 2 Brent Fulgham 2014-08-11 18:09:21 PDT
Comment on attachment 236418 [details]
patch

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

This is really close, but we have to take RTL languages into account.  Take a look at "textAlignResolvingStartAndEnd" in WebCore/editing/EditingStyle.cpp for a snippet that shows what we  need here.

r- to fix the RTL handling, but otherwise it looks great!

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:107
> +    if (alignment == CSSValueEnd || alignment == CSSValueRight)

CSSValueEnd is only the same as CSSValueRight in Left-to-Right contexts I think we need to do something where we check for RTL, and set alignment to left/right as needed.

> Source/WebCore/html/track/VTTCue.cpp:184
> +    if (alignment == CSSValueEnd || alignment == CSSValueRight)

Ditto my earlier comment about Start/End.
Comment 3 Roger Fong 2014-08-12 11:27:48 PDT
(In reply to comment #2)
> (From update of attachment 236418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236418&action=review
> 
> This is really close, but we have to take RTL languages into account.  Take a look at "textAlignResolvingStartAndEnd" in WebCore/editing/EditingStyle.cpp for a snippet that shows what we  need here.
> 
> r- to fix the RTL handling, but otherwise it looks great!
> 
> > Source/WebCore/html/track/TextTrackCueGeneric.cpp:107
> > +    if (alignment == CSSValueEnd || alignment == CSSValueRight)
> 
> CSSValueEnd is only the same as CSSValueRight in Left-to-Right contexts I think we need to do something where we check for RTL, and set alignment to left/right as needed.
> 
> > Source/WebCore/html/track/VTTCue.cpp:184
> > +    if (alignment == CSSValueEnd || alignment == CSSValueRight)
> 
> Ditto my earlier comment about Start/End.

So apparently, with RTL text, the text position is also reversed!
So a position of say, 80% actually is 80% from the right side of the video, not the left.
Thus the original calculations end up still being right!
Only managed to find this out after I had made the changes to handle RTL text...
So I think we're actually good with the first patch
Comment 4 Roger Fong 2014-08-12 11:28:15 PDT
although all the EWS bots failed?...
Comment 5 Roger Fong 2014-08-12 11:28:44 PDT
Comment on attachment 236418 [details]
patch

I think EWS was down for some time yesterday?
Comment 6 Brent Fulgham 2014-08-12 11:31:02 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 236418 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236418&action=review
> > 
> > This is really close, but we have to take RTL languages into account.  Take a look at "textAlignResolvingStartAndEnd" in WebCore/editing/EditingStyle.cpp for a snippet that shows what we  need here.
> > 
> > r- to fix the RTL handling, but otherwise it looks great!
> > 
> > > Source/WebCore/html/track/TextTrackCueGeneric.cpp:107
> > > +    if (alignment == CSSValueEnd || alignment == CSSValueRight)
> > 
> > CSSValueEnd is only the same as CSSValueRight in Left-to-Right contexts I think we need to do something where we check for RTL, and set alignment to left/right as needed.
> > 
> > > Source/WebCore/html/track/VTTCue.cpp:184
> > > +    if (alignment == CSSValueEnd || alignment == CSSValueRight)
> > 
> > Ditto my earlier comment about Start/End.
> 
> So apparently, with RTL text, the text position is also reversed!
> So a position of say, 80% actually is 80% from the right side of the video, not the left.
> Thus the original calculations end up still being right!
> Only managed to find this out after I had made the changes to handle RTL text...
> So I think we're actually good with the first patch

Awesome!  We'll just keep an eye on things when the patch lands, and fix any broken tests at that point.
Comment 7 Roger Fong 2014-08-12 12:21:20 PDT
landed: http://trac.webkit.org/changeset/172481