Skip to content
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

Replace activate event listener with onfocus #13

Merged
merged 13 commits into from Oct 18, 2016

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Oct 16, 2016

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
Copy link
Contributor Author

kabiroberai commented Oct 16, 2016

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

@kabiroberai
Copy link
Contributor Author

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)

@kabiroberai kabiroberai changed the title Removed (il)logcal check Replace activate event listener with onfocus Oct 16, 2016
@arnoappenzeller
Copy link
Owner

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
Copy link
Contributor Author

kabiroberai commented Oct 16, 2016

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
Copy link
Contributor Author

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
Copy link
Contributor Author

kabiroberai commented Oct 16, 2016

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
Copy link
Owner

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

@kabiroberai
Copy link
Contributor Author

kabiroberai commented Oct 17, 2016

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
Copy link
Contributor Author

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 kabiroberai reopened this Oct 17, 2016
@kabiroberai
Copy link
Contributor Author

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 arnoappenzeller merged commit f0093d6 into arnoappenzeller:master Oct 18, 2016
@arnoappenzeller
Copy link
Owner

Just merged your PR and added some more features!

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants