arnoappenzeller / PiPifier

PiPifier is a native macOS 10.12 Safari extension that lets you use every HTML5 video in Picture in Picture mode
MIT License
756 stars 60 forks source link

Replace activate event listener with onfocus #13

Closed kabiroberai closed 7 years ago

kabiroberai commented 7 years ago

For some reason, the activate event listener is not being called due to which the only way to validate the toolbar button when the tab is switched is via the MutationObserver. However, thanks to the checks that I had put in place earlier, the JS would not tell the Swift code to validate it since it had already told it to do so before.

I have fixed this by removing the check entirely, although you should revert this PR if you figure out how to make the activate event listener work.

kabiroberai commented 7 years ago

Forget everything that I said earlier (the solution was as simple as replacing Safari's activate with window.onfocus)

kabiroberai commented 7 years ago

A bit unrelated, but you should change the icon for the YouTube player with Google's own Material Design PiP icon (https://design.google.com/icons/#ic_picture_in_picture_alt)

arnoappenzeller commented 7 years ago

How did you run in problems with the current code?

I tested it quiet a time and it was alright even on tab change. So I lost a bit track of the recent changes 😂

kabiroberai commented 7 years ago

The first issue was that the activate event listener wasn't registering for some reason. I've fixed it by using window.onfocus instead.

Secondly, for some reason the toolbar didn't seem to update if a change happened extremely quickly, and removing the if statement fixed it.

kabiroberai commented 7 years ago

Huh, it seems like the extension doesn't like iframes atm, so I don't think you should merge right now (I'll fix it and tell you when it's done).

kabiroberai commented 7 years ago

I think that the iframe identifies itself as an entirely different SFSafariPage, so I'm trying to figure out how to bypass this. I was thinking that it could check for SFSafariTabs instead. Any thoughts?

arnoappenzeller commented 7 years ago

Do you have an example where it currently won't work with an iFrame embedded video?

kabiroberai commented 7 years ago

Look at this article on macstories.net - the PiP button flickers on and off depending on whether its video is in focus (without the check). If you add back the check it works fine, but it is still unresponsive on some websites.

kabiroberai commented 7 years ago

I'm reopening the PR now, because I fixed the issue (by adding the check back, but tweaking it a bit). Also, I've done a bit more cleanup.

kabiroberai commented 7 years ago

As part of the cleanup, I've also removed the team ID from the UserDefaults suiteName. Adding the team ID only silences the failed to read values warning, and according to Apple the alert is a bug that can be safely ignored.

arnoappenzeller commented 7 years ago

Just merged your PR and added some more features!

Great work!