Add a handler for ListView.SearchForVirtualItem in winforms backend for keyboard navigation in tables and detailed lists

[samtupy]

This introduces the ability to perform first-letter keyboard navigation in tables and lists on windows. In the majority of native lists and tables, it is possible to type the first couple of letters of an item that exists in the list to instantly scroll to that item. As a completely blind computer programmer, this method of list navigation is invaluable to me and other visually impaired computer users when navigating a list containing content we are already somewhat familiar with. I am creating a Toga application with a list containing hundreds of items, and discovered that, though it already works on MacOS with the Voice Over screen reader, I was unable to navigate to list items by letter on windows, making my large list much more difficult to navigate, as now I must use page up/down to hopefully get near the item I'm looking for then use the arrow keys and listen to my screen reader announce items until I find what I want. This confused me as such keyboard navigation is already a built-in feature of these native ListView controls on windows, and so I set out to figure out why this was broken in Toga as this library has a great API and is beautifully accessible to screen readers otherwise and I was hoping to get one of the few accessibility flaws I noticed officially resolved.

It turns out that Toga didn't register a handler for the SearchForVirtualItem event on it's ListView control in the Table widget, which is a requirement for such keyboard navigation to work in lists that are in virtual mode. While the microsoft documentation indicates that this event is used for the ListView.FindItemWithText method / location searching, it does not very clearly indicate that it also enables such first-letter keyboard navigation within the ListView.

This pull request implements a handler for the aforementioned event which, at least on my system, successfully enables keyboard navigation within my large list.

• The handler uses the text of the first column in the table, or self._aliases[0] to check against, which is usually what is wanted at least in an accessibility context.
• The _new_item method in the winforms Table widget class had 2 submethods, text and icon. Since the item text was needed by this handler, I cut the text() function out of the _new_item function and turned it into an _item_text method of the Table class to avoid duplicated code.
• The Microsoft winforms documentation mentions an IsPrefixSearch property in the SearchForVirtualItemEventArgs object which if True should match items if their text starts with that in the event, or else should match items containing text that exactly match that in the event. However when doing keyboard navigation in tables (which requires partial matches) and printing the value of this property, it always seemed to be False. Considering the purpose of this implementation and since I don't think the FindItemWithText method is used by Toga, I opted not to handle this property at present. The IncludeSubItemsInSearch property is similarly left unhandled both do to the fact that it doesn't seem to be needed for the goal of this handler and because Microsoft documentation seems unclear on how/when to implement it.
• This handler only responds to text searches and is a no-op if e.IsTextSearch is False, I have not comprehended the location search API and it was beyond the scope of what I was hoping to accomplish with this fix.
• Most importantly, if it's necessary how should I write a test for this? The biggest issue is that though all core tests pass on my machine, the testbed crashes with some sort of access violation in the webview2 loader. I was honestly not surprised by this, screen reader applications tend to do a lot of process injection to retrieve enough accessibility information to do their work, and with the rate at which windowed processes were launching in the UI testbed I would not be surprised to find out that my screen reader made it choke for any myriad of reasons assuming that something unrelated isn't going on. Regardless I did try to comprehend the testbed/probe setup encase it's needed and understand that perhaps I need to create some sort of test case which spawns a list and calls probe.type_character in combination with assertions to check whether the selection moved where intended? However I'm confused as to how to put that all together. Do I create a new async def test_keyboard_select function in test_table.py that uses the skip_on_platforms function to only run it on windows? Especially since I am having a hard time running the test bed in my environment, I figured it was best to ask here instead of messing with it much myself yet. The code does function in that keyboard navigation works exactly as intended in my tables now, but is untested otherwise.

Thanks for considering this contribution!

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

Thanks for the PR, and the detailed writeup about the why and how this feature works for visually impaired users. It's also great to hear that you've found Toga's accessibility controls helpful (other than this bug/omission, of course!)

