Skip to content

Teams: No longer inappropriately focus reaction menus when conversation messages are focused.#14363

Merged
michaelDCurran merged 2 commits into
betafrom
i14355
Nov 12, 2022
Merged

Teams: No longer inappropriately focus reaction menus when conversation messages are focused.#14363
michaelDCurran merged 2 commits into
betafrom
i14355

Conversation

@michaelDCurran

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #14355
Broadens approach from pr #11822 which was to fix #11821.

Summary of the issue:

When a conversation message is focused in Microsoft Teams (E.g. arrowing up and down a threaded conversation list) Teams displays a reaction menu along side the focused message. As Chromium therefore fires a menu popupStart event, NVDA handles this by faking focus on the menu itself
This behaviour has been historically necessary for win32 context menus that fail to focus the first item in the menu.
However, In Teams this is very disruptive.

Description of user facing changes

NVDA will no longer get stuck in a menu when arrowing up and down threaded conversations in Microsoft Teams.

Description of development approach

Broaden the approach taken in pr #11821 by suppressing handling of menu popupStart for any element in teams with 'menu' in its xml-roles attribute. Previously NVDA would also check for very particular message container classes on the parent's parent. But as Teams keeps moving these, this is no longer managable.

Testing strategy:

Performed steps in #14355. I.e. arrowed up and down a threaded conversation, ensuring each message was read and was not interuppted by a menu.
Ensured that pressing enter on the message still allowed the user to interact with the reaction menu when its items truely got focus.
Smoke tested other context menus in Teams such as the menu on the tab control and the search field.

Known issues with pull request:

This more broad approach may mean there could mean that NVDA won't announce "menu" for a menu that appears that does not focus one of its items until the user presses an arrow key. However, having checked all menus that I know, and waying up the slight confusion of NvDA saying nothing on a menu until pressing a key verses the real focus such as for a message being interuppted all the time by an incorrect menu, I feel this approach is by far the best choice for the user.

Change log entries:

New features
Changes
Bug fixes

  • NVDA will no longer get stuck in a menu when arrowing up and down threaded conversations in Microsoft Teams.
    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.

…ent handling as Teams keeps changing the layout of problematic menus. See #11821 and #14355.
@michaelDCurran michaelDCurran requested a review from a team as a code owner November 10, 2022 23:10
seanbudd
seanbudd previously approved these changes Nov 11, 2022
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 381781adef

LeonarddeR
LeonarddeR previously approved these changes Nov 11, 2022

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given its positive impact, could we have this on beta instead?

@michaelDCurran michaelDCurran changed the base branch from master to beta November 12, 2022 07:21
@michaelDCurran michaelDCurran dismissed stale reviews from LeonarddeR and seanbudd November 12, 2022 07:21

The base branch was changed.

@michaelDCurran

michaelDCurran commented Nov 12, 2022

Copy link
Copy Markdown
Member Author

@LeonarddeR agreed. I actually just forgot to set the base branch in the pr correctly. Thanks for catching. Fixed now. The commits themselves were correctly already on top of beta.

@michaelDCurran michaelDCurran merged commit a7ca449 into beta Nov 12, 2022
@michaelDCurran michaelDCurran deleted the i14355 branch November 12, 2022 07:26
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Nov 12, 2022
@michaelDCurran michaelDCurran modified the milestones: 2023.1, 2022.4 Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants