Skip to content

handle line-ending tags; refactor payload breaks#743

Open
conorom wants to merge 1 commit into
ableplayer:developfrom
conorom:348_line_ending_tags_and_refactor_breaks
Open

handle line-ending tags; refactor payload breaks#743
conorom wants to merge 1 commit into
ableplayer:developfrom
conorom:348_line_ending_tags_and_refactor_breaks

Conversation

@conorom

@conorom conorom commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #348
... for real this time 🤞

I thought I'd take another shot at this bug. When I'd looked at it a few years ago, I was certain the problem was with the OR condition here, i.e. the || /^\s+$/.test(nextLine)) { bit.

And it kind of is. That break-on-whitespace OR condition is always going to cause issues with unexpected whitespace use, and no small measure of confusion. In no way is any sort of non-line-ending whitespace a cue terminator per the WebVTT spec. Only a blank/empty line ends the payload/cue.

But it's not clear-cut stuff. Note also that a line ending with a tag and then a trailing whitespace causes slightly different behavior than a line simply ending in a tag. And example 2 in the issue description happens to have a trailing space, i.e. <v Roger Bingham>

I spent a lot of time working through this, admittedly chatting with Claude, and doing manual testing.
I'd already committed to removing the OR condition but once I examined the issue that caused it to be added in the first place I wanted to ensure I didn't cause a regression with multiple blank lines and empty cue payloads.

I won't bore you with any more details. The resulting change parses the below much-fiddled-with version of wwa_captions_en.vtt. I realize there are things in here that wouldn't be recommended in a real WebVTT file, but I'm happy that the parser doesn't break on them and the output is as I'd expect, as I loaded Video 1 in dev (over and over!).

WEBVTT
kind: captions
lang: en

NOTE This VTT file includes a few extras:
- Meta data at the top (lines 2 and 3)
- The current comment block, identified with NOTE and surrounded by spaces
- cue settings at the end of the first timestamp
- A cue span at the start of the second cue payload
- (in this case, a "voice span" <v> identifying the speaker)
- A WebVTT cue settings list after the timestamp at 39.132
- Currently AblePlayer does not support these extra features
- but the parser needs to be able to filter them out
-
- Added on April 1, 2015 for testing purposes (these are both valid):
- A cue block with no content
- Multiple line breaks between some cue blocks
-
- More info in the WebVTT spec (still evolving):
- http://dev.w3.org/html5/webvtt

00:00:00.429 --> 00:00:09.165
[ music ]

00:00:09.165 --> 00:00:10.792
<v Narrator 1>
You <i>want</i> these people.

00:00:10.792 --> 00:00:13.759
They <b>order</b> your products,
<b>sign up</b> for your services,

00:00:13.759 --> 00:00:16.627
<v Narrator 2> 
<b>enroll</b>
     in your classes,
<b>read</b> your opinions,

00:00:16.627 --> 00:00:18.561
and <b>watch</b> your videos.


00:00:18.561 --> 00:00:24.165
<v Narrator 3> You'll never see them, but they know you-
through your website.

00:00:24.165 --> 00:00:25.891
Or maybe not.

00:00:25.891 --> 00:00:30.396

00:00:30.396 --> 00:00:32.363
but a vibrant
community of individuals

00:00:32.363 --> 00:00:35.297
with varying tastes,
styles, and abilities.

00:00:35.297 --> 00:00:36.900


         

00:00:39.132 --> 00:00:41.000 position:10%,start align:start size:35%
<v Terrill> It's important for
web designers and developers

00:00:41.000 --> 00:00:45.500
to realize that what they see
currently on their computer,

00:00:45.500 --> 00:00:49.264
at their resolution, with their browser
and their operating system

00:00:49.264 --> 00:00:52.000
is not going to be necessarily
the same thing that everybody else sees.

@joedolson

joedolson commented Jun 10, 2026

Copy link
Copy Markdown
Member

Thank you, @conorom!

This looks good in principle; I'll probably not get around to taking a proper look until after 5.0 releases, though.

Would you be able to add unit tests for this? If not, I can, but if you're comfortable with that, they'd definitely be helpful!

@joedolson joedolson mentioned this pull request Jun 10, 2026
Closed
@conorom

conorom commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@joedolson Actually at one point (for a prior attempt before I hit and fought more corner cases) I had added Jest to the project and did have unit tests in place for script/webvtt.js.

I sort of decided that adding that to the PR would be too much.

Funny, at that time I was working off an out-of-date fork.
I see Jest has actually been added to the project since then.

I'm happy to add tests for the weird cases I can think of at this point (before I forget).

One question: is there documentation on how the current test suite is run or anything like that? Thanks!

@joedolson

joedolson commented Jun 11, 2026

Copy link
Copy Markdown
Member

The test suite is run using npm test. You may need to tweak the local endpoint; Able Player doesn't include a local set up, so you need to provide it with whatever your local is. I have it at http://localhost:8000.

It's also prone to timeouts, so you may need to adjust the default timeout. I haven't resolved this yet.

And I agree that adding a test suite is too much for the average PR; but since there are test runners already, I'd prefer to add tests along with functionality changes when possible.

@conorom conorom force-pushed the 348_line_ending_tags_and_refactor_breaks branch from 6794601 to c6986c9 Compare June 11, 2026 21:14
@conorom conorom force-pushed the 348_line_ending_tags_and_refactor_breaks branch from c6986c9 to f022cd8 Compare June 11, 2026 22:01
@conorom

conorom commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@joedolson Excuse the force-pushing.
I think the added test covers it, but let me know if you don't like the look of anything, or notice a problem.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@conorom @joedolson