feat(packages): remove native controls after mounting our controls#1727
Closed
sampotts wants to merge 1 commit into
Closed
feat(packages): remove native controls after mounting our controls#1727sampotts wants to merge 1 commit into
sampotts wants to merge 1 commit into
Conversation
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (10)
Players (5)
Skins (30)
UI Components (38)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (9)
Skins (27)
UI Components (32)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (14)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
sampotts
force-pushed
the
feat/native-controls
branch
from
f22d815 to
3c90967
Compare
luwes
reviewed
Jun 23, 2026
| if (hasMediaChanged || hasContainerChanged) { | ||
| this.#detachStore(); | ||
| this.#detach = store.attach(target); | ||
| if (isMediaControlsCapable(target.media) && target.media.controls) target.media.controls = false; |
Collaborator
There was a problem hiding this comment.
I read the issue but I find it slightly presumptuous. what if the user does want to use the native controls for some reason.
Member
There was a problem hiding this comment.
I can see that but at the same time I think it'd be unlikely someone has both provided custom controls and wants native controls?
Collaborator
Author
There was a problem hiding this comment.
I'd have preferred to add the native controls removal logic to our controls component but we don't actually have one (it's just a <div>) so we'd have to add a <media-controls> / <Controls> component, which would obviously add a tiny bit more to the bundle.
Member
There was a problem hiding this comment.
We do have controls components?
Collaborator
Author
There was a problem hiding this comment.
"You're absolutely right!"
I don't know why we're not using them currently in the skins (probably an oversight on my part way back) but I'll update and put the logic there instead.
mihar-22
reviewed
Jun 23, 2026
sampotts
force-pushed
the
feat/native-controls
branch
from
3c90967 to
c6eceee
Compare
Collaborator
Author
|
I've changed it to be based on mounting of our custom controls instead of the container. This would allow consumers to use native controls if they wanted to. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c6eceee. Configure here.
sampotts
force-pushed
the
feat/native-controls
branch
from
c6eceee to
1415071
Compare
sampotts
changed the title
Member
|
Going to close this out as |
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
4 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.
Summary
Remove the native controls after mounting our custom controls component. This allows for progressive enhancement and has been noted in the migration docs.
Given it affects skins, I won't merge until @mihar-22 gives it the ok.
Closes #1160
Note
Medium Risk
Touches core player attach lifecycle and mutates native
controlson media elements across HTML and React; behavior is covered by new tests but regressions could affect fallback playback without custom UI.Overview
Enables progressive enhancement: media can ship with native
controlsuntil a custom controls UI mounts, then native controls are turned off and restored if all custom controls unmount.A new controls registration path tracks mounted control roots (HTML
media-controlsvia player context; ReactControls.RootviaregisterControls/controlsMounted). The provider mixin and React provider useisMediaControlsCapableto setmedia.controls = falseonly while at least one controls instance is registered, with refcounting when multiple roots exist.Skins and presets wrap control chrome in
media-controls/Controls.Rootinstead of plain divs, anddefineControls()is wired into audio/live-audio UI registration. ReactVideo/Audioomit thecontrolsattribute whencontrolsMountedis true.Reviewed by Cursor Bugbot for commit 1415071. Bugbot is set up for automated code reviews on this repo. Configure here.