135823 2014-08-11 17:40:58 -0700 Adjust max-width of cues based on text alignment when cue size is expanded 2014-08-12 12:21:20 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 roger_fong webkit-unassigned bfulgham eric.carlson roger_fong oldest_to_newest 1028374 0 roger_fong 2014-08-11 17:40:58 -0700 <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 1028377 1 236418 roger_fong 2014-08-11 17:44:57 -0700 Created attachment 236418 patch 1028386 2 236418 bfulgham 2014-08-11 18:09:21 -0700 Comment on attachment 236418 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. 1028545 3 roger_fong 2014-08-12 11:27:48 -0700 (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 1028546 4 roger_fong 2014-08-12 11:28:15 -0700 although all the EWS bots failed?... 1028547 5 236418 roger_fong 2014-08-12 11:28:44 -0700 Comment on attachment 236418 patch I think EWS was down for some time yesterday? 1028548 6 bfulgham 2014-08-12 11:31:02 -0700 (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. 1028557 7 roger_fong 2014-08-12 12:21:20 -0700 landed: http://trac.webkit.org/changeset/172481 236418 2014-08-11 17:44:57 -0700 2014-08-12 11:31:18 -0700 patch patch.patch text/plain 4938 roger_fong SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE3MjQyMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDE0LTA4LTExICBSb2dlciBG b25nICA8cm9nZXJfZm9uZ0BhcHBsZS5jb20+CisKKyAgICAgICAgQWRqdXN0IG1heC13aWR0aCBv ZiBjdWVzIGJhc2VkIG9uIHRleHQgYWxpZ25tZW50IHdoZW4gY3VlIHNpemUgaXMgZXhwYW5kZWQu CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzU4MjMu CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWxsIHVu aXRzIGFyZSBpbiBwZXJjZW50YWdlcy4KKyAgICAgICAgSWYgd2UgYXJlIGxlZnQgYWxpZ25lZCB0 aGUgbWF4IGN1ZSB3aWR0aCBpcyB0aGUgMTAwIG1pbnVzIHRoZSBjdWUgcG9zaXRpb24uCisgICAg ICAgIElmIHdlIGFyZSByaWdodCBhbGlnbmVkIHRoZSBtYXggY3VlIHdpZHRoIGlzIHRoZSBjdWUg cG9zaXRpb24uCisgICAgICAgIElmIHdlIGFyZSBjZW50ZXJlZCB0aGUgbWF4IGN1ZSB3aWR0aCBp cyBqdXN0IDEwMC4KKworICAgICAgICAqIGh0bWwvdHJhY2svVGV4dFRyYWNrQ3VlR2VuZXJpYy5j cHA6CisgICAgICAgIChXZWJDb3JlOjpUZXh0VHJhY2tDdWVHZW5lcmljQm94RWxlbWVudDo6YXBw bHlDU1NQcm9wZXJ0aWVzKToKKyAgICAgICAgKiBodG1sL3RyYWNrL1ZUVEN1ZS5jcHA6CisgICAg ICAgIChXZWJDb3JlOjpWVFRDdWVCb3g6OmFwcGx5Q1NTUHJvcGVydGllcyk6CisKIDIwMTQtMDgt MTEgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+CiAKICAgICAgICAgW2lPU10gPHZp ZGVvPiBlbGVtZW50IHJlcXVlc3RzIGFyZSBtaXNzaW5nIHNlc3Npb24gY29va2llczsgc29tZXRp bWVzIHBlcnNpc3RhbnQgY29va2llcy4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2sv VGV4dFRyYWNrQ3VlR2VuZXJpYy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRt bC90cmFjay9UZXh0VHJhY2tDdWVHZW5lcmljLmNwcAkocmV2aXNpb24gMTcyNDIxKQorKysgU291 cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWVHZW5lcmljLmNwcAkod29ya2luZyBj b3B5KQpAQCAtMTAyLDEzICsxMDIsMTkgQEAgdm9pZCBUZXh0VHJhY2tDdWVHZW5lcmljQm94RWxl bWVudDo6YXBwbAogICAgICAgICB9CiAgICAgfQogCi0gICAgc3RkOjpwYWlyPGZsb2F0LCBmbG9h dD4gcG9zaXRpb24gPSBtX2N1ZS5nZXRDU1NQb3NpdGlvbigpOworICAgIGRvdWJsZSB0ZXh0UG9z aXRpb24gPSBtX2N1ZS5wb3NpdGlvbigpOworICAgIGRvdWJsZSBtYXhTaXplID0gMTAwLjA7Cisg ICAgaWYgKGFsaWdubWVudCA9PSBDU1NWYWx1ZUVuZCB8fCBhbGlnbm1lbnQgPT0gQ1NTVmFsdWVS aWdodCkKKyAgICAgICAgbWF4U2l6ZSA9IHRleHRQb3NpdGlvbjsKKyAgICBlbHNlIGlmIChhbGln bm1lbnQgPT0gQ1NTVmFsdWVTdGFydCB8fCBhbGlnbm1lbnQgPT0gQ1NTVmFsdWVMZWZ0KQorICAg ICAgICBtYXhTaXplID0gMTAwLjAgLSB0ZXh0UG9zaXRpb247CisKICAgICBpZiAoY3VlLT5nZXRX cml0aW5nRGlyZWN0aW9uKCkgPT0gVlRUQ3VlOjpIb3Jpem9udGFsKSB7CiAgICAgICAgIHNldElu bGluZVN0eWxlUHJvcGVydHkoQ1NTUHJvcGVydHlNaW5XaWR0aCwgIi13ZWJraXQtbWluLWNvbnRl bnQiKTsKLSAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eU1heFdpZHRo LCAxMDAuMCAtIHBvc2l0aW9uLmZpcnN0LCBDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTX1BFUkNFTlRB R0UpOworICAgICAgICBzZXRJbmxpbmVTdHlsZVByb3BlcnR5KENTU1Byb3BlcnR5TWF4V2lkdGgs IG1heFNpemUsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVSQ0VOVEFHRSk7CiAgICAgfSBlbHNl IHsKICAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eU1pbkhlaWdodCwg Ii13ZWJraXQtbWluLWNvbnRlbnQiKTsKLSAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShD U1NQcm9wZXJ0eU1heEhlaWdodCwgMTAwLjAgLSBwb3NpdGlvbi5zZWNvbmQsIENTU1ByaW1pdGl2 ZVZhbHVlOjpDU1NfUEVSQ0VOVEFHRSk7CisgICAgICAgIHNldElubGluZVN0eWxlUHJvcGVydHko Q1NTUHJvcGVydHlNYXhIZWlnaHQsIG1heFNpemUsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVS Q0VOVEFHRSk7CiAgICAgfQogCiAgICAgaWYgKGN1ZS0+Zm9yZWdyb3VuZENvbG9yKCkuaXNWYWxp ZCgpKQpJbmRleDogU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9WVFRDdWUuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVlRUQ3VlLmNwcAkocmV2aXNpb24gMTcy NDIxKQorKysgU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9WVFRDdWUuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0xNzgsMjEgKzE3OCwyOCBAQCB2b2lkIFZUVEN1ZUJveDo6YXBwbHlDU1NQcm9wZXJ0 aWVzKGNvbnN0CiAgICAgaWYgKGF1dGhvckZvbnRTaXplKQogICAgICAgICBtdWx0aXBsaWVyID0g bV9mb250U2l6ZUZyb21DYXB0aW9uVXNlclByZWZzIC8gYXV0aG9yRm9udFNpemU7CiAKLSAgICBk b3VibGUgbmV3Q3VlU2l6ZSA9IHN0ZDo6bWluKG1fY3VlLmdldENTU1NpemUoKSAqIG11bHRpcGxp ZXIsIDEwMC4wKTsKKyAgICBkb3VibGUgdGV4dFBvc2l0aW9uID0gbV9jdWUucG9zaXRpb24oKTsK KyAgICBkb3VibGUgbWF4U2l6ZSA9IDEwMC4wOwogICAgIENTU1ZhbHVlSUQgYWxpZ25tZW50ID0g bV9jdWUuZ2V0Q1NTQWxpZ25tZW50KCk7CisgICAgaWYgKGFsaWdubWVudCA9PSBDU1NWYWx1ZUVu ZCB8fCBhbGlnbm1lbnQgPT0gQ1NTVmFsdWVSaWdodCkKKyAgICAgICAgbWF4U2l6ZSA9IHRleHRQ b3NpdGlvbjsKKyAgICBlbHNlIGlmIChhbGlnbm1lbnQgPT0gQ1NTVmFsdWVTdGFydCB8fCBhbGln bm1lbnQgPT0gQ1NTVmFsdWVMZWZ0KQorICAgICAgICBtYXhTaXplID0gMTAwLjAgLSB0ZXh0UG9z aXRpb247CisKKyAgICBkb3VibGUgbmV3Q3VlU2l6ZSA9IHN0ZDo6bWluKG1fY3VlLmdldENTU1Np emUoKSAqIG11bHRpcGxpZXIsIDEwMC4wKTsKICAgICAvLyB0aGUgJ3dpZHRoJyBwcm9wZXJ0eSBt dXN0IGJlIHNldCB0byB3aWR0aCwgYW5kIHRoZSAnaGVpZ2h0JyBwcm9wZXJ0eSAgbXVzdCBiZSBz ZXQgdG8gaGVpZ2h0CiAgICAgaWYgKG1fY3VlLnZlcnRpY2FsKCkgPT0gaG9yaXpvbnRhbEtleXdv cmQoKSkgewogICAgICAgICBzZXRJbmxpbmVTdHlsZVByb3BlcnR5KENTU1Byb3BlcnR5V2lkdGgs IG5ld0N1ZVNpemUsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVSQ0VOVEFHRSk7CiAgICAgICAg IHNldElubGluZVN0eWxlUHJvcGVydHkoQ1NTUHJvcGVydHlIZWlnaHQsIENTU1ZhbHVlQXV0byk7 CiAgICAgICAgIHNldElubGluZVN0eWxlUHJvcGVydHkoQ1NTUHJvcGVydHlNaW5XaWR0aCwgIi13 ZWJraXQtbWluLWNvbnRlbnQiKTsKLSAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQ cm9wZXJ0eU1heFdpZHRoLCAxMDAuMCAtIHBvc2l0aW9uLmZpcnN0LCBDU1NQcmltaXRpdmVWYWx1 ZTo6Q1NTX1BFUkNFTlRBR0UpOworICAgICAgICBzZXRJbmxpbmVTdHlsZVByb3BlcnR5KENTU1By b3BlcnR5TWF4V2lkdGgsIG1heFNpemUsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVSQ0VOVEFH RSk7CiAgICAgICAgIGlmICgoYWxpZ25tZW50ID09IENTU1ZhbHVlTWlkZGxlIHx8IGFsaWdubWVu dCA9PSBDU1NWYWx1ZUNlbnRlcikgJiYgbXVsdGlwbGllciAhPSAxLjApCiAgICAgICAgICAgICBz ZXRJbmxpbmVTdHlsZVByb3BlcnR5KENTU1Byb3BlcnR5TGVmdCwgc3RhdGljX2Nhc3Q8ZG91Ymxl Pihwb3NpdGlvbi5maXJzdCAtIChuZXdDdWVTaXplIC0gbV9jdWUuZ2V0Q1NTU2l6ZSgpKSAvIDIp LCBDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTX1BFUkNFTlRBR0UpOwogICAgIH0gZWxzZSB7CiAgICAg ICAgIHNldElubGluZVN0eWxlUHJvcGVydHkoQ1NTUHJvcGVydHlXaWR0aCwgQ1NTVmFsdWVBdXRv KTsKICAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eUhlaWdodCwgbmV3 Q3VlU2l6ZSwgQ1NTUHJpbWl0aXZlVmFsdWU6OkNTU19QRVJDRU5UQUdFKTsKICAgICAgICAgc2V0 SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eU1pbkhlaWdodCwgIi13ZWJraXQtbWluLWNv bnRlbnQiKTsKLSAgICAgICAgc2V0SW5saW5lU3R5bGVQcm9wZXJ0eShDU1NQcm9wZXJ0eU1heEhl aWdodCwgMTAwLjAgLSBwb3NpdGlvbi5zZWNvbmQsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVS Q0VOVEFHRSk7CisgICAgICAgIHNldElubGluZVN0eWxlUHJvcGVydHkoQ1NTUHJvcGVydHlNYXhI ZWlnaHQsIG1heFNpemUsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUEVSQ0VOVEFHRSk7CiAgICAg ICAgIGlmICgoYWxpZ25tZW50ID09IENTU1ZhbHVlTWlkZGxlIHx8IGFsaWdubWVudCA9PSBDU1NW YWx1ZUNlbnRlcikgJiYgbXVsdGlwbGllciAhPSAxLjApCiAgICAgICAgICAgICBzZXRJbmxpbmVT dHlsZVByb3BlcnR5KENTU1Byb3BlcnR5VG9wLCBzdGF0aWNfY2FzdDxkb3VibGU+KHBvc2l0aW9u LnNlY29uZCAtIChuZXdDdWVTaXplIC0gbV9jdWUuZ2V0Q1NTU2l6ZSgpKSAvIDIpLCBDU1NQcmlt aXRpdmVWYWx1ZTo6Q1NTX1BFUkNFTlRBR0UpOwogICAgIH0K