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 Media Element crash in IOS with Sample app #1568

Merged
merged 7 commits into from
Nov 30, 2023

Conversation

ne0rrmatrix
Copy link
Contributor

@ne0rrmatrix ne0rrmatrix commented Nov 27, 2023

  • Bug fix

Description of Change

Fix Media Element Crash in IOS when going fullscreen or exiting full screen. This issue affect IOS 17.x. Outside the sample app the issue does not appear to be reproducible.

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

Not sure if we want to override OnDisappearing or some else like OnNavigatingFrom? Input would be greatly appreciated. If unloaded event is used it is triggered by going full screen.

@ne0rrmatrix
Copy link
Contributor Author

ne0rrmatrix commented Nov 27, 2023

Do we want to make changes in the documentation and change unloaded event to OnDissapearing? Using unloaded leads to unexpected event when using IOS 17.0.x while setting full screen. Not sure about IOS 16.0.x.

@jfversluis
Copy link
Member

Would you be able to test 16 easily?

@jfversluis jfversluis added the 📽️ MediaElement Issue/PR that has to do with MediaElement label Nov 27, 2023
@ne0rrmatrix
Copy link
Contributor Author

ne0rrmatrix commented Nov 27, 2023

@jfversluis > Would you be able to test 16 easily?

I can test on a simulator. But my Ipad is on IOS 17.x. I would not want to trust my mac mini to behave properly. It is old and runs very slowly. 2014 model year. So not even supported but still running latest. I only use it for building.

@jfversluis
Copy link
Member

Does the crash happen on the Simulator? If yes, testing iOS 16 on the Simulator is good enough for me right now.

@ne0rrmatrix
Copy link
Contributor Author

ne0rrmatrix commented Nov 29, 2023

For future reference. Should we be running our PRs against iOS 16.x and 17.x for testing purposes? I have an iPad running iOS 17 ATM.

@pictos
Copy link
Member

pictos commented Nov 29, 2023

For future reference. Should we be running our PRs against iOS 16.x and 17.x for testing purposes? I have an iPad running iOS 17 ATM.

the ideal would be to test on all supported versions, but we don't have time/budget for that... So testing on just one it's fine, two is amazing

@ne0rrmatrix
Copy link
Contributor Author

ne0rrmatrix commented Nov 29, 2023

@pictos Tested now on IOS 16.4 and 17.x. The fix works for both. Testing against main both IOS version have the issue where when clicking on full screen it unloads and disposes media element. The above fix resolves the issue on both versions. Testing on IOS 17 was on a physical Ipad and IOS 16 was done a mac in a simulator.

@pictos
Copy link
Member

pictos commented Nov 30, 2023

That sounds amazing. @jfversluis can you review this pr? Ill not have access to my mac until Christmas

@jfversluis jfversluis merged commit 4168e5a into CommunityToolkit:main Nov 30, 2023
8 checks passed
@ne0rrmatrix ne0rrmatrix deleted the FixMediaElementSample branch February 12, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📽️ MediaElement Issue/PR that has to do with MediaElement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] MediaElement Crash when entering Full Screen
3 participants