Conversation
Four items split out of the WEBDEV-8965 review so they didn't hold up the radio player release. Nothing covered the `color: inherit` rule on .unstyled-button. Buttons don't inherit colour from their parent, so without it the container's themed colour never reaches the icons and their currentColor falls back to the UA default. There's a test for that now, driven through the public theme variable. It skips the play/pause button on purpose, since that one sets its own colour and would pass either way. The decorative icons are hidden from assistive tech. Every button carries its own aria-label, and replay and skip-ahead each contain a <text>10</text> that gives the SVG accessible content of its own. All ten get the attribute, matching what ia-expandable-search-bar and ia-radio-player already do. The test walks the element through playing and the two quieter volume states as well, because pause, volume-medium and volume-mute never render otherwise, and it asserts which buttons it covered so a future icon can't slip past unchecked. MAX_PLAYBACK_RATE is documented. The one comment above the pair only described MIN_PLAYBACK_RATE. The playback rate goes through Intl.NumberFormat, in its own module alongside the one ia-transcript-view already has for durations. It was interpolated as a raw JS number, so it always formatted with a decimal point whatever the reader's locale. The test uses a third rather than a quarter step: formatting stops at three fraction digits in every locale, while the raw value spells out all sixteen, so the assertion tells the two apart without depending on the locale the tests run in. That formatter is also where an unusable rate now stops. `playbackRate` is public, so `NaN` could reach the display and the screen reader as "Playback speed, currently NaN". Rates outside the range the button steps through are left alone: a media element will still play at those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DaEr5YT1xpjK5ydNZPvjwx
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 85.79% 85.81% +0.02%
==========================================
Files 74 75 +1
Lines 2619 2623 +4
Branches 577 578 +1
==========================================
+ Hits 2247 2251 +4
Misses 195 195
Partials 177 177 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
The four nits and coverage gaps from Neeraj's review of #80, split out at the time so they didn't hold up the radio player release.
Three of the four are what the ticket asked for. The fourth turned into a bit more:
playbackRateis a public property, so an unusable value reached the screen reader as "Playback speed, currently NaN". The new formatter is the natural place to stop that, and there's already a test making the equivalent promise for volume. Rates outside the range the speed button steps through are still shown as they are, since a media element will happily play at those. Say the word if you'd rather that came out and went on its own ticket.aria-hiddenwent on all ten icons rather than just the two the ticket named. The other eight are equally decorative, andia-expandable-search-barandia-radio-playeralready do this, so this brings the component in line rather than starting something new.Two of the tests I wrote first were vacuous, which is the exact failure this ticket exists to clean up, so it's worth saying how they're pinned down now. The locale one compared
Intloutput against the raw number, which are the same string in the en-US test browser; it uses a third instead, where formatting stops at three fraction digits in every locale and the raw value spells out sixteen. And thearia-hiddensweep only ever saw seven of the ten icons, since pause and the two quieter volume icons never render in the default state; it now walks the element through those states and asserts which buttons it covered.I checked each fix by reverting it and re-running: every new test fails without the change it covers.
QA
Everything runs on the PR preview, no login needed.
Preview: https://internetarchive.github.io/elements/pr/pr-100/#elem-ia-playback-controls
Go to the playback controls demo and confirm:
✅ Only the
<ia-playback-controls>demo renders.✅ The speed button reads
1xand the volume reads100%.Press the speed button on the playback controls demo and confirm:
✅ It steps 1.25x, 1.5x, 1.75x, 2x, then back round to 0.5x.
✅ The decimal separator matches your browser's language. Set the browser to German and it reads
1,25x.Set "Playback rate" to something that isn't a number in the Settings table on the playback controls demo and confirm:
✅ It shows
1xrather thanNaNx.Inspect any control button in devtools on the playback controls demo and confirm:
✅ Its
<svg>carriesaria-hidden="true".🤖 Generated with Claude Code
https://claude.ai/code/session_01DaEr5YT1xpjK5ydNZPvjwx