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

Fix iOS control center notification in html5 mode #1530

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bikubi
Copy link
Contributor

@bikubi bikubi commented Nov 19, 2021

This is a fix for #1383. See commit message for how it works.
I've ran all tests & examples successfully.

I've also fixed to build script to work on Linux. It still works on my Mac, too (current macOS, sed from 2017 -- should be fine, I guess).

When html5 is forced, we don't really need to setupAudioContext.
When it's called too early, it unnecessarily creates a Web Audio gain
node which breaks the notification on iOS.
Linux/GNU sed can't handle `-i ''` ("in-place editing with empty filename)
omitting it reads from stdin and works on macOS (BSD) just as well.
Uninen added a commit to Uninen/howler.js that referenced this pull request Dec 14, 2021
@olliekav
Copy link

Any chance of getting this merged?

@goldfire
Copy link
Owner

I'm not able to merge this due to the distribution files being modified. However, I'm still seeing issues with the control center with these changes. Still doing some digging.

@bikubi
Copy link
Contributor Author

bikubi commented Dec 20, 2021

  • I'm happy to remove the build artifacts from the PR, I simply wasn't aware of your contribution procedure
  • What are the issues you're still seeing? I'm curious for my use case and maybe I can help with the digging.

@goldfire
Copy link
Owner

  • I'm happy to remove the build artifacts from the PR, I simply wasn't aware of your contribution procedure
  • What are the issues you're still seeing? I'm curious for my use case and maybe I can help with the digging.

No worries! I need to get around to setting up the guidelines on that through GitHub's contributor features. The build happens when when a new npm version goes out, so no need to do that in the PR.

It might be a separate issue, but if you pause in the control center and then play again the audio can't be heard until you play directly through howler again.

@emilsaj
Copy link

emilsaj commented Mar 2, 2022

@goldfire @bikubi any status of the merging of this? Would be great if it solved the issue!

@curthipster
Copy link

@bikubi Thanks for putting up this PR and for your comments on the related issues.

I was excited to discover this PR, and I found that it gets the media back into the control center! However, I'm running into the same bug that @goldfire describes:

if you pause in the control center and then play again the audio can't be heard until you play directly through howler again.

Your comment on #1262 appears to be related, but that's not fixing the issue for me.

It appears to me that once the audio is paused, the MediaSession isn't firing the play event at all.

@curthipster
Copy link

@goldfire I put together a simple player page that can be used to test the mobile lock screen and control center controls. Once I added in code to configure and handle MediaSession events, I'm not seeing the bugs anymore in my manual tests in this branch. Right now, the code is in the test player, but I wonder if it's a feature that you've considered adding directly to the library?

Can you advise on how best to put up a PR for you to review? One path would be to put up a PR with just the tests, which demonstrate that the controls are not working in the main branch. And then if you rebase this PR onto that, you'll see that they are. Another path would be to put up a PR against this PR branch, but it's not clear to me that I have permission to do that in this repo.

@srividya-sharma
Copy link

@goldfire @bikubi any status update for the merging of this? Facing the same issue of control center notification not appearing in ios.
Thanks in advance.

@satyrius
Copy link

satyrius commented Oct 3, 2022

@goldfire the issues with iOS is still a bother. Can we merge the fix?

@profispojka
Copy link

@goldfire the issues with iOS is still a bother. Can we merge the fix?

Hi. I had forked this lib and merged a few urgent PRs. If you want, make a pr to https://github.com/profispojka/loudest and you will soon find the changes in https://www.npmjs.com/package/loudest

@heijntje
Copy link

Is there any news on merging this @goldfire ? 🤞 I am using the use-sound library which uses howler and have no idea if and how I could fix this manually myself else I would. Even if it is not a full fix, it does not feel like it breaks anything and is a nice addition?

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

9 participants