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.

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 TokenizingTextBox automation peer #3891

Merged
merged 11 commits into from
Aug 12, 2021

Conversation

jamesmcroft
Copy link
Member

@jamesmcroft jamesmcroft commented Mar 25, 2021

Fixes #3910

Introduce an automation peer for the TokenizingTextBox based on a ListViewBaseAutomationPeer with the additional IValueProvider capability to return the value of the autosuggest text box.

This change improves the UI automation tree hierarchy to ensure that the control and its children are nested correctly.

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The TokenizingTextBox does not have an automation peer.

What is the new behavior?

An automation peer has been introduced that improves the interaction of the control through UI automation tooling.

The change includes the ability to interact with the AutoSuggestBox element by getting and setting the text value, and surfacing up through the automation peer.

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)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • 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

Other information

@ghost
Copy link

ghost commented Mar 25, 2021

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, Kyaa-dost and Rosuavio March 25, 2021 21:47
@michael-hawker
Copy link
Member

Thanks @jamesmcroft! We'll check this out with the accessibility tools. It should show up with Narrator, eh? @Kyaa-dost think you could test it out too and compare against the current store based sample app?

@azchohfi I think this would be a candidate to possibly take for 7.0.1 for added value to fix some accessibility? Assuming we can close on it soon and are still waiting on the Notifications and Sample App Caching fixes?

@michael-hawker
Copy link
Member

@jamesmcroft build error:

 D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\TokenizingTextBox\TokenizingTextBoxAutomationPeer.cs(32,21): error SA1623: The property's documentation summary text should begin with: 'Gets a value indicating whether' [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\Microsoft.Toolkit.Uwp.UI.Controls.Input.csproj]

@jamesmcroft
Copy link
Member Author

jamesmcroft commented Mar 26, 2021

@jamesmcroft build error:

 D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\TokenizingTextBox\TokenizingTextBoxAutomationPeer.cs(32,21): error SA1623: The property's documentation summary text should begin with: 'Gets a value indicating whether' [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\Microsoft.Toolkit.Uwp.UI.Controls.Input.csproj]

Apologies @michael-hawker, I copied the comments from the base implementation on that one. Should be sorted now

@michael-hawker
Copy link
Member

I forgot to flag this to 7.0.1 and already kicked it off. We didn't have a tracking issue either, so forgot about that too. We'll still get it reviewed soon and merged though for whichever next release we do.

@jamesmcroft
Copy link
Member Author

I forgot to flag this to 7.0.1 and already kicked it off. We didn't have a tracking issue either, so forgot about that too. We'll still get it reviewed soon and merged though for whichever next release we do.

Sorry that was my bad. I was working through bringing more WCT control support to Legerity.

I chose this one then got blocked so came to put a fix in, completely forgot to raise an item. I'll sort that out and link

@jamesmcroft jamesmcroft linked an issue Apr 1, 2021 that may be closed by this pull request
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.

Tested with narrator, the behavior seems very usable, but I just have a few question.

Co-authored-by: Rosario Pulella <Rosariopulella@gmail.com>
@jamesmcroft
Copy link
Member Author

Tested with narrator, the behavior seems very usable, but I just have a few question.

I think the suggestions were fair. The likelihood of someone using whitespace for these values is probably slim 👍

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.

Thanks @jamesmcroft, it's looking good!

{
[TestClass]
[TestCategory("Test_TokenizingTextBox")]
public class Test_TokenizingTextBox_AutomationPeer : VisualUITestBase
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth adding a test ensuring that the actual list of tokens is also being presented to UIA. Currently, we wouldn't notice if this breaks and as such leaving out crucial information to users using UIA.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies for the huge delay looking into these comments. I've added the additional test for checking the children of the automation peer.

To select the items, I used the SelectAllTokensAndText method on the TokenizingTextBox but noted in the test, this will select an empty text item and append it to the collection.

@michael-hawker
Copy link
Member

@jamesmcroft think you can address @chingucoding's comments?

@michael-hawker michael-hawker added this to the 7.1 milestone Jun 29, 2021
@michael-hawker
Copy link
Member

Thanks @jamesmcroft! @chingucoding this look good to you now? Might make sense to merge this one before your other TTB PR #4101? (though I don't think they conflict)

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.

Looks good to me, thank you for adding the automation peer and making TokenizingTextBox accessible @jamesmcroft ! @michael-hawker I think we can merge this now, this shouldn't interfere with my PR (and if it does, so be it) :)

@michael-hawker michael-hawker merged commit f995c16 into main Aug 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the jamesmcroft/tokenizingtextbox-automationpeer branch August 12, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Improve TokenizingTextBox UI automation support
5 participants