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.

Sign up for GitHub

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

Jump to bottom

fix(skia): make UIElement.HitTest strictly stronger than UIElement.IsViewHit #17948

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+24 −21
Draft

fix(skia): make UIElement.HitTest strictly stronger than UIElement.IsViewHit #17948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb671f9
fix(skia): make UIElement.HitTest strictly stronger than UIElement.Is…
ramezgerges Aug 15, 2024
1f2f658
chore: build error
ramezgerges Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Uno.UI/Microsoft/UI/Xaml/Controls/ScrollPresenter/ScrollPresenter.Uno.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ partial void InitializePartial()
private protected override ShapeVisual CreateElementVisual() => Compositor.GetSharedCompositor().CreateBorderVisual();
#endif

internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit();

Brush IBorderInfoProvider.Background => Background;

BackgroundSizing IBorderInfoProvider.BackgroundSizing => BackgroundSizing.InnerBorderEdge;
Expand Down
12 changes: 1 addition & 11 deletions src/Uno.UI/UI/Xaml/Controls/Border/Border.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,17 +322,7 @@ protected override void OnBackgroundChanged(DependencyPropertyChangedEventArgs e

internal override bool CanHaveChildren() => true;

internal override bool IsViewHit() => IsViewHitImpl(this);

internal static bool IsViewHitImpl(FrameworkElement element)
{
_Debug.Assert(element is Panel
|| element is Border
|| element is ContentPresenter
);

return element.Background != null;
}
internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may change behavior on non-Skia. It was already not correct behavior, but now will be incorrect in a different way, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could guard this with #if __SKIA__, but the change makes sense in general I think, so I would say it's better to change the behaviour on non-skia targets even if it's still not accurate.


#if !UNO_HAS_BORDER_VISUAL
private void UpdateBorder() => BorderRenderer.Update();
Expand Down
2 changes: 2 additions & 0 deletions src/Uno.UI/UI/Xaml/Controls/Border/IBorderInfoProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,6 @@ internal partial interface IBorderInfoProvider
#if UNO_HAS_BORDER_VISUAL
BorderVisual BorderVisual { get; }
#endif

bool IsViewHit() => Background != null || BorderBrush != null;
}
2 changes: 1 addition & 1 deletion src/Uno.UI/UI/Xaml/Controls/ContentPresenter/ContentPresenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1383,7 +1383,7 @@ protected override Size MeasureOverride(Size size)

internal override bool CanHaveChildren() => true;

internal override bool IsViewHit() => Border.IsViewHitImpl(this);
internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit();

/// <summary>
/// Registers the provided native element in the native shell
Expand Down
2 changes: 2 additions & 0 deletions src/Uno.UI/UI/Xaml/Controls/Page/Page.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ private void UpdateBorder()
}
#endif

internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit();

protected internal virtual void OnNavigatedFrom(NavigationEventArgs e) { }

protected internal virtual void OnNavigatedTo(NavigationEventArgs e) { }
Expand Down
2 changes: 1 addition & 1 deletion src/Uno.UI/UI/Xaml/Controls/Panel/Panel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ private protected void OnBackgroundSizingChangedInnerPanel(DependencyPropertyCha
#endif
}

internal override bool IsViewHit() => Border.IsViewHitImpl(this);
internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit();

#if !UNO_HAS_BORDER_VISUAL
private void UpdateBorder() => _borderRenderer.Update();
Expand Down
6 changes: 6 additions & 0 deletions src/Uno.UI/UI/Xaml/Controls/Popup/PopupPanel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,10 @@ internal override bool IsViewHit()

return Popup?.IsLightDismissEnabled ?? false;
}

// This shouldn't be needed, but due to the way we implement Popups, we might
// need to make Popups not hit-testable even if the PopupPanel has a Background.
#if __SKIA__
internal override bool HitTest(Point point) => IsViewHit();
#endif
}
Comment on lines +300 to 305
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really feeling comfortable with this. I'd personally prefer if we still keep the HitTestability.Visible check in SearchDownForTopMostElementAt. Then at some point we can try to remove IsViewHit completely, at least on Skia. That's kinda my personal preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree that is weird ... why we cannot rely on the Visual here?

