Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added automation peer class for carousel and item #3507

Merged
merged 27 commits into from
Feb 25, 2021
Merged

Added automation peer class for carousel and item #3507

merged 27 commits into from
Feb 25, 2021

Conversation

jamesmcroft
Copy link
Member

Fixes #3506

Changes have been made to introduce an AutomationPeer for the Carousel and CarouselItem control to improve the ability for UI testing.

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

When using a UI Automation tree tool, it is currently not possible to find the selected item of the Carousel as these properties are not exposed.

What is the new behavior?

A CarouselAutomationPeer and CarouselItemAutomationPeer has been added which is used by their respected control implementation in order to surface more automation properties.

The change includes minor changes to the Carousel and CarouselItem controls exposed as internal for use by the new AutomationPeer classes.

Below is the before and after of the UI Automation tree.

Before

Before

After

After

As you can see from the after screenshot, the change surfaces up the CarouselItem within the UI Automation tree. This allows additional properties to propagate up such as the Selected state.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Sep 27, 2020

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 ghost requested review from michael-hawker, azchohfi and Kyaa-dost September 27, 2020 16:43
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Sep 27, 2020
@jamesmcroft jamesmcroft mentioned this pull request Sep 28, 2020
Closed
@Kyaa-dost Kyaa-dost added this to the 7.0 milestone Sep 29, 2020
@michael-hawker michael-hawker mentioned this pull request Sep 29, 2020
Merged
7 tasks
Rosuavio
Rosuavio approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

Good catch and fix. For the visually impaired this would make the coracle usable.

@ghost
Copy link

ghost commented Nov 10, 2020

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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) and give me an instruction to get started! Learn more here.

michael-hawker
michael-hawker requested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Couple of questions/concerns around the strong reference and why we can't just use SelectedItem from the parent?

Microsoft.Toolkit.Uwp.UI.Controls/Carousel/CarouselAutomationPeer.cs Outdated Show resolved Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/Carousel/CarouselItemAutomationPeer.cs Outdated Show resolved Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/Carousel/Carousel.cs Outdated Show resolved Hide resolved
@michael-hawker michael-hawker mentioned this pull request Nov 10, 2020
Merged
7 tasks
@michael-hawker
Copy link
Member

@jamesmcroft looks like the test passed! 🎉🎉🎉

This all good to go now or is there anything else left you need to do?

@jamesmcroft
Copy link
Member Author

I had a quick skim through and I think everything is good unless there is something obvious I've missed 😅

I'm happy with the solution 👍

@michael-hawker
Copy link
Member

michael-hawker commented Feb 18, 2021 •
edited
Loading

Thanks @jamesmcroft I think we're good on our end once you've resolved @chingucoding's comments.

@CommunityToolkit CommunityToolkit deleted a comment Feb 18, 2021
@michael-hawker
Copy link
Member

@msftbot merge once @chingucoding approves.

@ghost
Copy link

ghost commented Feb 18, 2021

Hello @michael-hawker!

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:

  • I'll only merge this pull request if it's approved by @chingucoding

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".

@michael-hawker
Copy link
Member

michael-hawker commented Feb 23, 2021 •
edited
Loading

@jamesmcroft want to reply/address @chingucoding's comments and we can close this out? 🙂

@chingucoding mind resolving any items that are now all good for the comments?

@marcelwgn
Copy link
Contributor

@chingucoding mind resolving any items that are now all good for the comments?

Unfortunately, GitHub doesn't let me resolve my comments, only something the PR author can do apparently :(
Only comment that needs addressing in my opinion is #3507 (comment)

@jamesmcroft
Copy link
Member Author

Unfortunately, GitHub doesn't let me resolve my comments, only something the PR author can do apparently :(

How extremely odd. I left them open because I didn't want to resolve your feedback 😞

@marcelwgn
Copy link
Contributor

How extremely odd. I left them open because I didn't want to resolve your feedback 😞

That's very kind and thoughtful of you, thank you! 😊

marcelwgn
marcelwgn approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

Thank you for adding the peers and making Carousel accessible! Looks good to me.

@marcelwgn
Copy link
Contributor

CI failed @jamesmcroft, can you take a look?

@jamesmcroft
Copy link
Member Author

@chingucoding I think something changed with the Carousel when I've merged in. I will take a look and get this sorted 😄

@marcelwgn
Copy link
Contributor

@michael-hawker I think the bot gave up on this PR can you merge this for us?

@michael-hawker
Copy link
Member

Thanks @jamesmcroft and @chingucoding for being so thorough! This'll be a great example for us to template off of in the future!

@michael-hawker michael-hawker merged commit a5c7662 into master Feb 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the jamesmcroft/3506-carousel-automation branch February 25, 2021 17:51
@ghost ghost added the Completed 🔥 label Feb 25, 2021
@marcelwgn
Copy link
Contributor

Thank you @jamesmcroft for being so patient with this and thank you @michael-hawker for adding the test infrastructure for the new tests!

@jamesmcroft
Copy link
Member Author

An absolute pleasure to both of you!

Apologies that it has taken so long though. Definitely will be more on top of my PRs in the future 😅

@marcelwgn
Copy link
Contributor

Don't worry about that @jamesmcroft, there was some test infrastructure missing to get the tests up and running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranjeshj ranjeshj ranjeshj left review comments

@michael-hawker michael-hawker michael-hawker approved these changes

@marcelwgn marcelwgn marcelwgn approved these changes

@Rosuavio Rosuavio Rosuavio left review comments

@azchohfi azchohfi Awaiting requested review from azchohfi

@Kyaa-dost Kyaa-dost Awaiting requested review from Kyaa-dost

Assignees
No one assigned
Labels
accessibility ♿ auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.

Carousel control doesn't expose Selection in UI Automation tree
6 participants
@jamesmcroft @michael-hawker @marcelwgn @Rosuavio @ranjeshj @Kyaa-dost