Properly add and remove reflect children

[JeremyKuhne]

The bug was introduced as part of refactoring in #1034 - ordering got flipped with switching as to is.
The confusion was likely caused by the fact that the property setter called its own getter.

Update the code to be more explicit in terminology to hopefully avoid this from happening again.

Fixes #1311
Fixes #1520
Fixes #1607
Fixes #1644

Proposed changes

• Properly add message reflect children

Customer Impact

• Without this NO messages will be sent to child controls- this impacts a huge swath of functionality

Regression?

• Yes : Run dotnet format on src and tests & fix other formatting issues #1034

Risk

• Low

Test methodology

• Extensive debugging, ran automated and manual tests

Test environment(s)

Host (useful for support):
Version: 3.0.0-preview8-28405-07
Commit: d01b2fb7bc

Microsoft Reviewers: Open in CodeFlow

[JeremyKuhne]

I'll look to see what sort of automated tests can be added for this when I recover from investigating the issue. 😴

[RussKie]

Thank you Jeremy!

#1034 in commit 462c242 made the following change:

@@ -3803,9 +3801,8 @@ namespace System.Windows.Forms
                     value.AddReflectChild();
                 }
 
-                Control c = ReflectParent as Control;
                 reflectParent = value;
-                if (c != null)
+                if (ReflectParent is Control c)
                 {
                     c.RemoveReflectChild();
                 }

The orginal code that come from .NET Fx:

                Control c = ReflectParent as Control;
                reflectParent = value;
                if (c != null) {
                    c.RemoveReflectChild();
                }

A significance of the order of instructions isn't particularly obvious.
The fact that a backing field and the property are liberally interchanged without any obiovus reason is concerning.

[codecov]

Codecov Report

Merging #1661 into master will decrease coverage by 0.04871%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##              master       #1661         +/-   ##
===================================================
- Coverage   25.93581%   25.88709%   -0.04872%     
===================================================
  Files            807         807                 
  Lines         267788      267786          -2     
  Branches       37960       37960                 
===================================================
- Hits           69453       69322        -131     
- Misses        193404      193526        +122     
- Partials        4931        4938          +7

Flag	Coverage Δ
#Debug	25.88709% <100%> (-0.04872%)	⬇️
#production	25.88709% <100%> (-0.04872%)	⬇️
#test	100% <ø> (ø)	⬆️

[RussKie]

I've enhanced our manual integration tests and reworded the commit message (hope you don't mind).

💭 I wonder if we can enhance our integration tests to perform actions (like change selected item in a combo) and assert a label has the expected text?
No sure how we can deal with message boxes though, probably remove them altogether...

[RussKie]

Added more manual tests for richtextboxes

[weltkante]

Does this actually fix #1520 as claimed in PR description? There was a stackoverflow article linked in #1631 (resolved duplicate of #1520) which explains "friendly name hyperlinks" as a feature being added in RTF 5.0 and WinForms never being updated to support it. From that description it doesn't sound like being cause by this particular regression, since it already regressed in Desktop Framework long before. If #1520 is solved by this PR then #1631 might need to be revisisted.

[RussKie]

Does this actually fix #1520 as claimed in PR description?

It does.
Please see WinformsControlsTest/RichTextBoxes test example.

[weltkante]

I see, I'd recommend to reopen/unduplicate #1631 then because it probably doesn't get fixed by this PR (can't test easily at the moment, but I added an example to the issue)

[leecow]

tactics approved - preview 9