Windows icon indicating copy to clipboard operation
Windows copied to clipboard

SettingsExpander in SettingsExpander crashes the application

Open naumenkoff opened this issue 1 year ago • 4 comments

Describe the bug

If you define a SettingsExpander as an item within another SettingsExpander, the application crashes.

<controls:SettingsExpander Header="Parent">
    <controls:SettingsExpander.Items>
        <controls:SettingsExpander Header="Child" />
    </controls:SettingsExpander.Items>
</controls:SettingsExpander>

Steps to reproduce

1. Add a `SettingsExpander` to the layout.  
2. Add another `SettingsExpander` as a nested item inside the first `SettingsExpander`.  
3. Run the application.  
4. Observe that the application crashes.

Expected behavior

The application should not crash.

  • Automatically hide/not render the nested SettingsExpander.
  • Alternatively, the build process could prevent such layout from compiling/add hint.
  • Explicitly marking nested SettingsExpander usage as prohibited in the gallery documentation.

Screenshots

No response

Code Platform

  • [ ] UWP
  • [x] WinAppSDK / WinUI 3
  • [ ] Web Assembly (WASM)
  • [ ] Android
  • [ ] iOS
  • [ ] MacOS
  • [ ] Linux / GTK

Windows Build Number

  • [ ] Windows 10 1809 (Build 17763)
  • [ ] Windows 10 1903 (Build 18362)
  • [ ] Windows 10 1909 (Build 18363)
  • [ ] Windows 10 2004 (Build 19041)
  • [ ] Windows 10 20H2 (Build 19042)
  • [ ] Windows 10 21H1 (Build 19043)
  • [ ] Windows 10 21H2 (Build 19044)
  • [ ] Windows 10 22H2 (Build 19045)
  • [ ] Windows 11 21H2 (Build 22000)
  • [x] Other (specify)

Other Windows Build number

Windows 11 24H2 (Build 26100)

App minimum and target SDK version

  • [ ] Windows 10, version 1809 (Build 17763)
  • [ ] Windows 10, version 1903 (Build 18362)
  • [ ] Windows 10, version 1909 (Build 18363)
  • [ ] Windows 10, version 2004 (Build 19041)
  • [ ] Windows 10, version 2104 (Build 20348)
  • [x] Windows 11, version 22H2 (Build 22000)
  • [ ] Other (specify)

Other SDK version

No response

Visual Studio Version

Preview

Visual Studio Build Number

17.12.0 Preview 5.0

Device form factor

Desktop

Additional context

CommunityToolkit.WinUI.Controls.SettingsControls: 8.1.240916

Help us help you

No, I'm unable to contribute a solution.

naumenkoff avatar Nov 16 '24 09:11 naumenkoff

@niels9001 this isn't something I think we ever considered with the Settings pattern, eh?

I would guess it's related to #302 as SettingsExpander expects SettingsCards as children, and SettingsExpander itself (while looking and using a Card, isn't one itself).

Is this a special case we should allow? Or is this an odd design pattern we want to discourage?

@naumenkoff can you share the scenario for what you were trying to achieve?

michael-hawker avatar Nov 20 '24 17:11 michael-hawker

@niels9001 this isn't something I think we ever considered with the Settings pattern, eh?

I would guess it's related to #302 as SettingsExpander expects SettingsCards as children, and SettingsExpander itself (while looking and using a Card, isn't one itself).

Is this a special case we should allow? Or is this an odd design pattern we want to discourage?

@naumenkoff can you share the scenario for what you were trying to achieve?

These are just my observations 🙃 I don't think that this is a bad thing, because everything has its own limitations and it would be good to mention it somewhere (Gallery?) (I didn't find such information)

naumenkoff avatar Nov 20 '24 21:11 naumenkoff

Yeah, this isn't a pattern I have seen before across Windows. I can imagine it might come with some accesibility issues as well when nesting different expanders?

@naumenkoff do you have a specific UI in mind with the code you provided?

@michael-hawker @Arlodotexe is there a straightforward way to throw a warning or something else when not using a SettingsCard?

niels9001 avatar Nov 20 '24 21:11 niels9001

Yeah, this isn't a pattern I have seen before across Windows. I can imagine it might come with some accesibility issues as well when nesting different expanders?

@naumenkoff do you have a specific UI in mind with the code you provided?

@michael-hawker @Arlodotexe is there a straightforward way to throw a warning or something else when not using a SettingsCard?

No, sorry, I don’t have a UI in mind with this approach because I don’t like deep nesting. I actually stumbled upon this completely by accident during a refactor when I wanted to group several SettingsCards (the first being a ToggleSwitch and the second a TextBox) under a single SettingsExpander and accidentally replaced them both with a SettingsExpander.

Image

naumenkoff avatar Nov 20 '24 21:11 naumenkoff