Windows icon indicating copy to clipboard operation
Windows copied to clipboard

Fix: Throw exception if `RangeSelector` min and max args are equal

Open Lamparter opened this issue 1 year ago • 7 comments

Fixes #366

PR Type: Bugfix


This PR fixes an issue where DWM crashes if the arguments for both min and max of the range selector control are equal.

What is the current behavior?

When the Maximum and Minimum arguments of RangeSelector are equal, DWM crashes. This can be reproduced by even the Toolkit gallery app.

What is the new behavior?

If the max and min args of the range selector are equal, an exception will be thrown.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • [x] Based off latest main branch of toolkit
  • [ ] Tested code with current supported SDKs
  • [x] Contains NO breaking changes

Lamparter avatar Dec 01 '24 08:12 Lamparter

Should probably change the gallery app to prevent this as well (with the fix, gallery app would just crash)

sylveon avatar Dec 01 '24 08:12 sylveon

Should probably change the gallery app to prevent this as well (with the fix, gallery app would just crash)

The same would happen in any app. The issue is in the RangeSelector control, it's crashing because it threw an unhandled exception. A fix in the component should be enough.

Arlodotexe avatar Dec 01 '24 16:12 Arlodotexe

But don't you need to handle the exception in the WCT gallery app?

That's what I meant in my message in the dev-chat channel in uwpc

Lamparter avatar Dec 01 '24 17:12 Lamparter

@michael-hawker There seems to be some undefined behavior here. Should the control simply ignore when Min == Max, should it prevent that tick, or something else?

Arlodotexe avatar Dec 01 '24 19:12 Arlodotexe

We should pattern off Slider's behavior here for what we should do. In the base case there, setting min/max to the same value is allowed, and the value is just locked to those values as well:

image

So, I think the best expected behavior is the same here; we should honor that same underlying contract.

Therefore, we should understand what's not working in this scenario to enable that vs. just throwing a different exception.

This would also be worth doing anyway, as we should root-cause the underlying issue; as we should file a platform bug around the DWM crash, as that shouldn't happen regardless. (I wonder if this repros on WinUI 3 as well...)

@Lamparter is this still something you could/would be willing to help us with?

michael-hawker avatar Dec 02 '24 19:12 michael-hawker

Sure!

Lamparter avatar Dec 02 '24 21:12 Lamparter

@Lamparter thanks! Let us know if you need any assistance and how we can help support you.

Once we understand the root cause of what's causing the underlying crash, we can open/raise a platform issue on the WinUI repo too with a minimal repro without the toolkit maybe.

Then, we can hopefully just find a good workaround in our toolkit code here to prevent that scenario from occurring when the edge case bounds are setup, and add a test or two to help us ensure we don't regress in the future.

michael-hawker avatar Dec 02 '24 22:12 michael-hawker