OCTO-11611 Pair WebVTT closing tags by tag identity, not stack position - #431
Merged
Merged
Conversation
🟢 PR Compliance ReviewRisk Level: LOW
SAFE TO MERGE - No critical issues found Full report available in workflow artifacts |
🟢 PR Compliance ReviewRisk Level: LOW
SAFE TO MERGE - No critical issues found Full report available in workflow artifacts |
🟢 PR Compliance ReviewRisk Level: LOW
SAFE TO MERGE - No critical issues found Full report available in workflow artifacts |
lorandvarga
approved these changes
Sep 16, 2026
panasicov
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WebVTTWriter._partition_nodes_by_layoutassigned each closingSTYLEnode to alayout group by popping
open_tags, a stack holding only group indexes — norecord of which span an entry belonged to. The pop was blind: it assumed every
opening node has exactly one closing node and that spans nest properly. Two
node shapes break that assumption and yield cues whose inline markup does not
balance.
<i>a<u>b</i></u>straddling a boundary, the</i>popped the underline's index and the
</u>the italic's, so each cue closed atag it never opened. The same shape with a class span —
<c.loud>x<i>y</c></i>— is what forces the match to key on the tag's content keys rather than its
content:
<c.loud>carries{'classes': ['loud']}and</c>carries{'classes': []}, so direct dict comparison never matches.WebVTTReaderemits a karaoke timestamp ascreate_style(True, {"timestamp": us})with no closing node, so the entry waspushed and never popped; the stack stayed permanently misaligned and the next
genuine closing tag consumed the timestamp's index instead of its own.
Symmetrically, a closing tag with no opener took an open span's slot, leaving
that span's own tag unmatched in turn — spreading one stray tag across two cues.
Both require a caption whose text nodes carry differing
layout_info— theone-cue-per-position path — which is why the existing suite did not catch them.
Where these shapes come from. None of pycaption's own readers produce them
today, and the PR should not be read as fixing a reachable output bug. DFXP and
SAMI build spans through XML parsers that nest properly; SCC emits only italics
and balances them in
_remove_noop_italics; andWebVTTReader— the one readerthat emits karaoke timestamps and value-less closing tags — sets
layout_infoonthe
Captionand never on individual text nodes, so VTT input does not reach themulti-group path at all. These are node lists a library consumer assembles
directly, or that a future reader change would start producing. What the fix buys
is that tag balance in this path is now structural: it no longer depends on the
node list happening to hold a shape the blind pop tolerates.
This is not a regression from #429. The
open_tagsstack was introduced there,and for these same inputs
mainproduced strictly worse output: cues fused intoone block with two timing lines. This is unfinished work from that fix.
The change
The stack now keys on tag identity rather than stack position. Each opening tag
pushes
(frozenset(node.content), group_index), and a closing tag pairs with themost recent unclosed entry holding the same content keys — mirroring
WebVTTReader._pop_matching_tag, which matches on key sets for the same reason.Searching from the top of the stack closes nested same-key spans innermost-first.
A closing tag that matches nothing is one with no opening tag at all: every
reader builds a span's close node from the same keys as its open node
(
sami/reader.py,dfxp/reader.py,scc/specialized_collections.pyall pass thesame dict to both), so a genuine pair cannot miss. Nothing is popped for it, and
it joins the text before it — as
_pop_matching_tagalso leaves the stack aloneit joins the text before it — as
_pop_matching_tagalso leaves the stack alonewhen it finds no match.
Timestamp nodes are kept off the stack entirely, mirroring the same exclusion in
WebVTTReader._track_open_tag. Key-based pairing already declines to match atimestamp against a real closing tag, so this is belt-and-braces: it keeps every
entry on the stack a span still awaiting a tag of its own.
Tag balance is now independent of where the input placed its tags, at the
unchanged cost that a span crossing a boundary keeps its style only on its
opening tag's group. Spans that cross while sharing the same content keys still
cannot be told apart and fall back to LIFO.