Unregister hooks in ScrollViewerAssist when the ScrollViewer is unloaded

[chris-crowther]

Fixes #3130.

This covers the necessary clean-up of registered Win32 hooks and event handlers for the SupportHorizontalScroll and BubbleVerticalScroll attached properties.

To unregister the HwndSourceHook we have to capture the HwndSource in the Loaded event. I'm happy to consider an alternative approach.

I was tempted to promote the duplicated OnLoaded and OnUnloaded local methods to instance level methods as they are not property-specific.

[nicolaihenriksen]

Sorry for barging in on this code review; I was curious. 😃

This guard is a bit suspicious to me. Why do we only want to listen for the Unloaded event if the ScrollViewer is not already loaded? I think the situation where it IS loaded is equally relevant. This would be a scenario where the AP is not statically set in XAML, but applied at runtime after the ScrollViewer has loaded. I acknowledge this may not be the most common use case, but it is still possible. In short, I think we should handle both the ScrollViewer.IsLoaded=true/false cases.

Another thing to consider is using weak event handlers. Both the Loaded and Unloaded event handlers are currently registered as strong event handlers which unregister themselves when the corresponding event fires. However, I think there are scenarios where this will not work - not particularly related to your changes though:

If the ScrollViewer resides in a TabÌtem, it will be loaded and unloaded multiple times when switching between the tabs. The current implementation only seems to wire up the hooks when the AP changes which means it will register for the Loaded event at startup. However, after navigating away from the TabItem and back to it again, the Unloaded event will fire, removing the hook, and then there is nothing listening for the "second" Loaded event to re-register the hook. Perhaps this could be mitigated by simply using weak event handlers and never unregistering them?

This comment also applies for the changes to the other AP.

Thoughts?

[chris-crowther]

That guard is certainly suspicious! I'm going to step through the code again in the demo app...

[chris-crowther]

@nicolaihenriksen, you're right; this needs to support a variety of scenarios, including:

• Tab items with permanent content in which the ScrollViewer will be loaded and unloaded multiple times
• Tab items with transient content in which the ScrollViewer will be loaded once, unloaded once, and then (hopefully) garbage-collected
• Runtime changes to the attached properties after the ScrollViewer has been loaded

I've pushed some changes which should cover all of the above.

[Keboo]

Awesome. Thank you for putting this together