This bug represents substeps 10.2, 10.3 used to determine the text direction of the cue text. http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules "10.2 Apply the Unicode Bidirectional Algorithm's Paragraph Level steps to nodes using the following constraints, to determine the paragraph embedding level of the cue: [BIDI] * nodes represents a single paragraph. * The paragraph's text consists of the concatenation of the values of each WebVTT Text Object in nodes, in a pre-order, depth-first traversal, excluding WebVTT Ruby Text Objects and their descendants. 10.3 If the paragraph embedding level determined in the previous step is even (the paragraph direction is left-to-right), let direction be 'ltr', otherwise, let it be 'rtl'."
Created attachment 183639 [details] WIP Patch This is a work-in-progress patch, as I would like to make sure this is the right direction.
(In reply to comment #1) > This is a work-in-progress patch, as I would like to make sure this is the right direction. This is the way bidiResolver is called in GraphicsContext and TextRunIterator looks generic enough to me that it could be extracted from there and used in both places.
Comment on attachment 183639 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183639&action=review > Source/WebCore/html/track/TextTrackCue.cpp:640 > + if (!node->isTextNode() || node->localName() == rubyTag) I believe a "WebVTT Ruby Text Object" is a node with a local name of "rt" (rtTag). > Source/WebCore/html/track/TextTrackCue.cpp:648 > + String paragraph = paragraphBuilder.toString(); > + TextRun textRun(paragraph, paragraph.length()); > + LOG(Media, "%s", paragraph.utf8().data()); > + You can return early here if paragraphBuilder is empty.
Created attachment 184294 [details] Patch to determine the text direction
(In reply to comment #3) > (From update of attachment 183639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183639&action=review > > > Source/WebCore/html/track/TextTrackCue.cpp:640 > > + if (!node->isTextNode() || node->localName() == rubyTag) > > I believe a "WebVTT Ruby Text Object" is a node with a local name of "rt" (rtTag). Done. I used the rtTag, but named badly the variable. > > Source/WebCore/html/track/TextTrackCue.cpp:648 > > + String paragraph = paragraphBuilder.toString(); > > + TextRun textRun(paragraph, paragraph.length()); > > + LOG(Media, "%s", paragraph.utf8().data()); > > + > > You can return early here if paragraphBuilder is empty. Done. The patch is minimal to solely determine properly the text direction, since while working on it I think the parameter determination for RTL cues with is broken either in the implementation or in the spec. I will file a different bug to extend the added test with multiple rendering situations to make sure everything works properly (positioning/alignment/etc.)
Created attachment 184299 [details] Rebased patch
Comment on attachment 184299 [details] Rebased patch This looks reasonable to me, but most of it is out of my area of expertise so I would rather that someone else r+ it.
CC-ing Eric in the hope
(In reply to comment #8) > CC-ing Eric in the hope In the hope of a review, I meant :-)
Bidi layout is normally not done in the DOM. You need to know your line widths in order to paint. I assume that these text tracks are never wrapped?
Comment on attachment 184299 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=184299&action=review Can you help me understand what problem you're trying to solve? This does not seem like the right solution. > Source/WebCore/html/track/TextTrackCue.cpp:636 > + // ... to determine the paragraph embedding level of the first Unicode paragraph of the cue. > + > + // If the paragraph embedding level determined in the previous step is even > + // (the paragraph direction is left-to-right), let direction be 'ltr', otherwise, let it be 'rtl'. > + m_displayDirection = bidiRuns.firstRun()->level() % 2 ? CSSValueRtl : CSSValueLtr; This is a huge amount of code to answer this very simple question. You want basically text-direction: auto behavior it seems. If this is the right way to do this (and I don't believe that), then this is the wrong abstraction to use. You shouldn't have to do a full bidi line layout to ask what the direction of the first character is. :) Also, Bi-di text is just that "bi directional", so it commonly has mixed LTR and RTL. That's why we have lots of bidi runs. They're not going to all be the same direction. :)
(In reply to comment #11) > (From update of attachment 184299 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184299&action=review > > Can you help me understand what problem you're trying to solve? This does not seem like the right solution. So the WebVTT Cue Text rendering rules depend on a direction parameter, which is determined by the paragraph embedding level of the first paragraph of the cue - which is the concatenation of all those WebVTT text nodes. The direction parameter which results after the application of the Unicode Bidirectional Algorithm is further used at establishing the following parameters of a cue: x-position, y-position, maximum width (steps 10.4 - 10.11 of [1]) So this isn't a rendering of the cue lines (that's done later normally), it's just a determination for parameters that place the overall cue box within the video element (in case you think this is wrong, maybe we should file some bugs on the WebVTT spec). Either way, it looks like indeed what's needed is what RenderBlockLineLayout::determineDirection() does, but I'm not sure if the InlineIterator can be used here? [1] http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules
(In reply to comment #12) > Either way, it looks like indeed what's needed is what > RenderBlockLineLayout::determineDirection() does, but I'm not sure if the > InlineIterator can be used here? That was that conclusion I was afraid you'd come to. Since this isn't fundamentally rendered by the Rendering tree, we don't have Inlines for these boxes, right? You're still likely doing more work than necessary in your determineTextDirection function. You could just be iterating across the characters looking for strong directionality.
(In reply to comment #13) > (In reply to comment #12) > > Either way, it looks like indeed what's needed is what > > RenderBlockLineLayout::determineDirection() does, but I'm not sure if the > > InlineIterator can be used here? > > That was that conclusion I was afraid you'd come to. Since this isn't fundamentally rendered by the Rendering tree, we don't have Inlines for these boxes, right? At the point where this algorithm is run, no rendering happens (this is a preprocessing of the cue to ensure proper parameters are given when the actual rendering is done). > You're still likely doing more work than necessary in your determineTextDirection function. You could just be iterating across the characters looking for strong directionality. This is precisely what I need to do! (and I thought that this is what I was doing). So applying determineDirectionality with a TextRunIterator makes sense to you?
(In reply to comment #14) > This is precisely what I need to do! (and I thought that this is what I was > doing). So applying determineDirectionality with a TextRunIterator makes sense > to you? You're doing a lot more work than that! You're building a single string with all your text, creating text runs, and running the UBA over those text runs. All of this is necessary for rendering, but *not* for finding the directionality of the paragraph! You can simply iterate over the characters until you find one that's strongly directional. Then, obviously, bail.
Not sure if this is relevant to the current discussion, but the spec (http://dev.w3.org/html5/webvtt/#applying-css-properties-to-webvtt-node-objects) includes a full specification of the CSS to be used for a cue (and it is the CSS that is supposed to do all the rendering). Bidi-wise, this includes: the 'unicode-bidi' property must be set to 'plaintext' the 'direction' property must be set to [the value] determined by the rules for updating the display of WebVTT text tracks for the text track cue from whose text the List of WebVTT Node Objects was constructed. The 'text-align' property on the (root) List of WebVTT Node Objects must be set to the value in the second cell of the row of the table below whose first cell is the value of the corresponding cue's text track cue alignment: Text track cue alignment 'text-align' value Start alignment 'start' Middle alignment 'center' End alignment 'end' Left alignment 'left' Right alignment 'right'
(In reply to comment #16) > Not sure if this is relevant to the current discussion, but the spec (http://dev.w3.org/html5/webvtt/#applying-css-properties-to-webvtt-node-objects) includes a full specification of the CSS to be used for a cue (and it is the CSS that is supposed to do all the rendering). And that's what happens at that point - the cue is rendered with all the CSS parameters applied (section 3.5.1 as you pointed out). This is really not about rendering lines correctly, but computing the position of the cue within the video (and after the cue block is positioned, lines are rendered inside it normally). This is about step 10.2 of section 3.5. Let me give an example of the outcome for a cue with 'position' and 'alignment': 00:00:00.000 -> 00:00:30.000 position:30% align:end Cue text (arabic or english) If the direction determined in step 10.2 is: * LTR, the parameters are: size: 30%, x-position: 0%, y-position: 0% * RTL, the parameters are: size: 70%, x-position: 70%, y-position: 0% So I'm just trying to plug in properly the direction parameter. These parameters are then set as CSS values in: Step 10.7: If the text track cue writing direction is horizontal, then let width be 'size vw' Step 10.11: Let left be 'x-position vw' and top be 'y-position vh'. (These again are CSS values used by the next section to set CSS properties for the rendering; 'vw' and 'vh' are CSS units.)
Created attachment 187814 [details] Simple iteration through characters I stripped down the code to the simplest and direct method I could think of.
Ping about this? Are there concerns about the usefulness of the interpretation of the positioning attributes in the case of RTL text?
Comment on attachment 187814 [details] Simple iteration through characters View in context: https://bugs.webkit.org/attachment.cgi?id=187814&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Test: media/track/track-cue-rendering-rtl.html A general description of the patch would be nice. > Source/WebCore/html/track/TextTrackCue.cpp:597 > + // such as U+000A LINE FEED (LF), U+0085 NEXT LINE (NEL), and U+2029 PARAGRAPH SEPARATOR. > + return character == 0x000A || character == 0x0085 || character == 0x2029; Is it "such as" or "specifically"? The spec says "only" from what I can tell. > LayoutTests/media/track/track-cue-rendering-rtl-expected.txt:14 > +** The cue should contain arabic text ** > +EXPECTED (video.currentTime == '0.5') OK > +EXPECTED (testTrack.track.activeCues.length == '1') OK > +EXPECTED (testTrack.track.activeCues[0].text == 'تجربة') OK > +EXPECTED (testCueDisplayBox.innerText == 'تجربة') OK > + > +** The position should be default and CSS direction set to RTL ** > +EXPECTED (2 * testCueDisplayBox.offsetLeft == video.videoWidth - testCueDisplayBox.offsetWidth == 'true') OK > +EXPECTED (getComputedStyle(testCueDisplayBox).direction == 'rtl') OK This test should check more permutations of text, like a cue with multiple paragraphs, neutral characters, etc.
Other than the points I made, I think the implementation looks just fine.
Created attachment 193568 [details] More test cases
(In reply to comment #20) > (From update of attachment 187814 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187814&action=review > > Is it "such as" or "specifically"? The spec says "only" from what I can tell. I changed to check if Separator_Paragraph bit is set using Unicode::category. I hope that's appropriate. > This test should check more permutations of text, like a cue with multiple paragraphs, neutral characters, etc. I added more variations with strong, weak and neutral characters. Thanks for the review!
Comment on attachment 193568 [details] More test cases View in context: https://bugs.webkit.org/attachment.cgi?id=193568&action=review Thanks, lgtm! > Source/WebCore/html/track/TextTrackCue.cpp:591 > + return WTF::Unicode::category(character) & WTF::Unicode::Separator_Paragraph; FYI that this is an expensive, but functionally correct approach. Given the short size of the content usually being dealt with here, I think this is probably fine.
(In reply to comment #24) > (From update of attachment 193568 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193568&action=review > > Thanks, lgtm! > > > Source/WebCore/html/track/TextTrackCue.cpp:591 > > + return WTF::Unicode::category(character) & WTF::Unicode::Separator_Paragraph; > > FYI that this is an expensive, but functionally correct approach. Given the short size of the content usually being dealt with here, I think this is probably fine. Thanks, I'll keep this in mind (and change it, if there's a need) - at this point it should be run at most once per cue (or at most once each time the cue contents change)
Comment on attachment 193568 [details] More test cases Clearing flags on attachment: 193568 Committed r146128: <http://trac.webkit.org/changeset/146128>
All reviewed patches have been landed. Closing bug.