FWIW, I consider accessibility to be a first-class feature of Toga. That said, it's also an area where I'm very much aware there's a lot I don't know, so any assistance to fix things that we've missed is most definitely appreciated. I'm definitely motivated to fix any accessibility issues that are reported, and merge any enhancement (like this one) that has an accessibility benefit.

Regarding the specific patch, the implementation itself looks good on a first pass. The only issues I see are with testing.

The fact you've provided a testbed test is amazing - so thanks for that. However, it's also highlighted a gap in testing on Winforms - the test report is showing 3 line coverage misses, and 1 branch coverage miss. I haven't dug into those in detail; I'm guessing they're related to an edge case of the "search" behavior that the current test doesn't exercise.

The second obvious issue is the test failure that has been introduced on macOS. That's an odd one, as I can't see an obvious connection between the change you've made, and what is failing. It sounds like you might have access to macOS for testing purposes; if you're not able to test that, or you can't find the cause, let me know and I'm happy to take a look.

The last issue is a minor style thing - you've added a specific "skip if not windows" check to the test. That's the right idea here, but it's not the approach we take for a feature like this. Rather than hard code "windows" in the test, the preferred approach is to identify an assertion or setup step that is "test specific", and wrap that in a probe method that does a test skip on every platform that doesn't support the feature. That way, the test suite doesn't make an explicit reference to any specific platform, and the backend has a clear indicator for "features that could be added".

An alternative is to add a flag to the probe that indicates whether a feature does/doesn't exist, and raise the skip in the test. The downside to this approach is that it's less clear whether the feature doesn't exist because it's not appropriate for the feature to exist on the platform, or because it's a feature that could be implemented but hasn't been. Based on your explanation of the feature, that might be the right approach here - I can't say I've seen this sort of keyboard navigation on macOS... but it also doesn't sound out of the question as an option.

[samtupy]

Hi,

Thanks for the reply and interest!

As for the MacOS failure, indeed that's odd and I might have a hard time debugging it myself. This CI workflow is the only means I have of looking into that unfortunately, as I have an ARM mac and it seems that test is already passing. I'm not sure what I might have done to break that, as unless I'm mistaken' 100% of the code I added can only execute on windows accept for the beginning of the test function until it checks for platform. If there's something on my end I should try I'd be glad to do so, however that one seems like it might be out of my scope without further direction.

My thoughts on reorganizing the test case. First note is that my experience with these python testing frameworks is very limited right now, so I apologize if I'm not thinking of some basic feature that solves this problem. What you said makes sense about not referencing the single platform the test runs on in the multi-platform testbed. The best idea I came up with here based on what you said is to create a flag in the table probe called has_keyboard_navigation_helper or something like that, and if True, the testbed could call an assert_keyboard_navigation function in the probe or something? Basically the problem here is that this PR doesn't implement the keyboard navigation itself, it just registers an event that allows Windows to do the rest. On MacOS, for example, this already works. If you open a Toga application on a mac with data in a table and you focus on the table, pressing letters on the keyboard will start scrolling your focus to different rows on that table with text that begins with the letter you've typed. Similarly in Finder on a mac when focused on the files table, you should be able to start pressing the first letter of some of your filenames to jump your focus to them at least in most viewing modes. So Toga should never need to do anything on MacOS, or possibly other platforms, for this sort of feature to work out of the box though the implementation details of the feature are very platform specific. Should there be a test case that attempts trying this keyboard navigation on platforms that Toga doesn't do anything to help along in this regard? I'm personally apt to think, though by no means do I consider my self an expert on accessibility APIs with really any platform, that windows is going to be somewhat of an exception here. On most platforms this should just work with no assistance from Toga, indeed I was surprised when I learned about this extra needed event on windows in the process of trying to fix this for Toga E. if Toga's winforms ListView wasn't in virtual mode you'd have to do nothing and this keyboard navigation would just function. Is there a place for tests that are intended to run on just one platform only, preferably within the probe without the outer testbed application expecting them to be available on any other platform or probe / how should I think about this differently? Another way of wording my current thought process is that this seems like an extra implementation detail of the winforms backend specifically rather than an overall feature of Toga that is likely to need multi-platform implementation, which is why I chose the bugfix rather than feature label for this pr's changenote and I am not sure how to properly express this in the test application.

This is a good transition point to the missing statements in the coverage. I suspect this is because I tried to conform to as much of the Microsoft documentation on this event as I could sans the couple of things mentioned in the initial pr comment, and one of those things is the e.Direction property which indicates whether the text search should go backwards or forwards. I tried to implement it for completeness, but A, this first letter keyboard navigation always only searches forward to my knowledge and B, I don't think Toga uses the ListView.FindItemWithText method directly right now anywhere (considering that the event handler that made it work didn't exist until this pr) which is the only other documented way I saw to invoke the event with the search direction set to backwards. This also seems to indicate that the test for this should be moved to the probe instead of left in the outer testbed application if indeed you want me to call ListView.FindItemWithText to test that code branch, or else I could remove that section of code that allows for backwards searching unless Toga has some plan to use the winforms ListView.SearchForVirtualItem event for something else other than this keyboard navigation in the future?

How should I move forward with reorganizing the test given these details?

Thanks!

[freakboy3742]

As for the MacOS failure, indeed that's odd and I might have a hard time debugging it myself.

Looks like it might be an issue with the direction of the wind :-) I've just re-run the test again, and it appears to have passed. It's not a great outcome, but the nature of trying to verify GUI behaviour means we sometimes have weird test failures that aren't entirely reproducible. It's passing now, so let's call that a win.

Basically the problem here is that this PR doesn't implement the keyboard navigation itself, it just registers an event that allows Windows to do the rest.

True - but you can use this to your advantage. If you add an "navigate_with_keyboard()" method on the testbed, that method can call directly to the underlying keyboard handler on platforms that support keyboard navigation, raise a skip on platforms that could support it but don't currently, and raise xfail on platforms where it won't ever work (iOS and android would be the most likely here). Since navigating with the keyboard is one of the first things that the test does, that means the test will skip almost immediately if the platform isn't in a position to test.

On MacOS, for example, this already works. If you open a Toga application on a mac with data in a table and you focus on the table, pressing letters on the keyboard will start scrolling your focus to different rows on that table with text that begins with the letter you've typed.

True, but if we're in a position to confirm this programmatically, it would be desirable to do so. That way, we can confirm that the behavior actually exists, and we haven't inadvertently removed it by adding in an explicit keyboard handler for some other behavior.

That said - if developing that test is prohibitively complex, I'd also be OK with the macOS backend being an XFAIL, with an code comment in the probe to the effect that keyboard navigation is a default behaviour that doesn't need explicit testing.

Is there a place for tests that are intended to run on just one platform only, preferably within the probe without the outer testbed application expecting them to be available on any other platform or probe / how should I think about this differently?

No - but that's somewhat by design. Regardless of the platform, all behaviours of Toga are either:

• Present the platform (implemented as a live test)
• Possible on the platform, but not yet implemented (implemented as a SKIP tests)
• Fundamentally inappropriate for the platform (implemented as an XFAIL test)

For example, Toga's table API allow widgets to appear in cells - although macOS is the only backend that really supports this at present. The winforms backend doesn't currently support this, but in theory it could; so the test for this behavior is skipped, rather than making the test "macOS specific".

On the other hand, the Toga NumberInput has an API to support increments by steps - something the macOS widget supports. The Winforms implementation of NumberInput of the widget doesn't really map to that concept, so the test is marked XFAIL, rather than have a macOS specific test.

The only real case for a "platform specific test" would be something where the platform implementation requires a specific set of conditions that reflect some aspect of that backend's implementation. However, in those cases, it doesn't hurt to run the test on other platforms - it just doesn't expose any potentially problematic behavior.

This also seems to indicate that the test for this should be moved to the probe instead of left in the outer testbed application if indeed you want me to call ListView.FindItemWithText to test that code branch, or else I could remove that section of code that allows for backwards searching unless Toga has some plan to use the winforms ListView.SearchForVirtualItem event for something else other than this keyboard navigation in the future?

While I can understand the intention to add support for potential options cases in future, we only need to implement the features we're actually using. If an uncovered branches exists because of a widget option we're not using, and you couldn't hit those branches unless someone turned on that feature, my inclination would be:

1. Leave the implementation in place, but comment out the actual code
2. Raise a NotImplemented exception if that branch is ever executed, and
3. Mark the branch as # pragma: no cover. This will remove the branch from the coverage report
4. Leave a comment in the code explaining that the branch is to support backwards search, which the widget doesn't currently support... but if it were to, the commented out code is what the implementation will probably look like

That way, we get a safety mechanism if untested code is ever executed, but the background knowledge that you've captured in the implementation won't be lost in the event that we ever need it.

[samtupy]

Hi,

Thanks for the info!

Hopefully I've done a bit closer to what you've requested. I went with having a different method in each probe, mostly because this keyboard navigation works differently per platform and thus a common test case would be difficult. For example, on windows the end key moves to the bottom of a list, and assuming that 2 items start with the same letter, you can tap that letter twice to toggle between them. Where as on MacOS, home and end seem to not move to the top and bottom of the table unfortunately, while tapping a letter twice quickly does not move between 2 items starting with the same letter like windows unless the letter is pressed less than once a second or an arrow key is pressed intermittently. On windows assuming 0 rows are selected, pushing down arrow moves the cursor down one row and selects that, while on MacOS pushing the down arrow simply selects the currently focused row, while then any consecutive presses of down arrow begin moving the cursor. On android with a keyboard available, this keyboard navigation is actually available in some elements, though I have not tested to see whether it is a function of the Android operating system or the Talkback screen reader, I have also not tested a Toga application on Android yet. Nevertheless, it's quirk, or at least one of them, is that there can be scroll lag when navigating that way in large lists. I've not tested this sort of thing on Linux or IOS. Regardless since toga.Table.focus() is currently a no-op (which I'm hoping can be changed at some point - another issue I imagine), I don't know how to implement these in a better way without further guidance since at the beginning of the test, the keyboard focus on the table is questionable. I could probably go on listing other various quirks of this keyboard navigation per-platform but I imagine you get the picture, it would be hard to render any part of this sort of test as truly common, so a different probe method per platform seems like maybe the best way if I've comprehended the code and your input correctly?

On the other hand, this new CI failure with the windows testbed run, AssertionError: assert (50, 128, 200, 0.0) == (50, 128, 200, 1 ? 3.9e-03) and friends, I have no idea about, similar to the MacOS thing it seems to be rather unrelated to my patch at least on the surface and any code I moved around should still execute generally the same I think, not to mention the failing tests seem to be executing before any code I modified did. Is there anything I should do on my end to try resolving that or is it another random failure similar to MacOS?

Thanks!

[freakboy3742]

Hopefully I've done a bit closer to what you've requested. I went with having a different method in each probe, mostly because this keyboard navigation works differently per platform and thus a common test case would be difficult.

I can appreciate the practicality of your solution, but I don't love it. It means each backend is essentially doing its own testing, so you need to make sure each backend is being "sufficiently thorough" in whatever it is testing.

Looking at the tests themselves, though, it seems like there should be a path to a "common" test. The "user behaviours" are all very similar - down one row, down two rows, and so on.

There are two "differences" that I see -

1. The behavior of arrow on the first selection
2. The "wrapping" behavior at the end of the list.

Those are both "properties" of the table that can then be encoded and used in the test.

I image the test would end up looking something like:

test_keyboard_navigation(widget, source, probe):
    await probe.select_second_row_with_keyboard()
    assert widget.selection == source[1]
        
    probe.type_character("a")
    await probe.redraw("'a' pressed")
    assert widget.selection == source[2]

    probe.type_character("a")
    await probe.redraw("'a' pressed again")
    assert widget.selection == source[3]

    probe.type_character("<down>")
    await probe.redraw("'down arrow pressed")
    assert widget.selection == source[4]

    if probe.supports_wraparound_by_keyboard:
        probe.type_character("<end>")
        await probe.redraw("'end pressed")
        assert widget.selection == source[len(source)-1]

        probe.type_character("a")
        await probe.redraw("'a' pressed")
        assert widget.selection == source[0]

That is - do enough "select next row" testing to exercise all the code paths; but also recognise (and encode into the test based on the phenomena they represent) that each platform is slightly different. select_second_row_with_keyboard() accommodates the "how is the first row selected", and also gives the point where Android and Linux tests can be skipped; the end of the test uses a boolean property on the probe to avoid the behavior that doesn't exist on macOS. Does that make sense?

I have also not tested a Toga application on Android yet.

Don't worry too much about the Android table implementation - it's an implementation that technically works, but has a number of issues and needs to be massively rewritten to make it more appropriate for mobile device UX.

I've not tested this sort of thing on Linux or IOS.

There's no Table implementation on iOS, so you've already won there :-)

As for Linux - if you don't have access to a Linux machine (or the motivation to make this work on Linux), feel free to leave this as a skip.

On the other hand, this new CI failure with the windows testbed run, AssertionError: assert (50, 128, 200, 0.0) == (50, 128, 200, 1 ? 3.9e-03) and friends, I have no idea about, similar to the MacOS thing it seems to be rather unrelated to my patch

You're completely correct. That's another random failure - one that's been around long enough it's even been logged as an issue: #2000. Feel free to ignore that one; I've poked CI to re-run that test. It's passed the color test this time, but it still has the coverage failures.

[samtupy]

Ah ok, I see now! Sure, I can modify this to look like that. Luckily I think I just finally got the test coverage failures fixed in the last commit?

[freakboy3742]

Ah ok, I see now! Sure, I can modify this to look like that. Luckily I think I just finally got the test coverage failures fixed in the last commit?

Looks like it - and then a random iOS test failure spoiled the party :-) I've just re-started that test.

[freakboy3742]

Definitely looking good - a couple of minor tweaks and questions about some edge cases, but it's almost ready to land.

[freakboy3742]

Is there a reason to use this call, rather than self.widget.focus()?

[samtupy]

Yes, because as I'd previously mentioned, toga.Table.focus() is currently a no-op for some reason, it's been explicitly disabled in core/src/toga/widgets/table.py

    def focus(self) -> None:
        """No-op; Table cannot accept input focus."""
        pass

I was planning to open an issue about this as I, as well, don't understand why table.focus has been made into a no-op, it makes it so I can't focus my users on a list programmatically which I wanted to do since a giant list was the primary control of my app.

[freakboy3742]

Ah - my apologies - I forgot you'd raised that.

I can't give you a good answer for why it's been disabled; my guess is that there might have been some odd behavior related to selection handling. That's definitely worth a standalone issue (and investigation); if you can fix it, that would definitely be welcome. I can't see any fundamental reason why a table shouldn't be able to be given focus, as long as the behaviour otherwise well defined.

[freakboy3742]

Also - that issue doesn't need to be fixed to land this PR; but it might be worth dropping a comment on the "pseudo-focus" statements highlighting that they could be replaced with self.widget.focus() once the issue is resolved (adding a reference to the new ticket number for that issue).

[freakboy3742]

Minor style tweak - we try to keep comments to ~80 chars, allowing up to 88 at a stretch if it helps with flow.

[samtupy]

Sorry for the delay, this one is stumping me because I very rarely contribute to projects coded by sighted people and thus I haven't run into this restriction before. I'll work out a way to stylize comments such as by writing a program that reads clipboard input, trims all whitespace then reports on the line length so I know whether the comment needs further shortening without spamming the right arrow key 80 times and slowly counting, removing one word then doing it again etc. I won't be able to write comments for Toga's codebase until I've come up with an accessible solution or another blind person tells me one, and none I know who might know are around at the second.

[samtupy]

Sorry for double comment, just remembered to ask. Is this 80 chars per comment or 80 chars per line, I'm guessing per-comment?

[freakboy3742]

