Skip to content

OCTO-11611 Pair WebVTT closing tags by tag identity, not stack position - #431

Merged
OlteanuRares merged 3 commits into
mainfrom
OCTO-11611
Sep 16, 2026
Merged

OlteanuRares merged 3 commits into
mainfrom
OCTO-11611

Conversation

@OlteanuRares

@OlteanuRares OlteanuRares commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

WebVTTWriter._partition_nodes_by_layout assigned each closing STYLE node to a
layout group by popping open_tags, a stack holding only group indexes — no
record 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.

  • Crossed spans. With <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 a
    tag 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.
  • Nodes with no counterpart. WebVTTReader emits a karaoke timestamp as
    create_style(True, {"timestamp": us}) with no closing node, so the entry was
    pushed 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 — the
one-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; and WebVTTReader — the one reader
that emits karaoke timestamps and value-less closing tags — sets layout_info on
the Caption and never on individual text nodes, so VTT input does not reach the
multi-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_tags stack was introduced there,
and for these same inputs main produced strictly worse output: cues fused into
one 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 the
most 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.py all pass the
same dict to both), so a genuine pair cannot miss. Nothing is popped for it, and
it joins the text before it — as _pop_matching_tag also leaves the stack alone
it joins the text before it — as _pop_matching_tag also leaves the stack alone
when 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 a
timestamp 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.

@OlteanuRares
OlteanuRares requested a review from a team as a code owner September 15, 2026 12:20
@github-actions

Copy link
Copy Markdown

🟢 PR Compliance Review

Risk Level: LOW

  • Compliance Issues: 0 (0 critical)
  • Regressions: 0

SAFE TO MERGE - No critical issues found

Full report available in workflow artifacts

@github-actions

Copy link
Copy Markdown

🟢 PR Compliance Review

Risk Level: LOW

  • Compliance Issues: 0 (0 critical)
  • Regressions: 0

SAFE TO MERGE - No critical issues found

Full report available in workflow artifacts

@github-actions

Copy link
Copy Markdown

🟢 PR Compliance Review

Risk Level: LOW

  • Compliance Issues: 0 (0 critical)
  • Regressions: 0

SAFE TO MERGE - No critical issues found

Full report available in workflow artifacts

@OlteanuRares
OlteanuRares merged commit 4305ac3 into main Sep 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants