Skip to content

Fixed MaskedTextBoxAccessibleObject.Name if Owner.Mask is filled#7983

Merged
Tanya-Solyanik merged 1 commit intodotnet:mainfrom
NikitaSemenovAkvelon:Issue_7938-Fixing_MaskedTextBoxAccessibleObject_Name
Oct 28, 2022
Merged

Fixed MaskedTextBoxAccessibleObject.Name if Owner.Mask is filled#7983
Tanya-Solyanik merged 1 commit intodotnet:mainfrom
NikitaSemenovAkvelon:Issue_7938-Fixing_MaskedTextBoxAccessibleObject_Name

Conversation

@NikitaSemenovAkvelon
Copy link
Contributor

@NikitaSemenovAkvelon NikitaSemenovAkvelon commented Oct 21, 2022

Fixes #7938

Proposed changes

  • Overriden Name property in MaskedTextBoxAccessibleObject.
  • Added new unit tests.

Customer Impact

  • The Name of MaskedTextBoxAccessibleObject is more clarify.

Regression?

  • Yes

Risk

  • Minimal

Screenshots

Before

Screenshot 2022-10-21 121439

After

Screenshot 2022-10-21 120955

Test methodology

  • Manually
  • Unit testing

Accessibility testing

  • Inspect

Test environment(s)

.NET SDK:
Version: 8.0.100-alpha.1.22512.5
Commit: 1b80461e45

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22621
OS Platform: Windows

Microsoft Reviewers: Open in CodeFlow

@NikitaSemenovAkvelon NikitaSemenovAkvelon requested a review from a team as a code owner October 21, 2022 08:46
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7938-Fixing_MaskedTextBoxAccessibleObject_Name branch 2 times, most recently from b07058e to d37ad4a Compare October 21, 2022 12:00
Copy link
Contributor

@dmitrii-drobotov dmitrii-drobotov left a comment

Choose a reason for hiding this comment

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

Looks good, but I would add a comment to clarify the change

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Oct 24, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7938-Fixing_MaskedTextBoxAccessibleObject_Name branch from d37ad4a to 9ce9ec4 Compare October 25, 2022 07:58
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 25, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7938-Fixing_MaskedTextBoxAccessibleObject_Name branch from 6fed3a2 to 27f0e6a Compare October 25, 2022 09:19
}

public Control Owner { get; }
public virtual Control Owner { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about stirring you into a wrong direction. This is a breaking change -

Adding virtual to a member

While this change would often work without breaking too many scenarios because C# compiler tends to emit callvirt IL instructions to call non-virtual methods (callvirt performs a null check, while a normal call won't), we can't rely on it. C# is not the only language we target and the C# compiler increasingly tries to optimize callvirt to a normal call whenever the target method is non-virtual and the this is provably not null (such as a method accessed through the ?. null propagation operator). Making a method virtual would mean that consumer code would often end up calling it non-virtually.

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md#source-and-binary-compatibility-changes

We could add a second property with the same content which would be internal virtual. But it's really best done in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, @Tanya-Solyanik.

We should add a new internal property:

internal virtual Control OwnerInternal { get; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted changes

@Tanya-Solyanik Tanya-Solyanik merged commit d3e866b into dotnet:main Oct 28, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Oct 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2022
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.

For MaskedTextBox with Mask setting, the Name property in Inspect should be empty

4 participants