Skip to content

Refactoring AutomationId at the accessible object classes#6641

Closed
ArtemTatarinov wants to merge 1 commit intodotnet:mainfrom
ArtemTatarinov:dev/Issue_6603_Refactoring_AutomationId
Closed

Refactoring AutomationId at the accessible object classes#6641
ArtemTatarinov wants to merge 1 commit intodotnet:mainfrom
ArtemTatarinov:dev/Issue_6603_Refactoring_AutomationId

Conversation

@ArtemTatarinov
Copy link
Contributor

@ArtemTatarinov ArtemTatarinov commented Feb 8, 2022 •
edited
Loading

Fixes #6603

Proposed changes

  • The AutomationId property is moved to the AccessibleObject base class
  • This property returns null at AccessibleObject
  • It is returned at the GetPropertyValue method of AccessibleObject for the case AutomationIdPropertyId
  • The current implementations in the 4 classes derived from AO are kept the same way, but they are marked as overrides
  • Returning of the Owner.Name is moved to the ControlAccessibleObjectObject class to make it universal for all its descendants
  • All duplicate code with the same logic is cleaned out in the derived classes
  • No unit tests are added yet, but they will be after these changes will be discussed
  • Initial discussion on this topic is here: Moving GetPropertyValue duplicate logic to the base class #6429 (comment)

Customer Impact

  • Tests based on the old implementation of AutomationId could become obsolete

Regression?

  • No

Risk

  • Minimal

Test methodology

  • Unit tests
  • CTI testing

Test environment(s)

.NET SDK 7.0.0-preview-2.22103.2
Microsoft Windows [Version 10.0.19043.1466]

Microsoft Reviewers: Open in CodeFlow

@ArtemTatarinov ArtemTatarinov requested a review from a team as a code owner February 8, 2022 15:06
@ghost ghost assigned ArtemTatarinov Feb 8, 2022
@ArtemTatarinov ArtemTatarinov marked this pull request as draft February 8, 2022 15:14
@ArtemTatarinov
Copy link
Contributor Author

ArtemTatarinov commented Feb 8, 2022 •
edited
Loading

Hi, @Tanya-Solyanik, @dreddy-work, @RussKie, could you please look at this draft PR?

@ArtemTatarinov ArtemTatarinov force-pushed the dev/Issue_6603_Refactoring_AutomationId branch from 5c3f18d to 104a504 Compare February 8, 2022 15:36
@ghost ghost added the draft draft PR label Feb 8, 2022
@ArtemTatarinov ArtemTatarinov force-pushed the dev/Issue_6603_Refactoring_AutomationId branch from 104a504 to e16817c Compare February 11, 2022 09:07
@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Feb 11, 2022

nit: @ArtemTatarinov - wasn't the intention to remove

                    case UiaCore.UIA.AutomationIdPropertyId:
                        return Owner.Name;

from ControlAccessibleObject and replace it with AutomationId property override?

@Tanya-Solyanik Tanya-Solyanik added the waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed label Feb 11, 2022
@ArtemTatarinov ArtemTatarinov force-pushed the dev/Issue_6603_Refactoring_AutomationId branch from e16817c to 5c05707 Compare February 14, 2022 06:20
@ArtemTatarinov
Copy link
Contributor Author

ArtemTatarinov commented Feb 14, 2022

nit: @ArtemTatarinov - wasn't the intention to remove

                    case UiaCore.UIA.AutomationIdPropertyId:
                        return Owner.Name;

from ControlAccessibleObject and replace it with AutomationId property override?

@Tanya-Solyanik You're absolutely right, fixed it.

@ArtemTatarinov ArtemTatarinov force-pushed the dev/Issue_6603_Refactoring_AutomationId branch from 5c05707 to de44669 Compare February 16, 2022 06:45
vladimir-krestov
vladimir-krestov reviewed Feb 18, 2022
View reviewed changes
Copy link
Contributor

@vladimir-krestov vladimir-krestov left a comment

Choose a reason for hiding this comment

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

Looks great! But we need to check if adding AutomationId for all controls is correct.

src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlAccessibleObject.cs Outdated
InitHandle(ownerControl);
}

internal override string AutomationId => Owner.Name;
Copy link
Contributor

@vladimir-krestov vladimir-krestov Feb 18, 2022

Choose a reason for hiding this comment

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

AOs of all controls now will have this behavior, we have to make sure, that it's correct for the rest controls, that weren't implemented.

Copy link
Contributor Author

@ArtemTatarinov ArtemTatarinov Mar 31, 2022

Choose a reason for hiding this comment

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

I've manually tested with the Inspect tool boths control with AutomationId implemented and not implemented and also checked their behavior before the fix. Looks like behavior isn't changing: if we were returning specific AutomationId, then it will still be returned after the fix. And if there were no AutomationId (returning _ => null at the GetPropertyValue method at the AccessibleObject class), then it won't be after the fix too, because our default implementation of the virtual AutomationId property returns null. But maybe it need some further investigation too.

@ArtemTatarinov ArtemTatarinov force-pushed the dev/Issue_6603_Refactoring_AutomationId branch from de44669 to e4e93fb Compare March 30, 2022 13:09
@Tanya-Solyanik Tanya-Solyanik added the tenet-accessibility MAS violation, UIA issue; problems with accessibility standards label Apr 15, 2022
@vladimir-krestov vladimir-krestov removed the waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed label Apr 23, 2022
@dkazennov dkazennov self-assigned this May 19, 2022
@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented May 19, 2022

@dkazennov - what does inspect display when AccessibleObject returns null for AutomationId?

@dkazennov
Copy link
Contributor

dkazennov commented May 20, 2022 •
edited
Loading

@dkazennov - what does inspect display when AccessibleObject returns null for AutomationId?

@Tanya-Solyanik For the testing I commented out the override field AutomationId in the ListViewItemBaseAccessibleObject class and tested ListView control. If AutomationId is null then Inspect will display no AutomationId property at all.
It shows the property and the value if the field is overridden and is not null (a normal case).

As for this PR I've finished all necessary unit tests, but I was being unable to push my changes to ArtemTatarinov/winforms fork. I've asked Vladimir for necessary rights.

@RussKie
Copy link
Contributor

RussKie commented May 20, 2022

You may need to push a the changes into your own fork and open a new PR.

@dkazennov dkazennov mentioned this pull request May 23, 2022
Merged
@dkazennov
Copy link
Contributor

dkazennov commented May 23, 2022

I've finished this PR (added unit tests as requested) in new PR from my fork: #7206

@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Tanya-Solyanik Tanya-Solyanik Awaiting requested review from Tanya-Solyanik

@RussKie RussKie Awaiting requested review from RussKie

@dreddy-work dreddy-work Awaiting requested review from dreddy-work

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon Awaiting requested review from SergeySmirnov-Akvelon

@WPMGPRoSToTeMa WPMGPRoSToTeMa Awaiting requested review from WPMGPRoSToTeMa

1 more reviewer

@vladimir-krestov vladimir-krestov vladimir-krestov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vladimir-krestov vladimir-krestov

@ArtemTatarinov ArtemTatarinov

@dkazennov dkazennov

Labels

draft draft PR tenet-accessibility MAS violation, UIA issue; problems with accessibility standards

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactoring AutomationId at the GetPropertyValue method

5 participants

@ArtemTatarinov @Tanya-Solyanik @dkazennov @RussKie @vladimir-krestov