The limit is 80 characters per line of code, including any leading space. It's essentially a holdover from old teletype programming - you should be able to see the entire line of code on an 80x25 text mode terminal screen, with no line wrapping. We've now got much larger screens that aren't constrained by 80 columns, but visual readability decreases with line length, so it's now a "soft 80" limit.

The fact that this isn't an consideration for non-sighted programmers is a fascinating insight - thanks for letting me know about that.

As far as automated tooling goes - most sighted IDEs (e.g., Visual Studio) can do an "auto word-wrap" on comments; I'm not sure what your tools will allow. At the very least, the flake8 configuration should be catching this, but I've just noticed that our flake8 configuration sets a line length of 119 (the config is in tox.ini for... reasons), so we're not flagging > 88 character lines. That's something we should probably fix - I've opened #2975 to track this improvement.

In the meantime, I definitely don't want this to be an impediment to your ability to contribute to Toga. If you can't find an automated solution, I'm more than happy to do the last bit of leg work and re-flow long comments. I often end up tweaking comments in PRs anyway (purely for language style and content reasons), so it's not a major imposition.

[mhsmith]

I thought we standardized on 88 rather than 80, for the reasons explained here? It's better to be consistent, so we don't end up in the situation we had before when code was being reformatted every time it moved from one developer to another.

In VS Code I use this extension.

[freakboy3742]

The limit that is hard-enforced is definitely 88; I still think of it as a "soft 80". Make of that what you will 😀

And yes - I use that VSCode tooling as well; the issue is that the PR author is blind, and AFAIK isn't using Visual Studio.

[freakboy3742]

The check for not IsTextSearch makes sense; but we should either add an explicit test for generating this event, or document in the comment that it's difficult to generate under test conditions.

The _accessors and _data checks don't seem right, though - those are under our control. How is it possible to trigger those branches? I guess would be its protection for an empty table. However, I don't think accessors can be empty, and the empty table case is something we can explicitly add a test for.

[samtupy]

I had to use existing code to determine what I should check for. For example on line 209 of winforms table.py, the line if self._accessors: seemed to be an indication that self._accessors could be empty sometimes, so since I had no idea at what random times windows might fire this search event to the ListView control, I opted to check for it. Should I remove that check? As for isTextSearch being false, that happens if ListView.FindNearestItem is called. Should I test for that in the acquire_keyboard_focus function, just call self.native.FindNearestItem an make sure it returns None or something? Toga doesn't currently use this function otherwise, though I can understand wanting to make sure that the event is responded to correctly if winforms fires that event.

[samtupy]

Turns out that FindNearestItem throws an exception if the list isn't in small or large icons view, so indeed this just looks like a difficult branch to hit normally during testing. I don't know how to test an empty list with the current setup, do I make a test function that doesn't receive a source parameter or just make the list myself or something?

[samtupy]

I've found code that clears the table in another test and reused that, so there is now a test that verifies that empty tables do nothing as they should when a keyboard press is applied. Aside from literally calling the new winforms_search_for_virtual_item function itself and constructing an EventArgs object for it with isTextSearch set to false, I don't currently know how to test that statement other than what I've currently done which should at least test the code branch.

[freakboy3742]

I've dug into the if _accessors case on L209 - it's an edge case when all columns are removed from the table. I guess the same edge case should apply to keyboard search (if you haven't got any columns, you can't be searching on row content).

As for the isTextSearch case - there's no need to go to extreme lengths to test a case that shouldn't happen.

However - if you've added a test for the "empty table" case, that should give coverage to that branch, so it should be possible to drop the no-cover.

[freakboy3742]

This is looking really good. I've pushed a small tweak to the comments (mostly for content, but it also addresses one outstanding line length issue), and dropped the no-cover (since it should now be covered by the most recent test).

Thanks for the PR!

[freakboy3742]

Interesting... looks like the no cover is required. That suggests the handler can't be triggered on the "empty table" case... but I guess it doesn't hurt to have the safety catch.

I'll restore the no-cover, and we should be good to land this!