Update the logic about the attribute [RefreshProperties(RefreshProperties.All)] on PropertyGridView.cs

[LeafShi1]

Fixes #12031

Proposed changes

• Removing CommonEditorHide to cancel closing dropdown when performing a refresh on the property page
• In the SelectRow method, when the drop-down box gets the focus, _selectedGridEntry will be null and CommonEditorHide should not be executed.
• When property is in set, cancels the action of setting the focus on property page
• Add Checkbox to Demo Console

Customer Impact

• The property "Auto size" drop-down menu option now works properly by toggling it using the up/down arrow keys

Regression?

• No

Risk

• Minimal

Screenshots

Before

When we navigate to "Auto Size" dropdown using the up/down arrow keys, it is getting auto selected without hitting ENTER key

After

When we navigate to the "Auto-Size" drop-down menu using the up/down arrow keys, the selected menu item toggles normally

Test methodology

• Manually

Test environment(s)

• .net 10.0.0-alpha.1.24519.3

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.66483%. Comparing base (89e6bb9) to head (0e9222e).
Report is 49 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #12356         +/-   ##
===================================================
+ Coverage   75.64669%   75.66483%   +0.01813%     
===================================================
  Files           3119        3141         +22     
  Lines         635392      635615        +223     
  Branches       46933       47014         +81     
===================================================
+ Hits          480653      480937        +284     
+ Misses        151277      151235         -42     
+ Partials        3462        3443         -19     

Flag	Coverage Δ
Debug	75.66483% <100.00000%> (+0.01813%)	⬆️
integration	18.25031% <100.00000%> (-0.03210%)	⬇️
production	49.20606% <100.00000%> (+0.00594%)	⬆️
test	97.03085% <ø> (+0.00395%)	⬆️
unit	46.15639% <83.33333%> (-0.01495%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[Tanya-Solyanik]

I'm not sure about this approach, this attribute was serving a purpose, when AutoSize property is changed, other dependent properties should be updated in the PropertyBrowser. For example Size property might change.

[LeafShi1]

I'm not sure about this approach, this attribute was serving a purpose, when AutoSize property is changes, other dependent properties should be updated in the PropertyBrowser. For example Size property might change.

For the properties that set the value through the drop-down box, I think it is correct not to have this attribute

According to the previous logic (setting this attribute), as long as the option in the drop-down box changes, it thinks that the value you are currently switching is your final option, the drop-down box is folded, and the related attribute values ​​also change. Like this effect

But after removing this attribute, the related property values ​​will only change after selecting and pressing Enter to confirm this value. The modified effect

[merriemcgaw]

This sounds like something we may need to consider in our designing of the PropertyGrid Accessibility work. Is it even acceptable anymore to make changes unannounced like this?

[merriemcgaw]

@LeafShi1 can your team try to find out what properties are changed once you select the new value in the drop down (for each impacted drop down)? Let's get a clear picture of the problem so we can decide if there is an approach that we can find.

@Tanya-Solyanik would refreshing the dependent properties when the "ComboBox" loses focus be an option? That seems like the right time to be refreshing things to me anyway. We probably ought to add an accessibility notification that dependent properties are changing.

[LeafShi1]

@LeafShi1 can your team try to find out what properties are changed once you select the new value in the drop down (for each impacted drop down)? Let's get a clear picture of the problem so we can decide if there is an approach that we can find.

Please refer to the comment #12031 (comment)

[Tanya-Solyanik]

@merriemcgaw

@Tanya-Solyanik would refreshing the dependent properties when the "ComboBox" loses focus be an option? That seems like the right time to be refreshing things to me anyway. We probably ought to add an accessibility notification that dependent properties are changing.

I think this was implemented intentionally to indicate what would happen when the property is changed before changes are committed. But the underlying code is not actually changed yet. I'm not understanding the accessibility impact yet.

@LeafShi1 - the attribute that you removed is a public one. User controls have properties decorated with this attribute, your change is not fixing property grid drop downs for these controls. The right place to fix this would be in the property grid, or TypeDescriptor code where this attribute is read and processed.

[LeafShi1]

@LeafShi1 - the attribute that you removed is a public one. User controls have properties decorated with this attribute, your change is not fixing property grid drop downs for these controls. The right place to fix this would be in the property grid, or TypeDescriptor code where this attribute is read and processed.

Then the expected effect is still that when we use the up/down arrow keys only the item in the dropdown menu is toggled, and it is not selected until the ENTER key is pressed, right?

[merriemcgaw]

Then the expected effect is still that when we use the up/down arrow keys only the item in the dropdown menu is toggled, and it is not selected until the ENTER key is pressed, right?

That would be the behavior I'd like to see. The underlying accessibility issue at hand is that when a user tries to use up/down arrows to cycle between the options in the "combo box" it's automatically accepting the new value before the user does anything to move out of the PropertyGrid field or accept the value. The only time a new property value ought to be committed is when the user takes an action like hitting enter or moving from this field to another one.

[LeafShi1]

That would be the behavior I'd like to see. The underlying accessibility issue at hand is that when a user tries to use up/down arrows to cycle between the options in the "combo box" it's automatically accepting the new value before the user does anything to move out of the PropertyGrid field or accept the value. The only time a new property value ought to be committed is when the user takes an action like hitting enter or moving from this field to another one.

If we have to keep the attribute RefreshProperties.All, we can only change the logic about this attribute in the code. I will try to do this

[Tanya-Solyanik]

Looks good, but please get it tested by Olina's team first.

[LeafShi1]

Looks good, but please get it tested by Olina's team first.

No regression issue found with the private dlls

[Tanya-Solyanik]

@LeafShi1 - please backport this PR to the release/9.0 branch and add a servicing template to the PR description:
1 bug description
2 customer impact
3 regression?
4. testing done
5. risk

[LeafShi1]

/backport to release/9.0

[github-actions]

Started backporting to release/9.0: https://github.com/dotnet/winforms/actions/runs/11695690557

[github-actions]

@LeafShi1 backporting to release/9.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Update the logic about the attribute [RefreshProperties(RefreshProperties.All)] on PropertyGridView.cs
Using index info to reconstruct a base tree...
M	src/System.Windows.Forms/src/System/Windows/Forms/Controls/PropertyGrid/PropertyGridInternal/PropertyGridView.cs
M	src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/DemoConsole/MainForm.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/DemoConsole/MainForm.cs
CONFLICT (content): Merge conflict in src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/DemoConsole/MainForm.cs
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/Controls/PropertyGrid/PropertyGridInternal/PropertyGridView.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Update the logic about the attribute [RefreshProperties(RefreshProperties.All)] on PropertyGridView.cs
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[LeafShi1]

@Tanya-Solyanik A related issue #12440 need to be resolved firstly, after the fixed PR #12431 finished, I will backport these two PRs to the service branch.