Skip to content

Conversation

@rladuca
Copy link
Member

@rladuca rladuca commented Oct 23, 2019

On behalf of @Simon-IT :

This is related to my issue 642. In the hit testing by geometry algotithm, the order in which the transformations are crosses is wrong. In the issue 642 there is a project to test in very simple manner the problem.

3.1 port related to #1761

@ghost ghost requested a review from vatsan-madhavan October 23, 2019 23:52
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Oct 23, 2019
@ghost ghost requested a review from SamBent October 23, 2019 23:52
@rladuca rladuca added * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) Bug Product bug (most likely) labels Oct 23, 2019
@rladuca rladuca added this to the 3.1 milestone Oct 23, 2019
@SamBent
Copy link
Contributor

SamBent commented Oct 24, 2019

Related to #642

5.0 PR: #1761

Description

When using a DrawingVisual to render lightweight graphics, pushing two transforms causes hit-testing to fail, if the two transforms don't commute (e.g. a scale and a translate).

Customer Impact

User clicks (or moves mouse) over a drawing created as above, but doesn't get the expected response. Or user moves/clicks mouse and gets the response for a different part of the drawing.

Regression

No. This bug has been lurking since WPF's initial release (.NET 3.0, 2006).

Risk - Medium

The fix is in HitTestWithGeometryDrawingContextWalker.PushTransform, and affects all subsequent calls to that class. This is an internal class, but the method is an override of a public base class (DrawingContext). The normal usage is internal: when a hit-test result is requested, internal code creates an instance of the class, asks it to walk the drawing instructions, and extracts the result. This usage is low-risk. The only difference visible to the public is that hit-test queries now return the correct result. It's difficult to imagine an app relying on the incorrect result; to do so would require intimate knowledge of the transforms being applied, essentially re-implementing the entire hit-testing pipeline.

A greater risk is an app that never noticed the hit-test results were wrong. Now the app will react when it didn't before. User's clicks and moves may incite behavior that the app never tested. An app that goes to the trouble of declaring behavior related to this kind of hit-testing should have noticed that it wasn't working, but oversights like this sometimes happen.

There's also a risk with an app that interacts directly with the internal class, using the public methods of the public base class DrawingContext. The app could easily see different behavior. However this presumes that the app can obtain a reference to the internal instance. I don't see any obvious way for that to happen, but I'm not 100% sure it can't.

Workarounds

Hit-testing works correctly if the app combines the transforms correctly in the original drawing instructions, replacing two calls of PushTransform with one. But this is awkward to do when the app builds the drawing from separate components, each of which may contribute its own transform. This is the customer's situation.

@vatsan-madhavan vatsan-madhavan added auto_merge bot-command and removed * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) labels Oct 29, 2019
@ghost
Copy link

ghost commented Oct 29, 2019

Hello @vatsan-madhavan!

Because this pull request has the auto_merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 186371b into dotnet:release/3.1 Oct 29, 2019
@rladuca rladuca deleted the transform3.1 branch December 10, 2019 22:10
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto_merge bot-command Bug Product bug (most likely) PR metadata: Label to tag PRs, to facilitate with triage Servicing-approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants