Skip to content

Conversation

@Epica3055
Copy link
Member

@Epica3055 Epica3055 commented Nov 24, 2023

Fix #10343

Related to PR_7349

Cause of this issue

Take a look here this PR_7349 had made
image

Same as this:
https://github.com/dotnet/winforms/pull/7349/files#diff-2cdcad323e7cd83529f6ded1cfd60aa6906ccbb32755221d36cdb67f72b507c0L306-L315

We can see this PR_7349 was to address memory leaks.

Indeed the code here was problematic because if there are no Accessibility Client listening to then there is no need to create AccessibilityObject

        protected override void OnHandleCreated(EventArgs e)
        {
            base.OnHandleCreated(e);
            if (IsHandleCreated)
            {
                // The null-check was added as a fix for a https://github.com/dotnet/winforms/issues/2138
                _dataGridView?.SetAccessibleObjectParent(AccessibilityObject);
            }
        }

The author addressed this problem by this

        protected override void OnHandleCreated(EventArgs e)
        {
            base.OnHandleCreated(e);

            // The null-check was added as a fix for a https://github.com/dotnet/winforms/issues/2138
            if (IsHandleCreated && _dataGridView?.IsAccessibilityObjectCreated == true)
            {
                _dataGridView.SetAccessibleObjectParent(AccessibilityObject);
            }
        }

But this posed another problem, when the handle is created before Accessibility Client is activated, the code here _dataGridView.SetAccessibleObjectParent(AccessibilityObject); never get a chance to be involved.

https://github.com/dotnet/winforms/blob/71aeb2e40dc2d6d7972e86665857c84cc7e8e02e/src/System.Windows.Forms/src/System/Windows/Forms/DataGridView/DataGridView.Methods.cs#L27496C4-L27507

As we can see: the editingControl has no parent.

Regression?

  • Yes

Screenshots

Before

DataGridView_AccessibilityInsightIssue

After

DataGridView_NET7 0

Test methodology

  • manually

Test environment(s)

9.0.0-alpha.1.23572.27

@ghost ghost assigned Epica3055 Nov 24, 2023
@ghost ghost added the draft draft PR label Nov 24, 2023
@Tanya-Solyanik
Copy link
Contributor

@Epica3055 - great fix and great explanation! Please make sure to test it for memory leaks.


// The null-check was added as a fix for a https://github.com/dotnet/winforms/issues/2138
if (_dataGridView?.IsAccessibilityObjectCreated == true)
if (m.MsgInternal == PInvoke.WM_GETOBJECT && IsHandleCreated && _dataGridView?.IsAccessibilityObjectCreated == true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@Epica3055 Epica3055 marked this pull request as ready for review November 28, 2023 03:12
@Epica3055 Epica3055 requested a review from a team as a code owner November 28, 2023 03:12
@ghost ghost removed the draft draft PR label Nov 28, 2023
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Dec 1, 2023
@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Dec 1, 2023

Looks good, but I'm not sure about the shipped/unshipped API file changes. Please look up history of these files to see how we are removing overrides. I believe we are marking them with *REMOVED* string in the unshipped file.

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Dec 1, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 4, 2023
@Tanya-Solyanik
Copy link
Contributor

@Epica3055 - please send these privates to @Olina-Zhang for testing before porting to release/8.0

@Tanya-Solyanik Tanya-Solyanik added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Dec 4, 2023
@Epica3055
Copy link
Member Author

Olina tested this PR, and found there is a memory-leak problem.
I investigated and found that the memory-leak problem actually resulted from this PR:10407.
I also tried PropertyGrid and found there is a memory-leak problem too after this PR:10407.

@Olina-Zhang Olina-Zhang removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Dec 8, 2023
@Epica3055 Epica3055 merged commit 7dd6544 into dotnet:main Dec 13, 2023
@ghost ghost added this to the 9.0 Preview1 milestone Dec 13, 2023
@Epica3055
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/winforms/actions/runs/7189547779

@github-actions
Copy link
Contributor

@Epica3055 backporting to release/8.0 failed, the patch most likely resulted in conflicts:

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

Applying: fix [Accessibility] Accessibility Insights tool keeps loading when mouse-over DataGridView #10343
Using index info to reconstruct a base tree...
M	src/System.Windows.Forms/src/PublicAPI.Unshipped.txt
A	src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewComboBoxEditingControl.cs
A	src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewTextBoxEditingControl.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewTextBoxEditingControl.cs
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewComboBoxEditingControl.cs
Auto-merging src/System.Windows.Forms/src/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/System.Windows.Forms/src/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix [Accessibility] Accessibility Insights tool keeps loading when mouse-over DataGridView #10343
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Dec 13, 2023

Olina tested this PR, and found there is a memory-leak problem. I investigated and found that the memory-leak problem actually resulted from this PR:10407. I also tried PropertyGrid and found there is a memory-leak problem too after this PR:10407.

@Epica3055 - Great investigation, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2024
@Epica3055 Epica3055 deleted the Fix_Issue_10343 branch January 8, 2025 08:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Accessibility] Accessibility Insights tool keeps loading when mouse-over DataGridView

3 participants