Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Added automation peer class for carousel and item #3507
Added automation peer class for carousel and item #3507
Changes from 7 commits
0ccbedc
36235df
2b9bd47
84d9e00
d0c6b6f
7e14dc2
870b743
a706ca6
c9825f1
662e501
411e39d
b959fbf
631f6b2
ba3c81f
f42f236
41491ee
951774d
8cc836f
7c8c126
c544bb0
3b2d965
4dbba32
ddea9ce
79ea866
32b015d
df21fca
7547a4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a Carousel without any selection look like (ignoring empty carousels as there isn't really reasonable to treat that with selection).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this true, an item should always be selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we returning the class name? Narrator should already announce the class/type of the control, having the name of the control be the same makes it more confusing for users relying on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This is the standard that I've seen across a lot of Windows controls and even based this one off the DataGrid within the toolkit.
Do agree though that this could be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I have seen, WinUI 2 controls (mostly) don't do that, they try to get the a name from AutomationProperties.Name or their content; if that fails they return an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good compromise. I'll do some testing with the inspect tool. I don't know what the outcome would be for the name of the control if it is null or an empty string from the automation peer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this to return the name of the control if provided, otherwise it will fallback to the base implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So UIA users can reach a state that isn't reachable through mouse interaction? Or is there a way to unselect an item as user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The
Carousel
doesn't provide the user functionality for unselecting an item. Easily sorted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sorted now, removed the code for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, narrator should already announce the class/type. Announcing it twice doesn't help users much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this also to return the name of the control if provided, otherwise it will look for a
TextBlock
if there is one in the item's template and return that text, otherwise will fallback to the base implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the AutomationProprties.SizeOfSet property is the better way to do this then hardcoding it in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this, I don't think this was actually needed. But I have implemented the size of set and position in set methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesmcroft I didn't see the new size/position code just 'string.Empty' think it's still useful to return that if we have nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, if a name is provided, it will return "My Name 1" which isn't actually what you'd want. The size was added in originally because we were returning the class name so it would have been "CarouselItem 1". Based on the feedback, the class name has been removed so not sure this was needed.
I can always add the class name back in though or if nothing else, return the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not an if case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be changed. Wanted to keep this in line with the other automation peers which were created for the DataGrid in the toolkit so as not to cause any confusion in code styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that an if case would be better in either case since a few interfaces all result in the same behavior. Having a switch case with 4 cases that all do the same (return
this
) looks a bit off in my opinion, but that's just my opinion.