dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 982 forks source link

Do not create accessible objects until owner control is created #4390

Closed RussKie closed 3 years ago

RussKie commented 3 years ago

Is your feature request related to a problem? Please describe.

Currently an accessibility object can be created before its owning control is materialised (i.e. have a valid Handle):


// .NET 5.0 sample

var button = new Button();                     // handle is not yet created
button.AccessibilityObject.DoDefaultAction();  // accessible object is created

form.Controls.Add(button);
form.Show();                                   // button.Handle is created

Such design places a lot of responsibility on SDK to constantly check and guard against accessing an owner control and its handle before their are fully created and live.

A lot of work has been done in response to https://github.com/dotnet/winforms/issues/3062, which added a lot of code (e.g. https://github.com/dotnet/winforms/pull/3432), and has a perf impact (as negligible as it may be). However with the steady stream of accessibility-related work there is a significant overhead in implementing guards and rails against accidental handle access.

Describe the solution you'd like and alternatives you've considered

It makes no sense to access an accessibility object before its owner is materialised, i.e. a non existent control can't be navigated to or inspected in any accessibility tools (e.g. Inspect or AI4Win).

We should consider the following:

  1. not creating an instance of accessibility object until its owner is fully materialised and has a valid Handle,
  2. removing all unnecessary null checks and handle checks in ControlAccessibleObject and its descendants
weltkante commented 3 years ago

not creating an instance of accessibility object until its owner is fully materialised and has a valid Handle,

Should the AccessibilityObject return null or throw in this case? (I don't have a particular opinion just wanted to bring up this option.)

RussKie commented 3 years ago

This is a good question, thank you! My initial feeling was to return null, but since it would be a breaking change throwing would be equally appropriate. I expect we will discuss this internally, and welcome external thoughts and opinions as well.

vladimir-krestov commented 3 years ago

I have made a primary investigation, if we can set AccessibleObject as null.

Why do we need it:

Issues:

Conclusion: I don't like this change. I see only negative effects. There are more disadvantages then advantages. My opinion - don't touch that is working. We will bring many problems to customers for the sake of the code clean up and the performance micro improvement.

My proposal: Keep it as is. Add more "IsHandleCreated" checks, that are missed for new implementations made after #3432

//cc: @merriemcgaw

RussKie commented 3 years ago

Thank you.