Fix crash detaching KeyboardBehavior #16
Conversation
Both the handler and the content are null on detach Fixes: davidortinau#13
| _content.KeyUp -= OnKeyUp; | ||
| _content.PreviewKeyDown -= OnPreviewKeyDown; | ||
| _content.PreviewKeyUp -= OnPreviewKeyUp; | ||
| _content = null; |
There was a problem hiding this comment.
@jeremy-visionaid why set content to null? I don't think this behavior should be responsible for that. What if the app is just removing the behavior but the content should remain unchanged?
Setting content to null doesn't seem right, so removing that until further clarification so we can at least fix this crasher.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a null-reference crash when detaching KeyboardBehavior by caching the window’s content element and guarding against null before unsubscribing its key events.
- Introduced a private
_contentfield to holdwindow.Content - Updated
OnAttachedToto assign and subscribe via_content - Added a null check in
OnDetachedFrombefore unsubscribing
Comments suppressed due to low confidence (1)
src/Plugin.Maui.KeyListener/KeyboardBehavior.Windows.cs:14
- [nitpick] The field name
_contentis generic; consider renaming it to_windowContentor_platformContentto clarify its purpose in storing the window's root element.
UIElement? _content;
| var window = bindable.Window.Handler.PlatformView as Microsoft.UI.Xaml.Window; | ||
|
|
||
| window.Content.KeyDown += OnKeyDown; | ||
| window.Content.KeyUp += OnKeyUp; | ||
| window.Content.PreviewKeyDown += OnPreviewKeyDown; | ||
| window.Content.PreviewKeyUp += OnPreviewKeyUp; | ||
|
|
||
| //platformView.KeyDown += OnKeyDown; | ||
| //platformView.KeyUp += OnKeyUp; | ||
| //platformView.PreviewKeyDown += OnPreviewKeyDown; | ||
| //platformView.PreviewKeyUp += OnPreviewKeyUp; | ||
| _content = window.Content; | ||
| _content.KeyDown += OnKeyDown; | ||
| _content.KeyUp += OnKeyUp; | ||
| _content.PreviewKeyDown += OnPreviewKeyDown; | ||
| _content.PreviewKeyUp += OnPreviewKeyUp; |
There was a problem hiding this comment.
Add a null check for window and window.Content before assigning to _content to avoid a potential NullReferenceException. For example:
if (bindable.Window?.Handler?.PlatformView is Microsoft.UI.Xaml.Window win && win.Content is UIElement content)
{
_content = content;
// subscribe events...
}| window.Content.KeyUp -= OnKeyUp; | ||
| window.Content.PreviewKeyDown -= OnPreviewKeyDown; | ||
| window.Content.PreviewKeyUp -= OnPreviewKeyUp; | ||
| if (_content is null) |
There was a problem hiding this comment.
After unsubscribing the events in OnDetachedFrom, consider resetting _content = null; to release the reference and avoid holding onto the UI element longer than necessary.
|
Oh. Haha. Just coming back to this one this morning. I am tired and have a cold so maybe I should just try again on this one... 😅 IIRC In my own solution + app I have behaviours attached to multiple pages, so my approach maybe wasn't reasonable here. I can take another look later if you want. Thanks for merging the other fixes! |
|
@jeremy-visionaid I've asked Matthew on the MAUI team as well about it, and he says '_content = null' resets the local reference only which seems good. If it's working in your scenario with multiple pages then that's confirmation enough for me. I'll return that and merge it after a quick confirmation here. Thanks for the PRs! |
Setting local to null
|
Thanks for being so responsive! Yeah, it's just a local reference, but I don't think the assumption that it's only used with a single element is necessarily valid. I've got some other work to do before I can come back to this one. More concerning to me is that this isn't going to work for my app anyway without fixes upstream like for #14, so if you have contacts in the WinUI/Windows App SDK team you could poke then that would be really appreciated! In the meantime, since I'm in NZ, I'll stage some more changes/fixes ready for your attention tomorrow 😄 |
|
Seems reasonable to go ahead and merge this as-is now. |
Yeah, it's at least not worse than it was even if it's not 100% correct. Thanks! |
2 participants
Both the handler and the content are null on detach
Fixes: #13