Make support for search suggestions more generic, supporting Excel comment mentions and Google Chrome location bar suggestions

[michaelDCurran]

Link to issue number:

Fixes #13772
Fixes #13764
Fixes #13522

Summary of the issue:

• When starting to type an @mention in a Microsoft Excel comment, NVDA does not report available suggestions when arrowing through them. ( [Excel- Add @mention]: Screen Reader Users are not Notified when Search Suggestions Appear after Typing "@" in the comment  #13764)
• When typing in the Google Chrome location bar, NVDA does not report rich information for each suggestion, nor does it report when a suggestions control (switch to tb, remove suggestion) is selected. (Chrome: Omnibox: User is not notified of useful information in the first item of keyword search #13522)
• As a developer, adding support for new suggestion scenarios is tedious as specific mappings must be always made.

Description of user facing changes

• NVDA will now report the selected suggestion when arrowing through suggestions when typing an @mention in a Microsoft Excel comment.
• NVDA will report the selected suggestion or suggestion control (switch to tab, remove suggestion) in the Google Chrome location bar.

Description of development approach

Make support for suggestions on editable text fields more generic, allowing it to be supported in more places without having to specifically choose particular overlay classes.

This adds support for reporting @mention suggestions in Microsoft Excel comments, and reporting of suggestions and related controls in the Google Chrome location bar.

Specific changes:

• IAccessible NVDAObject: implement the controllerFor property making use of IAccessible2 relations
• NVDAObject: add an isDescendantOf method which should return True if the object this method is called on is a descendant of the given obj argument. The base method raises NotImplementedError. Efficient implementations for UIA and Ia2Web have been provided.
• Implement event_selection on the base NVDAObject, which reports the object being selected, if the current focus is controlling an ancestor of the selected object. This replaces SuggestionListItem UIA NVDAObject's event_UIA_elementSelected logic. Now it is more generic, and works purely on tree topology, rather than checking the name and appModule etc.
• Remove event_selection implementation from IAccessible NvDAObject as the base handles this now.
• Add an event_controllerForchange method to the base NVDAObject which does nothing.
• UIA NVDAObject: implement event_UIA_controllerforchange to directly call event_controllerforchange, allowing for more generic code, but still backwards compatibility for subclasses to still override event_UIA_controllerforchange if they need to.
• behaviours:
  • Rename EditableTextWithSuggestions to InputFieldWithSuggestions and inherit directly from NVDAObject making this class more generic.
  • InputFieldWithSuggestions class: implement event_controllerForChange which will fire the suggestionsOpened and suggestionsClosed events based on whether there are objects in the controllerfor list. this code was taken from SearchField UIA NVDAObject's event_UIA_controllerforchange.
  • Rename EditableText to EditabletextBase
  • Add a replacement EditableTextWithSuggestions class that inherits from both InputFieldWithSuggestions and EditableTextBase
  • Add a replacement EditableText class which inherits from EditableTextwithSuggestions and EditableTextBase.
    Essentially all this builds suggestion support into EditableText, but keeps the EditableTextWithSuggestionsClass specifically available for compatibility.

Testing strategy:

• Followed steps in [Excel- Add @mention]: Screen Reader Users are not Notified when Search Suggestions Appear after Typing "@" in the comment  #13764
• Followed steps in Chrome: Omnibox: User is not notified of useful information in the first item of keyword search #13522
• Confirmed that suggestions still report in the Windows Start screen when typing content and arrowing down through the suggestions.
• Confirmed that suggestions still report in the Windows Settings screen, when typing content into the Settings search and arrowing down through the suggestions.
• Further testing would be welcome by @josephsl please.

Known issues with pull request:

• Suggestion open and suggestion close sounds are not played for the Google Chrome location bar suggestions as IAccessible2 does not have a controllerForchange event. This feature would be nice, but probably not expected by users anyway.
• UIA_invalidatedLayout event has not been abstracted, thus auto reporting of suggestion count is still only available in specific UIA situations such as Settings search. this could be addressed separately at a later date if possible.

Change log entries:

New features
Changes
Bug fixes

• Suggestions are reported when typing an @mention in in Microsoft Excel comments. ( [Excel- Add @mention]: Screen Reader Users are not Notified when Search Suggestions Appear after Typing "@" in the comment  #13764)
• In the Google Chrome location bar, suggestion controls (switch to tab, remove suggestion etc) are now reported when selected. (Chrome: Omnibox: User is not notified of useful information in the first item of keyword search #13522)
  For Developers

Code Review Checklist:

• Pull Request description:
  • description is up to date
  • change log entries
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• API is compatible with existing add-ons.
• Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• Security precautions taken.

[josephsl]

Hi,

Preliminary test results: works well in People app where contacts are announced automatically (test condition: Windows App Essentials add-on is disabled). I'll take a look at code this weekend.

Thanks.

[josephsl]

Should we mark empty classes as deprecated and targeted for removal in 2024.1? Also, I think it woudl be best to talk about NVDAObject.isDescendantOf and the reorganized class hierarchy in behaviors module somewhere in "changes for developers" section. Thanks.

[josephsl]

By the way, for @jcsteh, do you think if the approach taken in this PR can be adopted for use in Firefox and other Mozilla products? I imagine it will help when reviewing suggestions in Firefox address bar.

CC @derekriemer in case you would like to employ this change in Notepad++ enhancements.

Thanks.

[jcsteh]

Firefox already exposes the controllerFor relation on the address bar. That should mean this already works, but I"m guessing it doesn't based on your comment and the fact that Firefox wasn't mentioned in the PR description.

Perhaps Firefox doesn't fire the selection event on the first item; I'd need to check. If that's true, the reason would be that the list gets shown with the item already selected, so we don't fire the selection event because it would be redundant given that it was always selected since the list came into existence. I can see why that's a problem for NVDA, but I'm also not really sure how to solve it without special casing or causing a bunch of redundant events elsewhere.

That said, per the ARIA spec for autocompletes, Firefox fires focus on the selected item when you use the up and down arrow keys, which would override this new code for items other than the first. I'd love to see this changed, but it can't be changed without breaking backwards compatibility all over the place. I've had an idea for a while to hack around this in NVDA, ignoring those focus events and instead keeping focus on the text box, but that's a much bigger and riskier change.

• Suggestion open and suggestion close sounds are not played for the Google Chrome location bar suggestions as IAccessible2 does not have a controllerForchange event.

It's worth noting that Firefox exposes the collapsed/expanded state on the address bar depending on whether the list is showing and fires a stateChange event when that changes. That seems like a reasonable proxy for the lack of a native controllerForChange event. Chrome doesn't expose the expanded/collapsed state, but it seems like something they might be willing to do.

This feature would be nice, but probably not expected by users anyway.

I'm curious as to why users wouldn't expect this. Is this simply because of the previous state of things or is there some broader reason here? It seems to me that the lack of consistency would be unexpected, particularly for new users.

[seanbudd]

@josephsl

Should we mark empty classes as deprecated and targeted for removal in 2024.1?

The current policy is that if aliases for deprecated objects can remain without a major maintenance cost, then they shouldn't be removed.

[seanbudd]

general approach looks good, only minor fixes/questions raised

[michaelDCurran]

@jcsteh just the previous state of things. I think I'm just trying to say that I wouldn't hold back this pr simply because we can't make the sounds play, that's all.

[josephsl]

Hi,

Update: this PR also resolves #13772. Thanks.

[AppVeyorBot]

• PASS: Unit tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0ju557e9bp0u528j/artifacts/output/nvda_snapshot_pr14222-26764,0d477a32.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 22.6,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.2,
  TEST_START 0.0,
  TEST_END 13.9,
  FINISH_END 0.2
• FAIL: Lint check. See test results for more information.
• PASS: Translation comments check.
• PASS: System tests (tags: installer NVDA).

See test results for failed build of commit 0d477a32af

[michaelDCurran]

@seanbudd have I missed any review actions from you here?

[seanbudd]

frustratingly I noticed my pending review never published

[AppVeyorBot]

• PASS: Translation comments check.
• FAIL: Unit tests. See test results for more information.
• FAIL: Lint check. See test results for more information.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ture8kkf8rubwjdv/artifacts/output/nvda_snapshot_pr14222-26808,5e6e0b3f.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 22.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.1,
  TEST_START 0.0,
  TEST_END 0.8,
  FINISH_END 0.1

See test results for failed build of commit 5e6e0b3f60