Added automation peer class for expander #3504
Conversation
|
Thanks jamesmcroft for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested review from
ghost
added
the
There was a problem hiding this comment.
Hey @jamesmcroft can we move the peers to their own namespace like the platform does?
Thanks!
Microsoft.Toolkit.Uwp.UI.Controls/Expander/ExpanderAutomationPeer.cs
Outdated
Show resolved
Hide resolved
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
|
Hello @RosarioPulella! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@msftbot make sure @michael-hawker has a chance to review before you merge. |
|
Hello @RosarioPulella! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
| /// <summary> | ||
| /// Defines a framework element automation peer for the <see cref="Expander"/> control. | ||
| /// </summary> | ||
| public class ExpanderAutomationPeer : FrameworkElementAutomationPeer, IToggleProvider |
There was a problem hiding this comment.
@jamesmcroft I missed we should have used the IExpandCollapseProvider in addition or instead of maybe? Thoughts?
There was a problem hiding this comment.
Let me take a look, I don't see why not though. Would you want this to be inclusive of IToggleProvider or in favor of?
There was a problem hiding this comment.
@jamesmcroft good question. I don't know if it's a problem to provide multiple interfaces, probably best to include both if possible? I know some other code will probably look for one over the other, so I imagine having both would allow it to be used by the most other code parts looking for certain types of peers. @chingucoding @ranjeshj any thoughts?
Would it make sense to add unit tests like in #3544 as well?
There was a problem hiding this comment.
I don't know if it's a problem to provide multiple interfaces, probably best to include both if possible?
I would argue that having both interfaces only makes sense if you expose both patterns in GetPatternCore. So at least implement what you are providing as part of GetPatternCore. For what it's worth, the WinUI expander control only specifies the ExpandCollapsePattern, so I am inclined to say that that pattern/interface is sufficient.
Would it make sense to add unit tests like in #3544 as well?
Yes, I think testing that peer and the exposed functions of the peer should be done.
There was a problem hiding this comment.
You can have multiple patterns supported for sure. ExpandCollapse pattern made sense for Expander.
There was a problem hiding this comment.
@jamesmcroft if you've got some cycles still to continue helping with this, mind submitting a follow-on PR with the tweaks? Thanks!
There was a problem hiding this comment.
@michael-hawker I'll get another PR open for these changes 😄
6 participants
Fixes #3502
Changes have been made to introduce an
AutomationPeerfor theExpandercontrol to improve the ability for UI testing this control.PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, when trying to access an
Expandercontrol by an Automation ID or other automation property, it is not possible as this is not propagated correctly.What is the new behavior?
A
ExpanderAutomationPeerhas been added which is used by theExpandercontrol in order to surface automation properties correctly.The automation peer not only surfaces up the Automation ID but has been coded so that the toggle state of the control is also available.
Below is the before and after of the UI Automation tree.
Before
After
As you can tell from the change, the ability to traverse the UI Automation tree is vastly improved allowing the
Expander's content to be surfaced correctly instead of separate UI elements.PR Checklist
Please check if your PR fulfills the following requirements: