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

NVDAObjects.UIA: handle UIA notification event if enabled by users #11590

Closed
wants to merge 35 commits into from

Conversation

josephsl
Copy link
Collaborator

@josephsl josephsl commented Sep 10, 2020

Link to issue number:

Closes #10956
Indirectly closes #10626

Summary of the issue:

Currently NVDA announces all UIA notification events. These include update notifications in Microsoft Store, audio volume change announcements from File Explorer, page load messages in Microsoft Edge and many others.

Description of how this pull request fixes the issue:

Add a setting in Object presentation settings panel to toggle UIA notification announcements, to be labeled "report app notifications" (formerly "report accessible event notifications"). This setting is separate from "Report notifications" as toasts and help balloons are not the same thing as UIA notification messages. To communicate this fact, "report notifications" is now a checkable list with two options:

  • Help balloons and toasts
  • App notifications

Note: "app notifications" is provisional. Since the PR touches UIA notification event handler, it might be called "other notifications" for the time being until people submit PR's to apply this setting to alerts from other apps and accessibility API's.

Testing performed:

Tested with Windows 10 App Essentials for a while, as well as:

  1. Announcing UIA notification messages from Edge, Store, and other apps.
  2. Verifying that UIA notifications are not announced when app notifications is turned off.

Known issues with pull request:

The term "app notifications" might be called "other notifications" if suggested.

Change log entry:

New features:

Added a setting in object presentation settings panel to configure announcement of app notifications such as page load progress in Microsoft Edge and audio volume changes in File Explorer. (#10626, #10956)

Changes:
Report notifications setting in object presentation settings panel is now a checkable list that can configure announcement of help balloons and toasts and/or app notifications. (#10626, #10956)

Thanks.

…nvaccess#10956.

Toasts, help balloons and others are not really the same thing as UIA (accessible event) notifications. Therefore add a completley different setting to toggle UIA notification announcements. The key mentions UIA notifications because that's where accessible event messages come from.
…o do so by users. Re nvaccess#10956.

By default, UIA notification events will be handled, but if it isn't, return early from notification event handler.
… checkbox. Re nvaccess#10956.

Separate from 'report notifications' setting, add a checkbox to toggle UIA (accessible event) notification announcement.
…ses earlier than Version 1709 (Fall Creators Update). Re nvaccess#10956.

As UIA notification event (part of IUIAutomation5) was introduced in Version 1709, disable this checkbox if using earlier releases.
@LeonarddeR
Copy link
Collaborator

Interesting approach, it reminds me of a particular changelog 😂.
Anyhow, I think if we change configurability of notifications, we should consider the following:

  1. Regardless of the setting, notifications always get discarded when focus is out of the app window. Why not make this configurable, e.g.

    • Always off
    • When app has focus
    • Always

As can be seen in Narrator, it seems that Microsoft means some notifications to be system global (volume notifications). While i think limiting to foreground app is a sane default, if we provide people configurability to disable them altogether, we should give them the possibility the other way around as well.

@josephsl
Copy link
Collaborator Author

josephsl commented Sep 11, 2020 via email

@feerrenrut
Copy link
Contributor

I'd like to sanity check that this needs to be configurable at all. Windows has an option for "focus assist" which allows users to turn off notifications, but perhaps this does not affect all notifications?

@josephsl
Copy link
Collaborator Author

josephsl commented Sep 11, 2020 via email

@LeonarddeR
Copy link
Collaborator

@josephsl Have you also explored this setting to unregister from uia notification events entirely, rather than discarding them?

@josephsl
Copy link
Collaborator Author

josephsl commented Sep 12, 2020 via email

@LeonarddeR
Copy link
Collaborator

@josephsl Are you intending to address my request (i.e. making this a tristate option that allows you to circumvent the foreground app check)?

@josephsl
Copy link
Collaborator Author

josephsl commented Oct 10, 2020 via email

nvaccess#10956.

In order to support announcing UIA notifications from the foreground (focused) app, change UIA notification announcement setting from a boolean to an option list. The available settings are 'always', 'focused app', and 'off'.
…or is coming from a background app. Re nvaccess#10956.

Do not announce UIA notifications if:
* Notifications are off.
* Notifications are coming from background apps when NVDA is asked to announce notifications from the focused app.
…cations' from a checkbox to a combo box. Re nvaccess#10956.

Suggested by Leonard de Ruijter: add a possibility to limit UIA notification announcement to focused app. To do this, change accessible event notifications control from a checkbox to a combo box. Also, add a dedicated tuple containing settings and labels for UIA notifications combo box and refer to it when setting and retrieving values when saving object presentation settings.
source/NVDAObjects/UIA/__init__.py Outdated Show resolved Hide resolved
source/config/configSpec.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@josephsl
Copy link
Collaborator Author

josephsl commented Oct 12, 2020 via email

@CyrilleB79
Copy link
Collaborator

Known issues with pull request:

When pressing F1 from Object Presentation panel wihle focused on the new setting, the user guide does not open to the newly added paragraph on "Report accessible event notifications".

Yes it does, provided that you have re-built the doc. Tested with Appveyor snapshot build's temp copy.
You should also ensure that no other browser window is open with annother user guide, since I have noted that it may prevent the virtual cursor to jump at the targetted paragraph. Anyway, this issue is not specific to this PR and may appear with any setting control.

…w 'focused app'. Re nvaccess#10956.

Reviewed by Leonard de Ruijter: for backward compatibility, default notification value is focused app - announce notifications coming from the app the user is using at the moment.
nvaccess#10956.

Suggestion from Leonard de Ruijter (subtle optimization): swap focused app and app module check: app modules will be compared if UIA notification setting is set to 'focusedApp', otherwise return early.
…oreground'. Re nvaccess#10956.

Suggestion from Leonard de Ruijter: rename 'focused app' to something more descriptive that informs users that notifications will be announced from focused apps.
LeonarddeR
LeonarddeR previously approved these changes Oct 13, 2020
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
@josephsl
Copy link
Collaborator Author

josephsl commented Oct 13, 2020 via email

@codeofdusk
Copy link
Contributor

codeofdusk commented Oct 30, 2020

Personally, I'd rather see this unified into the "report dynamic content changes" setting, but a ternary option is interesting.

…ification event. Re nvaccess#10956.

UIA notification event should be handled if:
1. App notifications is on.
2. And NVDA is told to announce foreground notifications and the user is using the app in question.
3. Or all app notifications can be announced provided that app notifications is on.
Subclasses can change this behavior in their own UIA notification event handlers.
…ification values, remove 'off'. Re nvaccess#10956.

Rename notification values to app notification values, and remove 'off' due to report notifications UI redesign (next commit).
… list.

Report notifications is now a checkable list with two items:
* Help balloons and toasts: this partially resurrects the old name for this 'checkbox'.
* App notifications: announce notifications from apps.
Two distinct keys are used to check these boxes. Not only this preserves backward compatibility, it also allows other kinds of notifications to be added later, the first of which will be app notifications (chiefly UIA notification event for now but can be expanded to cover other notifications).
…ons' to 'report app notifications' and define an event to enable/disable it.

Rename 'accessible event notifications' to 'report app notifications' to better communicate that these are separate from help balloons and toasts as well as prepare for possibilities other than UIA notification events from apps. Also, as part of report notifications checkable list, define an event handler that will enable/disable app notifications list dpeneindg on checked state of 'app notifications' from report notifications.
…ations/report app notifications.

For report notifications, save individual checkbox states to different keys to preserve backward compatibility.
…tions.

Rename 'report accessible event notifications' to 'report app notifications', as well as describe the new 'app notifications' in report notifications section. In 'report app notifications' section, include an anchor to 'report notifications' section because app notifications checkbox must be checked in order to view app notifications list.
@josephsl
Copy link
Collaborator Author

josephsl commented Feb 6, 2021

Hi,

Hopefully all concerns should be addressed.

Note that I called the second option "app notifications" to make it future-proof (the confspec key is aptly named "reportAppNotifications"). I'm willing to call it "other notifications" for now if folks say it should be that way for the time being (also updated initial PR comment to describe the work so far).

Thanks.

@AppVeyorBot
Copy link

See test results for failed build of commit 03b7cdf5ab

@josephsl josephsl requested review from a team as code owners May 31, 2021 21:42
@seanbudd seanbudd requested a review from feerrenrut June 1, 2021 02:37
@josephsl
Copy link
Collaborator Author

josephsl commented Jun 1, 2021

Hi,

I'm willing to work on this throughout summer, but if folks think it isn't ready, I think it should be closed and/or let someone improve upon what we have so far.

Thanks.

@seanbudd seanbudd added the closed/needs-new-author A new author is required to continue work on the PR label Jun 1, 2021
@seanbudd seanbudd removed their request for review October 4, 2021 22:57
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

UserGuide looks good, thank you!

@CyrilleB79
Copy link
Collaborator

@josephsl, you use "app" instead of "application" in the UI and the User Guide. Is it a common usage among non-technical English speaking people?
I think that Windows speaks of "application", so keeping this term seems clearer and less technical to me, at least regarding French translation.

Anyway, I am not a native English speaker nor someone living in an English speaking country. So please decide by yourself or with native English speaking people if this suggestion is interesting or not.

@josephsl
Copy link
Collaborator Author

josephsl commented Nov 23, 2021 via email

@CyrilleB79
Copy link
Collaborator

Yes. "app" is used also in French for mobile applications but is not so common for desktop applications. And from what I know, "app" is not used in Windows terminology in formal context; it may be used just in informal context.

Thus, I will advocate to use "application" in the French translation, even if English UI uses "app".

This position just regards French translation; feel free to use the most clear and understandable term for English speaking people, "app" or "application".

@josephsl
Copy link
Collaborator Author

josephsl commented Feb 11, 2022

Hi,

2022 update: it appears the nature of UIA notification event is being expanded to cover live text (see microsoft/terminal#12358). If the Terminal prototype (see the linked PR) is adopted, it will means one cannot press NVDA+number row 5 to turn off live text when NVDA reads Windows Terminal output in future Terminal/Windows 11 releases. Also, it appears Windows 11 dev channel build itself will show a flyout when you change volume, and the associated UIA notification display string has also changed. If this trend continues, then I guess we might as well consider UIA notification events to be a way of informing screen readers about screen changes, thereby letting folks press NVDA+5 to also toggle notification event announcement (as suggested in an earlier comment); this would make the PR way simpler with a caveat that apps should have a say when NVDA decides to handle notification event, and in at least one case, what to say and not to say (that's Calculator as UIA notification event is fired when display screen changes).

On the other hand, it can be argued that UIA notification event and live regions/text are different things. The overall purpose of UIA notification event is to inform assistive technologies about what's happening on the screen that aren't necessarily live text. Take Alarms and CLock, for example - when you start or stop stopwatch, a notification event is fired which tells screen readers that the stopwatch has started or stopped. While it may or may not be a live text, notification is still announced by NVDA. In contrast, when loading a page in Microsoft Edge, notification event is fired which informs screen reader users about the page load status with a load progress control shown (or not shown, I can't really verify that at the moment).

Given the update I have, I think it makes sense to revisit this PR in northern summer (July, perhaps), and if Windows Terminal and other apps decide to adopt UIA notification event as the mechanism to inform users about dynamic (screen) content changes, then I would vote to replace this PR with a new PR that will refine dynamic content changes setting to include UIA notifications (with accompanying user guide changes).

Thanks.

@feerrenrut
Copy link
Contributor

@josephsl if your intention is not to go ahead with this PR can you close it, or if you are planning to change the approach please convert to a draft.

@josephsl
Copy link
Collaborator Author

josephsl commented Feb 15, 2022 via email

@josephsl
Copy link
Collaborator Author

Hi,

Reminder to self or anyone: revisit with a new approach in norther in northern summer. Closing.

@josephsl josephsl closed this Feb 15, 2022
@Adriani90 Adriani90 added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Sep 6, 2023
@Adriani90
Copy link
Collaborator

I am adding the abandoned label here, so someone can find this work easier when filtering the coresponding label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. closed/needs-new-author A new author is required to continue work on the PR
Projects
None yet
9 participants