<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
Created attachment 236418 [details] patch
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.
(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
although all the EWS bots failed?...
Comment on attachment 236418 [details] patch I think EWS was down for some time yesterday?
(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.
landed: http://trac.webkit.org/changeset/172481