Also in all cases, please remove the #if __SKIA__. Even if not used on other platforms yet, the code should be shared to allow easier maintenance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on Visual here is probably buggy due to bugs in popup implementation itself that will need to be their own separate work. In this case, IsViewHit can return false while the visual can actually hit test. This basically makes me more convinced into keeping the check in the VisualTreeHelper as done with #17973. My opinion here is that #17973 is enough for now. It's not worth the hassle around trying to force HitTest to be a stronger condition.

For now, the only place we know it's not stronger is PopupPanel, and this PR forces it to be strongly by basically just calling IsViewHit.

(btw, if we proceeded with this PR, it will be a better idea to return IsViewHit() && base.HitTest(point) I think)

17 changes: 9 additions & 8 deletions src/Uno.UI/UI/Xaml/Media/VisualTreeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,12 +501,6 @@ internal static (UIElement? element, Branch? stale) SearchDownForTopMostElementA
clippingBounds = clippingBounds.IntersectWith(transformToElement.Transform(clip)) ?? default;
}
TRACE($"- clipping (absolute): {clippingBounds.ToDebugString()}");

// The region where the current element draws itself.
// Be aware that children might be out of this rendering bounds if no clipping defined.
// This is expressed in the window (absolute) coordinate space.
var renderingBounds = transformToElement.Transform(new Rect(new Point(), element.LayoutSlotWithMarginsAndAlignments.Size)).IntersectWith(clippingBounds) ?? Rect.Empty;
TRACE($"- rendering (absolute): {renderingBounds.ToDebugString()}");
#else
// First compute the transformation between the element and its parent coordinate space
var matrix = Matrix3x2.Identity;
Expand Down Expand Up @@ -662,12 +656,19 @@ internal static (UIElement? element, Branch? stale) SearchDownForTopMostElementA
// We didn't find any child at the given position, validate that element can be touched,
// and the position is in actual bounds(which might be different than the clipping bounds)
#if __SKIA__
if (element.HitTest(transformToElement.Inverse().Transform(testPosition)) && renderingBounds.Contains(testPosition))
if (element.HitTest(transformToElement.Inverse().Transform(testPosition)))
#else

if (elementHitTestVisibility == HitTestability.Visible && renderingBounds.Contains(testPosition))
#endif
{
// UIElement.HitTest should be a stronger condition than elementHitTestVisibility == HitTestability.Visible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UIElement.HitTest just does Visual hit testing, which isn't aware of whether the UIElement's background is NULL or not. Aren't we creating transparent background in both cases? Also, what if the user gets the Visual and modifies the background, in this case they are not the same. I tend to think we should really restore elementHitTestVisibility == HitTestability.Visible check

Copy link
Member

@dr1rrb dr1rrb Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's the case so far, but the Visual could be aware if the background is null a pixel has not been painted, or not (even if it has been painted using with transparent color). If so, the elementHitTestVisibility == HitTestability.Visible check would indeed only be about what hit-testability we push to the native element and therefor should be less restrictive than the IsViewHit().

// On WASM we match the native-element hit-testability (i.e. the `pointer-events` css property) to the
// HitTestVisibility of the corresponding managed element, so if we implement UIElement.HitTest for WASM
// in the future, this assertion makes sure that we're not regressing. If at some point
// elementHitTestVisibility != Visible but element.HitTest returns true, then this would break WASM
// hit-testing because `pointer-events` will be `none`, but element.HitTest is true.
global::System.Diagnostics.Debug.Assert(elementHitTestVisibility == HitTestability.Visible);

TRACE($"> LEAF! ({element.GetDebugName()} is the OriginalSource) | stale branch: {stale?.ToString() ?? "-- none --"}");
return (element, stale);
}
Expand Down
Loading