-
Notifications
You must be signed in to change notification settings - Fork 533
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
Update to work with the latest Apple TV #866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling this together! A couple of questions..
inject.js
Outdated
@@ -361,8 +361,7 @@ function defineVideoController() { | |||
p.insertBefore(fragment, p.firstChild); | |||
break; | |||
case location.hostname == "tv.apple.com": | |||
// insert after parent for correct stacking context | |||
this.parent.getRootNode().querySelector(".scrim").prepend(fragment); | |||
// pass-through, the latest version of AppleTV works with default behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this branch? No purpose, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up modifying it so that the hover works too (before it would show and you could use hotkeys but not modify the UI directly)
inject.js
Outdated
@@ -667,7 +667,7 @@ function initializeNow(document) { | |||
); | |||
}); | |||
observer.observe(document, { | |||
attributeFilter: ["aria-hidden"], | |||
attributes: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a heavy performance tax, instead of opting in wholesale, we can provide a list of attributes. Is there an attribute we can target for AppleTV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there is, I'm a little worried about it changing cuz it's kind of random, but yeah the perf tax might be high otherwise
7c07b1b
to
5938a55
Compare
5938a55
to
ef18df7
Compare
@nathanielherman looking at your last commits, this looks good.. Good to merge from your side? |
@nathanielherman testing it locally, the controller is injected successfully — yay. However, can't move it, stacking order? LMK once we're good to merge. |
@igrigorik can't move it? I can hover it and modify it successfully, I'm not familiar with what moving means though |
I'm good to merge, probably won't have time to modify it further if there's more you want to do |
Hi! Has this issue been fixed? Is it finally working with Apple TV+? Thank you! |
@nathanielherman merging, thanks again for your contribution! |
How do I enable it on tv.apple.com? It's still not working. |
@flubbian you have to install the extension directly from the latest github version rather than through the store, that should work |
The old insertion code no longer works, and the change that allows injection into apple-tv-plus-player is a different attribute change than 'aria-hidden'