-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extension is no longer working after browser updates and manifest v3 (SOLVED). #47
Comments
Thank you for making this but please make a PR, not only this will help devs merge faster and with proper attribution, this will also help users ensure there is no rogue code as happend with other popular extensions. |
Interesting! I could create a
|
@XanSama I've created a I could of course just update the branch using the zip file you have attached, but I would really like to give proper attribution. Cheers |
@XanSama Are you sure that the
I think the biggest challenge are sites which do not insert their audio/video elements into the DOM tree at all, e. g. Spotify which (according to Chrome Developer Tools / Heap Snaphot) has a Detached Video Element. Getting access to those would be really cool. The |
Good catch on the attachShadow; site I was testing with apparently doesn't always use them. Unfortunately this version required blanket host_permissions for <all_urls> as we need to be able to inject script into WORLD:MAIN to make the hooks work as intended. I also added a hook for HTMLMediaElement.prototype.play and that resolves the issue with the hidden object in the test page. Confirmed working on all tests, Spotify, new Reddit, and YouTube. Still don't feel like making a PR so here's another attachment. |
@XanSama Nice! I did not know about this Hooking Trick (inject a function into Element I really woudln't call myself a software developer, especially not with JavaScript which I kinda hate with a passion. Golang is much more up my alley. Nonetheless (after reading your code) I already thought that hooking into a HTMLMediaElement method might be what we need for sites like Spotify. Though my idea was to somehow hook into the constructor itself and remember those deatched elements for when we want to switch their Your approach (hooking the By the way, I'm perfectly happy with you providing your code/changes as attachments to this issue. I'll soon update the I'm also currently working on a new version of the Rain-Fighters/AudioPick web site (using hugo) which I want to have proper documentation about how the new ( As soon as the web site is ready and I've tested, understood and documented the v3 version of the extension well enough, I'll update the Chrome Web Store page, too. (Probably early 2024). Which reminds me .... Merry Christmas and a Happy New Year to everyone and a BIG THANK YOU to @XanSama who made us this xmas present. |
@XanSama Can you give me your GitHub I basically need to know those leading digits ... |
Address is: "109573827+XanSama"... I'd initially not hooked the constructor because it doesn't seem to be reliably called for several ways that the objects can be created but your comment inspired me to take another look. I've updated it to hook Document.prototype.createElement in addition to the others as in theory that needs to be called to create an element outside of the DOM/shadowDOM. I've also adjusted the createElement() and play() hooks to create an eventListener per audio/video element that watches for the custom event "changeSinkId". This allows us to set the sinkId on all elements immediately rather than waiting for the next play(). Also added a dirty catch for that runtime error to suppress it; could probably actually handle the error but it doesn't seem to matter in my testing. |
Hmm, does hooking Brainstorming ....
Would that work? Also, let me update the extension once on GitHub with your 0.3.6 version and my white-space/EOL fixes so that you can base your 0.3.7 version on that. OK? |
Unfortunately not. The play() hook does address that though. Honestly we may be able to do without the createElement hook and just use the play() hook with the event listener for immediate sink changes. |
yeah, lets not make things too complicated. I'll push your 0.3.6 version in a moment. Please use the updated version on GitHub as a base for you next version (with the custom event listener added via the play() hook). |
@XanSama pushed your Also, I'm wondering, if we still need the ShadowRoot hook. Don't we catch elements from the Shadow DOM via our play hook (+ custom event listener), too? |
AudioPick-manifest-v3_0.3.7.zip Re-based and added the event trigger. Also cleaned up the resources example a little. Edited to correct attachment. |
@XanSama hmm, looks like you attached the wrong (same as before) archive. |
Woops. Edited with correct version now. |
@XanSama Imported and pushed I really like the P. S.: I'm still wondering whether we actually need to attach to ShadowRoots or even need to observe the DOM tree with the event trigger in place ... hmm ... Good Night! |
Merry Christmas! Tests OK on all examples, Reddit, and Spotify using only the play() hook. It seems to test OK with the shadowRoot code disabled as well... theoretically if the site managed to load/create an element in the shadowDom that the user could directly interact with -before- our content script loaded it wouldn't catch that with the shadowRoot code disabled but I'm having a hard time imagining how one would even do that to test... Tentatively attaching |
@XanSama Almost! The issue is that clicking To reproduce the issue use our example page and
I still think that the custom event listener is the way to go, i. e. the actual (A) Visible/Attached HTMLMediaElementsThese are either already in the DOM tree or are inserted later. The former we catch by traversing the tree once (-> But instead of (or additionally to) changing the The (B) Detached HTMLMediaElementsThose are dynamically created at runtime and are usually not inserted into the DOM tree later. We assume that the only way to activate them while they are not visible is by calling their Note that dynamically created elements might already have the event listener, when their Also note that for this approach it does not matter whether an element was attached to a shadowRoot or not. Or am I overlooking something? What do you think? P. S.: I havent commited and pushed the new P. P. S.: I assume that we still need the P.P.S.: Also, I'm still getting some errors ... (Context/URL does not matter) P. P. P. S: And it looks liike it does not work on soundcloud. I've played around a bit with the developer console and it looks like soundcloud has DetachedAudioElements into which we apparently manage to inject our event listener via the |
Ahh, that was what I ran in to in my initial testing but missed it after I adjusted the play/pause buttons because I wasn't interacting directly with the media element. Confirmed / reproduced / resolved. I've updated the code that sets the sinkId to set the sinkId and add a listener to an object on first pass, or leave it alone if it already has a listener. Objects generally all handle their own sinkId updates after the initial set. I tweaked the injection (set it to all frames) and the content.js message handling (only register a handler in the top window). SoundCloud appears to be using AudioContext objects for their streaming playback. I didn't know those existed until just now but I've implemented a hook of them as well in this version using the same logic/event handling as the existing functions. Tests OK on all resource tests using direct media controls and javascript triggers. Tests OK on Spotify, Soundcloud, Youtube, and Reddit. Let me know if I missed anything! Re:PPS - Yes, the function prototypes can't be hooked from the ISOLATED world content.js runs in, and main.js can't readily message with the rest of the extension from the MAIN world it runs in so both are required currently. |
Yeah, looks a lot better now. Still some minor issues, but I'll push 0.3.8 after some further tests and some more white space/indention cleanup, i. e. you need to base your next version (I'm sure there will be one 😉) on the upcoming push to GitHub. Meanwhile, thanks a LOT for your work. The fact that AudioPick now supports Spotify and Souncloud, i. e. sites which do not insert their Media Elements into the DOM tree, is fantastic. Not to mention all the changes that were needed to adjust the extension for Manifest V3. ⭐ ⭐ ⭐ |
@XanSama I've pushed One thing that still does not work (reliably) is opening a SoundCloud link in a new tab, e. g. which then automatically starts to play and apparently does not inject the listener. Interestingly it seems to work, when you have stored a domain or global device other than "default". A timing issue? I was also wondering about this: Lines 23 to 26 in eb8d258
I mean, we are setting Cheers |
That check is needed because AudioContext objects don't support "default" as a sinkId. If you want an AudioContext to use the default sinkId you need to submit a blank value instead. The SoundCloud issue you describe may be related to that; I realize I didn't put the same logic in the code that sets the initial sinkId. You're right that we should be using e.detail there; missed that one in my haste. I've just done some quick testing and it seems like all objects will accept a blank sinkId and interpret it as default so I've removed the object type check and now just set the sinkId to "" if the selected device is "default" regardless. I've also added the logic to the code that sets the initial value (just below the lines you quoted). If that doesn't resolve the SoundCloud issue then we may need to take a look at the logic in content.js for injectSinkId() but I can't picture how it would cause issues currently. |
@XanSama Thansk a lot! I still (even with the new The other thing I'm still not (fully) convinced of is that we actually need to handle shadowRoots. IMHO we either get those elements via injecting into Anyway, I'm VERY HAPPY about the current state/functionality of the Manifest V3 Version. Hence I'm heading towards releasing it on GitHub and then on the Chrome Webstore, i. e.
Thanks again for this 🎄WONDERFUL CHRISTMAS PRESENT 🎄 |
@XanSama I'm thinking about removing the option to set/store a global audio sink/device to reduce the number of unneeded/unwanted microphone access exceptions added by the extension. Since we are now able to set/store per domain sinks/devices (which we could not before), we would loose very littlle functionality, but win a lot of user acceptance. This also better matches the fact that we do not have a global options panel and that you can only open the popup for HTTPS tabs/sites. What do you think? |
I'm unable to replicate this on my end. If it is a race condition I wonder if it could be mitigated by adjusting the load order of the scripts in manifest.json so main.js loads before content.js? Might be worth a try.
The issue with shadowRoots is that if they're created with the mode:closed option the observer won't catch them and our code in main.js wont be able to access them or their children after creation. If we don't find them with the observer or initial document parsing and they don't call .play() then we're SoL. By hooking attachShadow and attaching the observer to the shadowRoot's handle at the time of creation we avoid being locked out. There's actually a case the current code doesn't address: if the page has created a closed shadowRoot -before- our code loads that shadowRoot and its children are invisible to us. We could address this using the chrome.dom.openOrClosedShadowRoot API from content.js for our queryShadowSelectorAll function, but then we'd need to move/copy some of the observer & sinkId/event code into content.js to get value from it. If we wanted to do that the function would be something like this:
I don't mind. Originally I had it formatted to please jslint but you should format it however you want to maintain it.
That's fair. It does seem unlikely that someone would have a default audio device set for their workstation and want to use a different default for all browsing. I have no strong feelings one way or another on this change.
I'm not sure I'm a fan of blanket disabling http coverage. I guess it's a layer of security against a targeted MITM from someone who knew the extension was in use but that seems... unlikely. Definitely makes testing less convenient if you're not running SSL on your dev server. I'll defer to you on this as the maintainer/publisher of the version in the Chrome store. Regarding not having an options panel: I was just pondering if we should maybe make one to at least be able to see and remove stored domain preferences. Can't list the devices easily from there but displaying stored data should be doable. I've also been wondering if it's maybe worthwhile to implement something similar to the sinkId change we're doing but for input devices. Not to actually change input devices used by the websites (though we could, I suppose) but to force them to use a non-existent sink or otherwise disable actual access to the microphone even though we granted the permission... |
I'll give it a try, i. e. change the order in
I think I'll just leave the shadowRoot handling as it is at the moment and wait, until we bump into actual cases in the wild which require further actions.
I thought that I'm planning to provide our test page with some additional demo links to YouTube (Music), Spotify, SoundCloud, ... on the new (SSL encrypted)
Assuming that we are currently storing Device Names (possibly edited by the user) and not Device Ids (automatically assigned by the system), I was already thinking about lettting the popup merge-match the stored entries with the actually found device names (grey/disabled row for an unmatched stored value) and have a separate column/marker to indicate which device is currently stored as the default for that domain.
Hmm, I'lll give it a thought. For now I think we should focus on minimizing the number of sites we are creating microphone exceptions for, i. e. to only those for which the user actually picked or stored a non-default device . And set it back to "ask" when the user changes it back to system default (as we already do, I think). To me it seems like we are setting more exceptions than needed ... |
Chrome and Edge both seem to allow it on HTTP with the <all_urls> setting.
That's an excellent idea.
Agreed. Currently if we manually select default from the pop-up it does set it back to ask. I was originally hoping for a more robust logic than that but didn't manage to implement anything better. Related to both points I think it would be logical for setting a domain back to the default device to clear the stored value for that domain.
I like this one best. Still looks good at 16x16 for icon sizing, too. |
@XanSama As you can see, I've pushed a few changes ... Swapping the order of injected scripts did not solve my issue with not intercepting SoundCloud early enough when a new tab starts to autoplay immediately. But since a reload usually fixes it, it's not my top priority at the moment. I have removed the option to store a global Currently, after the extension is loaded and as soon as you open/reload any HTTP(S) site, this site get's an extension managed microphone permission. The merge/matching idea for the popup's device list is still on my table. Maybe I'll also find the reason for those unneeded microphone permissions while I'm on it. Cheers |
Looking good :) At a glance I can't see why we'd be requesting mic permissions when they're not needed but admittedly I'm wiped from Christmas and have picked up a case of COVID so my brain isn't performing at peak. Also I think I'm going to step back and let you run with it from here. I don't even actually use the extension 😅 I just picked it up because I needed an example of how to override sink IDs for a private project. Figured making it work again would be a good crash course in the related API. That is to say... Thank you for all of your work! AudioPick has been an invaluable learning resource for me. Happy New Year and good luck going forward :) |
It was a lot of fun working with you. Glad you got something out of it, too. Best wishes for the future! Thanks a LOT! Happy New Year and Get Better Soon! |
A small change that makes a big difference: 77167ab |
Looks like we should prefix the names of global variables (and functions) we insert into MAIN Example page: https://daily.dev/blog/create-chrome-extension-with-html-css-and-javascript Resolved by commit fab7925 |
Another issue/error may occur when the extension has just been (re-)loaded, This is more likely to happen during development (reload extension), but may also occur when updating the extension from the Chrome Webstore. Resolved by commit 1db7a93 We need to catch the error here: Lines 174 to 180 in 77167ab
|
I've just pushed a major popup / UI Enhancement 1db7a93 ... getting close to release. 🧨 HAPPY NEW YEAR EVERYONE 🧨 |
I've just released version |
Version |
Version Thanks again to @XanSama for rewriting the old Manifest V2 extension for Manifest V3 and making it work again. ❤️ Closing this issue ... |
AudioPick_ManifestV3.zip
I don't feel like submitting a pull request but the attached is a rewritten version of the plugin that is confirmed working with the latest builds of Chrome and Edge (major version 120).
If the author wants they can update the repo/published plugin.
If not this can be extracted and loaded as an unpacked extension as long as developer mode is enabled in the browser.
The text was updated successfully, but these errors were encountered: