fix(skia): make UIElement.HitTest strictly stronger than UIElement.IsViewHit #17948
Conversation
github-actions
bot
added
the
area/skia ✏️
ramezgerges
force-pushed
the
skia_shapes_hittesting
branch
from
d9aa625
to
fa0f21a
Compare
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17948/index.html |
| // 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17948/index.html |
ramezgerges
force-pushed
the
skia_shapes_hittesting
branch
from
4e1734e
to
1f2f658
Compare
github-actions
bot
removed
the
area/skia ✏️
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17948/index.html |
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17948/index.html |
| if (elementHitTestVisibility == HitTestability.Visible && renderingBounds.Contains(testPosition)) | ||
| #endif | ||
| { | ||
| // UIElement.HitTest should be a stronger condition than elementHitTestVisibility == HitTestability.Visible |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Not sure if it's the case so far, but the Visual could be aware if the background is a pixel has not been painted, or not (even if it has been painted using with transparent color). If so, the nullelementHitTestVisibility == 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().
|
|
||
| return element.Background != null; | ||
| } | ||
| internal override bool IsViewHit() => ((IBorderInfoProvider)this).IsViewHit(); |
There was a problem hiding this comment.
This may change behavior on non-Skia. It was already not correct behavior, but now will be incorrect in a different way, I think.
There was a problem hiding this comment.
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.
GitHub Issue (If applicable): followup to #16201
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
Internal Issue (If applicable):