Skip to content

Normalize declarative waveform configuration - #22

Closed
scruffian wants to merge 3 commits into
arraypress:mainfrom
scruffian:scruffian/waveform-pr-fixes
Closed

scruffian wants to merge 3 commits into
arraypress:mainfrom
scruffian:scruffian/waveform-pr-fixes

Conversation

@scruffian

@scruffian scruffian commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Stop package imports from auto-initializing [data-waveform-player]; WaveformPlayer.init() remains the explicit declarative path.
  • Preserve existing CDN/script behavior by keeping auto-initialization in browser builds.
  • Normalize declarative config: allow only documented button alignments, accept only valid playback-rate lists, and sanitize legacy data-play-icon / data-pause-icon SVG instead of inserting raw markup.
  • Keep constructor/API custom icons working through playIcon and pauseIcon.

Why

Gutenberg allows users to supply arbitrary HTML in data attributes. We should guard against this by only allowing supported attributes. We also allow users of the module to initialize playlist players programmatically, while this package also supports declarative markup. This keeps both paths predictable without silently breaking existing declarative icon users.

Testing

  • npm test
  • npm run build

cc @arraypress

@scruffian
scruffian marked this pull request as ready for review August 10, 2026 12:32
arraypress added a commit that referenced this pull request Aug 10, 2026
… against claimed elements)

buttonAlign is interpolated into a class name, so an unrecognised value from
either configuration path reached the markup. It is now coerced back to 'auto'
in createDOM(), which covers constructor options and data-button-align alike.

WaveformPlayer.init() only recognised its own data-waveform-initialized flag,
so an element built programmatically but also carrying data-waveform-player
was re-initialized by the next scan. It is now skipped by instance lookup.

Both reported by @scruffian in #22.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@arraypress

Copy link
Copy Markdown
Owner

Thanks Ben — you found two real things here, and I've landed both in 1.24.4, credited to you in the changelog:

  • buttonAlign validation. The resolved value is interpolated into class="waveform-track waveform-align-${buttonAlign}", so an unrecognised value genuinely reached the markup. I put the allowlist in createDOM() rather than parseDataAttributes so it guards constructor options too — the data-attribute path wasn't the only unvalidated one, and the wrappers all forward buttonAlign as a plain string.
  • The getInstance(element) guard in autoInit(). That's the actual double-init bug, and it fixes it on its own — an element built programmatically carries no data-waveform-initialized flag, so the scan needed to recognise it by instance.

I'm going to pass on the rest, though.

Disabling auto-init for ESM/CJS is a breaking change for anyone doing import '@arraypress/waveform-player' with declarative markup, and the __WAVEFORM_PLAYER_AUTO_INIT__ define means the ESM and IIFE builds would behave differently — a permanent documentation and support tax. The double-init problem it targets is already solved by the getInstance guard above, so the cost isn't buying anything the cheaper fix doesn't.

The SVG sanitizer is the one I feel strongest about. If someone can write data-play-icon="<svg onload=…>" into the page, they can already write <script> — the sink only exists when a host app interpolates untrusted input into that attribute, which is an escaping problem in the host rather than in the player. In Gutenberg that's wp_kses's job at save time. Meanwhile a hand-rolled allowlist in a zero-dependency package is something I'd own forever, it silently breaks existing icons using mask/filter/style/pattern, and it'd be inconsistent anyway since constructor playIcon stays raw and the React/Vue/Svelte wrappers all go through that path. The docs advertise "raw SVG markup" and I'd like to keep that true.

This library is framework-agnostic and I don't want to reshape its contract around one host's escaping model — but I appreciate you digging into it, and the two fixes above wouldn't have happened otherwise. Closing this in favour of what shipped; happy to look at anything else you hit.

@arraypress arraypress closed this Aug 10, 2026
@scruffian

Copy link
Copy Markdown
Contributor Author

Thanks for the quick reply. What do you also think about adding validation for playback rates?

@arraypress

Copy link
Copy Markdown
Owner

Coming back to this: yes — and you were more right than the question implied. It wasn't just playback rates.

Digging into playbackRates turned up that setJson in parseDataAttributes only guards syntax, not shape. JSON.parse accepts '2' and '{"a":1}' happily, so a valid-but-wrong value sailed through and detonated at the first .map() — data-playback-rates="2" threw playbackRates.map is not a function and took the whole player down. markers had the identical gap ('"hello"' → markers.forEach is not a function), and a string has a truthy .length, so the existing guard waved it straight through.

That pattern turned out to be live in four packages. waveform-playlist was the worst: parseTracks() read every track's data-markers through a bare JSON.parse, and it runs before anything renders — so one malformed attribute anywhere in the markup destroyed the entire playlist rather than costing that one track its markers. waveform-bar's persisted queue, waveform-editor's numeric attributes, and the waveform-gen CLI all had versions of the same thing.

Shipped in 1.25.0: every option is now normalized once, on the merged result, so the constructor path, the data-* path and every wrapper get identical guarantees at a single point. An unusable value resolves to its documented default and warns, naming the option, rather than reaching a consumer that assumed a shape nobody checked. That covers the numeric options too — data-height="tall" used to become NaN and size the canvas to zero, which looked like a broken player rather than a bad attribute.

Two related changes while I was in there: playbackRates now accepts the bare list an author actually types (data-playback-rates="0.5, 1, 2") alongside the JSON form, and setPlaybackRate clamps to 0.25–4 instead of 0.5–2 — the old cap meant the menu could offer a rate the setter would silently refuse, so the label read 10x while playback ran at 2x.

You're credited in the changelog. My position on the SVG sanitizer hasn't changed — that one's still an escaping problem in the host — but the validation question was the right one to push on, and none of the rest would have happened without it. Thanks for coming back to it.

@jeryj

jeryj commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Thanks for your consideration and follow-ups on this, @arraypress! I opened #24 as a middle ground that avoids requiring the package to maintain an SVG sanitizer.

Gutenberg only needs the core, programmatic player—not automatic declarative initialization. Exporting the core separately lets us initialize only the elements Gutenberg creates and controls, without scanning unrelated user-authored markup across the page. The existing default and CDN initialization behavior remains unchanged. This would be greatly useful for us.

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