Compute positioning of single cues #99
Conversation
|
I've changed it up a bit now. The WebVTTParser now has a new method, One of my questions here is how should we best integrate this into the test suite? We'd have to create a fake text track list class and probably have separate JSON tests for these as it deals with multiple cues at a time. |
| return -1; | ||
| // TODO: Have to figure out a way to determine what the position of the | ||
| // Track is in the Media element's list of TextTracks and return that + 1, | ||
| // negated. For now return 0. |
There was a problem hiding this comment.
"For now return 0" and then you return 100. Typo in code or comment?
There was a problem hiding this comment.
Typo in the comment. I originally had it as 0, but switched it to 100 as that is the default value in the spec.
There was a problem hiding this comment.
OK, please fix this so the comment matches the code.
|
I've changed it a bit since my last comment. |
| return 100; | ||
| if (typeof cue.line === "number") | ||
| return cue.line; | ||
| if (!cue.snapToLines) |
There was a problem hiding this comment.
I don't think this case is necessary, if you're going to return 100 in the default case below anyway.
|
I've updated as per your requests Dave and converted all the current JSON tests to test the processing model as well. Adding enough tests to get good coverage for this is going to be a big job, but not too hard, so I was thinking it would be a good candidate for a student to do. If you agree then we can land this and open an issue for the student to work on? |
|
|
||
| return { | ||
| direction: position.direction, | ||
| writingMode: writingMode, |
There was a problem hiding this comment.
Get rid of the block of code from 414-418, the writingMode variable, and use a tertiary operation here too, like you do in lines 423, 424, etc.
|
I think having students work on these tests is reasonable, yes. |
| return { top: yPos, left: xPos, size: size }; | ||
| } | ||
|
|
||
| var direction = calcParagraphDirection(), |
There was a problem hiding this comment.
I still don't understand the need for these variables at all. Why not just move these functions down to 409++ and store the return value directly on the return object?
There was a problem hiding this comment.
I'm struggling to see a different way of doing this. What your suggesting would work for direction, but xPos, yPos, and size all change after they are calculated initially. When the margin is applied they all change based on the relationship with each other. So we calculate size, then xPos and yPos based on size, and then we apply a margin to the values and at that time either xPos and size will change or yPos and size will change.
| return "ltr"; | ||
| }(); | ||
|
|
||
| var boxLen = function(direction){ |
There was a problem hiding this comment.
should just be function boxLen(direction){ (i.e., no need for var here)....
OK, reading this again, you're running this function right away. Use this pattern:
var boxLen = (function(direction){
...
}(this.direction));
|
One last nit, and I think this is good. r=me with that fixed. |
This patch implements support for processing multiple VTTCues from multiple TextTracks at once as well as applying the initial CSS positioning and styles to the VTTCues once they are converted to a DOM tree.
This implements the part of the processing algorithm that positions single cues. I'd like to get some feedback on this before I proceed further.
What's left to do after this is to implement the part of the algorithm that adjusts the positions of cues in relation to the other cues that are being displayed and to implement the part that handles regions as well.
We also have to figure out the bidirectional paragraph problem and add more tests to make sure that we are hitting every path in the processing model algorithm. I think some students might be able to help us with that.
Partial fix for #89 and #86.