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

Report unavailable (disabled) objects during object navigation #15785

Merged
merged 1 commit into from Nov 26, 2023

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Nov 15, 2023

Link to issue number:

Closes #15477

Summary of the issue:

In a GUI, disabled (greyed) items are visible to sighted users. So unless there is a good reason, blind people should be able to see them too. Greyed items convey meaning in a GUI and not being able to see them causes people to miss information conveyed by the GUI.

For example in the new audio panel, if you run NVDA from source (or probably portable), the audio ducking item is greyed out (disabled). Being allowed to find the disabled item informs the user that NVDA has an audio ducking options but that this options can not be enabled.

Unfortunately, disabled controls are ignored during object navigation.

This is also inconsistant with document review mode where disabled objects are not ignored.

Description of user facing changes

During object navigation (complete, simple or flattened), disabled object will not be ignored anymore.

Description of development approach

  • For presentation type, do not classify objects with state UNAVAILABLE to presType_unavailable.
  • For windows isUsableWindow does not return False for windows that are not enabled

Additional notes and questions for the reviewer

  1. Since objects with state UNAVAILABLE are not classified to * For presentation type, do not classify objects with state UNAVAILABLE to presType_unavailable anymore, the name presType_unavailable may be considered confusing. I may change it if you want but this will be an API-breaking change (maybe it's not worth it); moreover, I have no idea of suitable name.
  2. I do not know if there were other reasons for excluding unavailable objects from object navigation. The initial work is in commit 9a3666a. @michaelDCurran any idea why unavailable objects were skipped along with invisible objects?
  3. In commit 62c10a3 (Support for the MSAA implementations of standard IME/TSF candidate lists), source/NVDAObjects/IAccessible/mscandui.py has been modified to discard controlTypes.State.UNAVAILABLE. I do not know the reason why this was done and I wonder if it is still applicable after the modifications of this PR. @michaelDCurran (again), any hint?
  4. In source/NVDAObjects/behaviors.py:"We don't want to handle invisible or unavailable objects" (see Dialog.getDialogText). I have not found dialogs where this method is called to test, but I imagine that I should also remove unavailable object filtering from there?
  5. Should i add an item in API breaking changes paragraph of the change log or is the change log item enough in the Change section? I am asking because changing the object hierarchy may break some add-ons that rely on identifying objects relatively to others.

Testing strategy:

  • Manual tests: As described in Disabled (unavailable) GUI items should not be skipped during object navigation #15477, tested that I can navigate to unavailable objects (disabled NVDA GUI controls, disabled edit field of standard folder name in Explorer's property window). Tests were done with all 3 types of object navigation.
  • Test in alpha to check that there is no excessive unwanted verbosity or any other bugs related to the change of the navigation conditions in the object hierarchy.

Known issues with pull request:

None

Code Review Checklist:

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

@amirmahdifard
Copy link

@CyrilleB79 does this mean that the unavailable state will no longer be missed for the unavailable items in menus anymore? if yes, it is grate! if you don't understand what i mean,
1: install nvda remote addon for example.
2: go to the tools sub menu, remote sub menu, and with your up and down arrow keys, find an item that it says unavailable at the end, such as Mute remote unavailable m when the remote is not connected: then, press your numpad 4 or numpad 6 or numpad 7 or numpad 9, or numpad 1 or numpad 3 depends you want to review it line by line, word by word or character by character, you will only get to see mute remote by using object navigation: firstly, it's shortcut wich is m is not visible in object navigation, and second, it's unavailable state is also not visible in object navigation, only the mute remote is visible in object navigation: the m unavailable at it's end is not fvisible in object navigation
it's absolutely my huge dream that this gets resolved: can you please confurm? thanks a lot!

@CyrilleB79
Copy link
Collaborator Author

Hi @amirmahdifard

It's not very clear to me what you do and expect. Indeed, when using Numpad1/3/4/6/7/9, it is not expected to hear the shortcut key since the shortcut key just belongs to the text "Mute remote": the "M" letter is just underlined visually so that sighted people can see the shortcut.

IMO, to clarify your expectations, it would be useful to open a new issue with the template so that you can describe how you reproduce the issue step by step, what you expect and what you get. If this PR fixes it, I will link it and it will be closed when this PR is merged.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review November 15, 2023 21:31
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner November 15, 2023 21:31
@amirmahdifard
Copy link

@CyrilleB79 the thing i said is a simple thing: if you step on a unavailable item, moving object navigation doesn't announce that this option is unavailable, it just announces that option's name and that's all: it is clear now?

@amirmahdifard
Copy link

@CyrilleB79 or can you give me a build that is .exe file that contanes this pr on it, i will install and confurm it my self if you stil didn't properly understand what i mean. thanks

@CyrilleB79
Copy link
Collaborator Author

@amirmahdifard,, the build of this pR is here.
But you will probably not be able to test easily NVDA remote with it due to its incompatibility with NVDA 2024.1 (even if you override incompatibility).

Also I doubt this fixes your issue, so I suggest you open a specific issue.

@Adriani90
Copy link
Collaborator

Is this information available also when tabbing through dialogs? I mean when moving system focus? If not, this might be worth looking into, in addition to object navigation.

@jcsteh
Copy link
Contributor

jcsteh commented Nov 16, 2023

  1. I do not know if there were other reasons for excluding unavailable objects from object navigation. The initial work is in commit 9a3666a. @michaelDCurran any idea why unavailable objects were skipped along with invisible objects?

I'm not certain, but it's worth noting that entire windows can be unavailable. For example, if there's a modal dialog open, the app window behind that dialog is unavailable. This is a little different to the case of a control being unavailable. In the case of an unavailable window, the window is usually obscured by foreground content in such a way that it's not really visible to sighted users either, even though they can see bits of it in the background. To put it another way, the fact that they can see it is more presentational than semantic. If we care about this, we could make an exception for top level windows and clients.

  1. In source/NVDAObjects/behaviors.py:"We don't want to handle invisible or unavailable objects" (see Dialog.getDialogText). I have not found dialogs where this method is called to test,

The Run dialog is an easy example. Any Win32 dialog should trigger this really.

but I imagine that I should also remove unavailable object filtering from there?

I don't think so. This method is used to get caption text from a dialog. Caption text probably shouldn't be greyed out anyway, but if it is, I don't think it's useful to read that as part of the dialog description. The user can still object navigate if they want to explore it.

  1. Should i add an item in API breaking changes paragraph of the change log or is the change log item enough in the Change section? I am asking because changing the object hierarchy may break some add-ons that rely on identifying objects relatively to others.

Tricky. On one hand, relying on simple review is not ideal for an add-on; it has always had the propensity to be less performant and consistent. On the other hand, the real object hierarchy is pretty complex and we've never given specific guidance that add-ons shouldn't do this. My gut feeling is that this should not be considered an API breaking change, but I can also follow the reverse perspective.

Is this information available also when tabbing through dialogs? I mean when moving system focus? If not, this might be worth looking into, in addition to object navigation.

We don't control system focus. The system or app does. I don't think we should mess with that, and doing so would make things less efficient for the user. Normally, the user doesn't want to interact with unavailable things. This change just gives them a way to perceive them if they want to explore further with the review cursor.

@LeonarddeR
Copy link
Collaborator

I have the feeling that this change should be hidden behind a feature flag for broader testing first.

@amirmahdifard
Copy link

@CyrilleB79 i don't need to test this only with nvda remote, i just used it as an example: i can test it with windows nt services list context menus where stat or stop options are unavailable due to that service is already running or it is stopped.

@amirmahdifard
Copy link

@CyrilleB79 no, it doesn't fixes the issue: i focused on an unavailable option and i read it using the numpad 6 key to look at bottom of that option word by word, and i only heard that option's name: i didn't hear unavailable at the end of it.

@amirmahdifard
Copy link

and also, i noticed a huge problem with this build: it was my first time trying nvda 2024.1 snapshot: windows nt service is inaccessible! my windows is Windows 10 22H2 (AMD64) build 19045.3693
steps: with nvda 2024.1, press windows key to open the search box, and tipe services
klick on the result that says services app,
you will see that inside that menu pressing tab or arrow keys nvda just reads unknown or doesn't speeck anything
so i tryed this unavailable thing somewhere else.

@LeonarddeR
Copy link
Collaborator

and also, i noticed a huge problem with this build:

This is normal as pull requests builds are not signed. They are therefore not allowed to perform administrative tasks.

@CyrilleB79
Copy link
Collaborator Author

@CyrilleB79 no, it doesn't fixes the issue: i focused on an unavailable option and i read it using the numpad 6 key to look at bottom of that option word by word, and i only heard that option's name: i didn't hear unavailable at the end of it.

As asked before (#15785 (comment), #15785 (comment)), please discuss this further in a dedicated new issue. The current PR is not aimed to solve the issue you are describing. Thanks.

@CyrilleB79
Copy link
Collaborator Author

Is this information available also when tabbing through dialogs? I mean when moving system focus? If not, this might be worth looking into, in addition to object navigation.

@Adriani90, what is or is not focusable and reachable with tab/shift+tab is defined by the system or the application, not by NVDA as explained by @jcsteh; it's a matter of system/application design, not NVDA's design. Maybe in some very specific cases of poorly designed application it may be interesting to override the native tab order. In any case, it's not the subject of this PR and should be discussed further in a dedicated ticket or PR. If you open such a ticket, please provide a specific example with context.

@CyrilleB79
Copy link
Collaborator Author

Thanks @jcsteh and @LeonarddeR for your valuable feedback.

  1. I do not know if there were other reasons for excluding unavailable objects from object navigation. The initial work is in commit 9a3666a. @michaelDCurran any idea why unavailable objects were skipped along with invisible objects?

I'm not certain, but it's worth noting that entire windows can be unavailable. For example, if there's a modal dialog open, the app window behind that dialog is unavailable. This is a little different to the case of a control being unavailable. In the case of an unavailable window, the window is usually obscured by foreground content in such a way that it's not really visible to sighted users either, even though they can see bits of it in the background. To put it another way, the fact that they can see it is more presentational than semantic. If we care about this, we could make an exception for top level windows and clients.

I have tested with what I can see with my low vision in notepad, when the modal "Do you want to save changes" dialog is open. The main Notepad window is not obscured but fully greyed out. Moreover, one part of the Notepad main window is hidden by the modal foreground dialog.

Unless there is a strong reason to do so, I am rather reluctant to make a special case for top level windows. Object navigation already allows to navigate to hidden windows, e.g. all the top level windows behind a maximized foreground application window. The fact that the window is greyed out is not a strong argument in my opinion.
All this should be re-discussed more globally however if/when #7759 is implemented.

  1. In source/NVDAObjects/behaviors.py:"We don't want to handle invisible or unavailable objects" (see Dialog.getDialogText). I have not found dialogs where this method is called to test,

The Run dialog is an easy example. Any Win32 dialog should trigger this really.

Thanks for the example. The Notepad's "Do you want to save changes" dialog is another one.

but I imagine that I should also remove unavailable object filtering from there?

I don't think so. This method is used to get caption text from a dialog. Caption text probably shouldn't be greyed out anyway, but if it is, I don't think it's useful to read that as part of the dialog description. The user can still object navigate if they want to explore it.

OK. With your example, I understand better now. I agree with you to continue to to filter a greyed item in the process to determine the dialog's caption. If one day we encounter a problematic dialog with greyed out caption textn which seems very unlikely, we may reconsider this on the basis of a concrete situation.

  1. Should i add an item in API breaking changes paragraph of the change log or is the change log item enough in the Change section? I am asking because changing the object hierarchy may break some add-ons that rely on identifying objects relatively to others.

Tricky. On one hand, relying on simple review is not ideal for an add-on; it has always had the propensity to be less performant and consistent. On the other hand, the real object hierarchy is pretty complex and we've never given specific guidance that add-ons shouldn't do this. My gut feeling is that this should not be considered an API breaking change, but I can also follow the reverse perspective.

Two points:

  1. I think that using simple review relationship between controls was discouraged in some documentation. I do not remember if it was in NVDA dev's guide or in @josephsl's add-on dev guide.
  2. Anyway, this question does not matter since this PR not only modifies simple review mode but also full object navigation, i.e. when simple review mode is off.

I have the feeling that this change should be hidden behind a feature flag for broader testing first.

I am not opposed to this (if NVAccess asks it), provided that the changes are tested in NVDA alpha stage. If 2024.1beta is too near, we may extend the test period to NVDA 2024.2 alpha stage.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 21, 2023
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @CyrilleB79

@seanbudd
Copy link
Member

I don't think the API breaking warning is necessary, to answer (5).
I've requested @michaelDCurran as a review to answer the other 4 questions, otherwise this PR looks fine to me

@michaelDCurran
Copy link
Member

  • Keep filtering unavailable from dialog text.
  • There were some IME candidate lists which inappropriately exposed the unavailable state for their UI. This was overly verbose and semantically wrong of the UI. I would continue to remove it.
  • The primary reason for skipping over unavailable objects in simple review was for unavailable windows. However, as pointed out here, and to me by @seanbudd sighted people can clearly see unavailable (greyed out) controls. Even lists etc. So, I think to best match the sighted experience, we should go with this pr and no longer skip unavailable objects. Two extension of this which we could consider are: 1. continue to skip over unavailable objects with a role of window, so that we do not needlessly navigate into entirely unavailable trees. 2. We could refuse to allow going firstChild from an unavailable object. Thus the user could still navigate to it and have it reported, but not spend time moving within its children. However, I believe that even unavailable lists still visually show their items, so I'm not as convinced of this one.

@CyrilleB79
Copy link
Collaborator Author

@michaelDCurran I hope I correctly match your answers with my questions.

Answer to question 4:

  • Keep filtering unavailable from dialog text.

All OK. I had not done any changes on this anyway since I have already been convinced by @jcsteh's explanation.

Answer to question 3:

  • There were some IME candidate lists which inappropriately exposed the unavailable state for their UI. This was overly verbose and semantically wrong of the UI. I would continue to remove it.

Thanks for this clarification. Anyway, I had not done any change.

Answer to question 2:

  • The primary reason for skipping over unavailable objects in simple review was for unavailable windows. However, as pointed out here, and to me by @seanbudd sighted people can clearly see unavailable (greyed out) controls. Even lists etc. So, I think to best match the sighted experience, we should go with this pr and no longer skip unavailable objects.

OK

Two extension of this which we could consider are:

  1. continue to skip over unavailable objects with a role of window, so that we do not needlessly navigate into entirely unavailable trees.

I do not see the advantage to do so:

  • If you are using a keyboard land on a disabled window with object navigation, you can just press NVDA+pavnum6 to skip this window and explore the next window. You do not need to navigate the subtree of the disabled window.
  • If you are using flat review touch commands, I do not see any use case to explore out of the current foreground top level window.

@michaelDCurran, in which situation would you be forced to navigate the whole subtree of a disabled window?

  1. We could refuse to allow going firstChild from an unavailable object. Thus the user could still navigate to it and have it reported, but not spend time moving within its children. However, I believe that even unavailable lists still visually show their items, so I'm not as convinced of this one.

With my low vision, I seem to see that children of a disabled list are still visible. Confirmation with good eyes needed however since the contrast is very low due to greying.

So @michaelDCurran, do you still expect something from me on this PR?

@michaelDCurran
Copy link
Member

michaelDCurran commented Nov 22, 2023 via email

@XLTechie
Copy link
Collaborator

XLTechie commented Nov 22, 2023 via email

@CyrilleB79
Copy link
Collaborator Author

@CyrilleB79 wrote:

  1. continue to skip over unavailable objects with a role of window, so that we do not needlessly navigate into entirely unavailable trees.

I do not see the advantage to do so: * If you are using a keyboard land on a disabled window with object navigation, you can just press NVDA+pavnum6 to skip this window and explore the next window. You do not need to navigate the subtree of the disabled window.

It is easy to miss the "unavailable" indication. Object nav is tricky enough for new users (and sometimes even experienced users who haven't used it much), without adding the possibility of accidentally moving into an unavailable window tree. Let me ask the question a different way though. Since this PR adds the new behavior of possibly navigating unavailable window trees, which didn't previously exist, the question is what value does this have? If the answer is none, and it is easy to prevent with a simple if check on object type and status, then why shouldn't NVDA prevent such useless navigation from accidentally happening?

First, it is not written very clearly in this discussion, but we actually speak of skipping only disabled top-level windows, not all disabled windows.

First reason to not skip disabled top-level windows is to simplify how object navigation works. It is more consistant to have all objects navigable (enabled and disabled) rather than having all object navigable except in the special case of disabled top-level windows.

Moreover sighted people are able to see disabled windows (moving a partially masking foreground modal window if needed). I do not see a good and strong reason why blind people should not have access to this information.

An example where this could be useful is the following. I take quick notes in various Notepad windows without even bothering to give a file name to it. When I close Windows, I am asked if I want to save the changes. Whenn the "Do you want to save" window is focused, I can use the object navigation to have a quick check on the disabled window behind to see for which instance of Notepad the question is asked.

But more generally, I would say that we should not restrict people's creativity, and thus I do not think that I have to exhibit another specific use case for that if you find the previous one not enough realistic or useful.

At last, regarding the risk to be lost in the object hierarchy, I consider that leaving the foreground window is from far the highest risk to be lost (tracked by#7759). Once you have left the foreground window, either you navigate in enabled or disabled windows does not matter so much.

@XLTechie
Copy link
Collaborator

XLTechie commented Nov 23, 2023 via email

@amirmahdifard
Copy link

@CyrilleB79 hi,
now i understood the work of this pr wich is really nice!
unavailable options can be navigated during object navigation: this is really useful and also makes sence, because we can navigate unavailable options using the arrow keys too anyway.
for example, before this pull request, when we opened notepad, pressed the alt key and pressed the left arrow key once to land on the system sub menu, the restor option in there was unavailable, and we could navigate it and land on it using up and down arrow keys, but it skipped during object navigation. But now it can be navigated and landed when using object navigation, that is, with desktop Keyboard layout, holding numpad 0 and navigating back and forth with numpad 4 and numpad 6 as whell.
it is very nice, thanks so much for it!

@amirmahdifard
Copy link

@CyrilleB79 before this pr, unavailable options were navigated during object navigation only when simple review mode was disabled.

@amirmahdifard
Copy link

@CyrilleB79 i want to know what's difrent between simple review mode disabled or enabled? thanks

@CyrilleB79
Copy link
Collaborator Author

@CyrilleB79 i want to know what's difrent between simple review mode disabled or enabled? thanks

From the dedicated paragraph in the User Guide:

When enabled, NVDA will filter the hierarchy of objects that can be navigated to exclude objects that aren't of interest to the user; e.g. invisible objects and objects used only for layout purposes.

In the future, for questions regarding the current behaviour of NVDA, please think to look at the User Guide or ask on general NVDA mailing list. Thanks.

@seanbudd seanbudd merged commit 746e8fa into nvaccess:master Nov 26, 2023
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Nov 26, 2023
@amirmahdifard
Copy link

@CyrilleB79 ok, really thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled (unavailable) GUI items should not be skipped during object navigation
9 participants