Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

New ToolTipClosing, ToolTipOpening attached events and ToolTip.Opened, ToolTip.Closed

Open maxkatz6 opened this issue 2 years ago • 15 comments

What does the pull request do?

public class ToolTip
{
+     public static readonly RoutedEvent ToolTipOpeningEvent;
+     public static readonly RoutedEvent ToolTipClosingEvent;
+     public static void AddToolTipOpeningHandler(Control element, EventHandler<RoutedEventArgs> handler);
+     public static void RemoveToolTipOpeningHandler(Control element, EventHandler<RoutedEventArgs> handler);
+     public static void AddToolTipClosingHandler(Control element, EventHandler<RoutedEventArgs> handler);
+     public static void RemoveToolTipClosingHandler(Control element, EventHandler<RoutedEventArgs> handler);
}

Behavior is ported from WPF, but implementation is different, just like our ToolTip API is already quite different. Most of this decisions were made for consistency with the rest of the framework.

Common:

  1. When ToolTipOpening is raised, it's possible to replace TipProperty, which will cause opening of another tip.
  2. It's also possible to set ToolTipOpening Handled=true, which will essentially cancel tooltip opening.
  3. It's not possible to cancel ToolTipClosing in any way.

Different:

  1. Instead of adding attached events to the ToolTipService, I added them to ToolTip class. Following consistency with other attached ToolTip APIs.
  2. Same about adding new members to the Control class - we never added anything ToolTip (or ContextMenu) related to the Control type itself - everything is attached.
  3. Opened/Closed events are direct-routed in WPF, but CLR events in this PR. Following the same behavior with ContextMenu and Flyout events.

To raise Opening/Closing events, I used IsOpen.Coerse callback, which is also handy to cancel tooltip opening. In WPF these events were only usable via ToolTipService, and handled there. In general, in Avalonia tooltip is completely usable without ToolTipService, so these events should as well.

Checklist

  • [x] Added unit tests (if possible)?
  • [x] Added XML documentation to any related classes?
  • [ ] Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Fixed issues

Fixes #15232

maxkatz6 avatar Apr 24 '24 04:04 maxkatz6

You can test this PR using the following package version. 11.2.999-cibuild0047624-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

avaloniaui-bot avatar Apr 24 '24 04:04 avaloniaui-bot

In case of custom drawn controls, tooltip content may depend on the position of the cursor thus it would be very useful to have a location of the pointer in ToolTipOpeningEvent that is causing the tooltip to show up if it is possible

BAndysc avatar Apr 24 '24 07:04 BAndysc

3. It's not possible to cancel ToolTipClosing in any way.

Is it a technical limitation? I can see uses in preventing tooltips from closing. (Edit: for reference ContextMenu and Flyout have preventable Closing)

MrJul avatar Apr 24 '24 07:04 MrJul

@MrJul I am just following WPF behavior here. Technically, it can be done.

@BAndysc would be a bit tricky to pass position from ToolTipService to ToolTip.

maxkatz6 avatar Apr 24 '24 08:04 maxkatz6

Should we unify the API between all popup-like controls here?

Having all necessary events (Opening, Opened, Closing, Closed) on Popup and re-trigger them on ContextMenu, ToolTip and Flyout. Currently it feels a bit random, and the signature sometimes differ between controls (e.g. CancelEventArgs vs standard event args).

MrJul avatar Apr 24 '24 08:04 MrJul

Should we unify the API between all popup-like controls here?

Having all necessary events (Opening, Opened, Closing, Closed) on Popup and re-trigger them on ContextMenu, ToolTip and Flyout. Currently it feels a bit random, and the signature sometimes differ between controls (e.g. CancelEventArgs vs standard event args).

I'm a big fan of unifying APIs everywhere it's possible. This is a great place to do it IMO.

Also note that Expander is related to this discussion and while the naming is different (Expanding/Collapsing/etc.) usage and API patterns should be the same as the rest.

robloo avatar Apr 24 '24 13:04 robloo

@maxkatz6 The update works well. I was able to dynamically update a tooltip before it displayed.

I agree that having a consistent API for the Opening, Opened, Closing, and Closed events in various places would be ideal.

I'm also usually a fan of using something like e.Cancel to prevent a default behavior. e.Handled is technically there to mean stop routing an event further. The way WPF uses it for ToolTipOpening events is sort of a gray area, since they hijack e.Handled to say whether to cancel the default functionality. One could probably argue either way on that usage.

billhenn avatar Apr 24 '24 16:04 billhenn

@billhenn @maxkatz6

I'm also usually a fan of using something like e.Cancel to prevent a default behavior. e.Handled is technically there to mean stop routing an event further. The way WPF uses it for ToolTipOpening events is sort of a gray area, since they hijack e.Handled to say whether to cancel the default functionality. One could probably argue either way on that usage.

Note that this exact topic was discussed for Expander here: https://github.com/AvaloniaUI/Avalonia/pull/9979#discussion_r1070724369. The agreement was to add CancelRoutedEventArgs to go along with the existing CancelEventArgs establishing Avalonia's convention. We differ here from WPF but it's better for the same reasons you state. All controls should be using this convention -- tooltip as well.

Just keep in mind Avalonia has these cancel event args already and 3rd party libraries should ideally use them as well.

robloo avatar Apr 24 '24 18:04 robloo

@cla-avalonia recheck

jmacato avatar Apr 25 '24 14:04 jmacato

  • [x] All contributors have signed the CLA.

cla-avalonia avatar Apr 25 '24 14:04 cla-avalonia

@cla-avalonia agree

maxkatz6 avatar Apr 26 '24 00:04 maxkatz6

Updated PR:

  1. Reimplemented attached events to not rely on Coerce callback
  2. Changed Opening args type from RoutedEventArgs to CancelRoutedEventArgs
  3. Removed Opened and Closed events. These weren't part of the initial issue, and can be re-added in the future. It's likely involve breaking changes in ContextMenu or Flyout controls, as these have different events. Also, Popup itself wasn't really intended to be cancellable.

Note, Closing event is still not cancellable. If necessary, it can be implemented later.

maxkatz6 avatar May 12 '24 10:05 maxkatz6

You can test this PR using the following package version. 11.2.999-cibuild0048346-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

avaloniaui-bot avatar May 12 '24 11:05 avaloniaui-bot

@maxkatz6 Thanks, I tried the PR build and it does what we needed for the tooltip opening notification.

billhenn avatar May 13 '24 17:05 billhenn

You can test this PR using the following package version. 11.2.999-cibuild0048447-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

avaloniaui-bot avatar May 14 '24 13:05 avaloniaui-bot