Skip to content

Work around broken Data validation dropdown lists in Microsoft Excel#17042

Merged
seanbudd merged 7 commits into
betafrom
i15138
Aug 27, 2024
Merged

Work around broken Data validation dropdown lists in Microsoft Excel#17042
seanbudd merged 7 commits into
betafrom
i15138

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented Aug 22, 2024

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #15138

Summary of the issue:

If a cell in Microsoft Excel has a data validation type of list, NvDA does not report anything when it is opened and or when selecting a value.
Note that this is specifically talking about the list shown when the cell has data validation set. The dropdown list shown when auto completing from values in higher cells does report okay.

Technical info

  • The list is a SysListView32 control.
  • It has a native UIA implementation.
  • NvDA generally does not trust UIA implementations for SysListView32 as they have been somewhat limited.
  • Therefore NVDA falls back to MSAA for the list.
  • Although UIA core is proxying the UIA events to MSAA events, the UIA implementation seems to be incomplete as AccessibleObjectFromEvent is always returning E_FAIL for any focus win event fired by the control.
  • Even if NvDA is told to specifically use UIA for this control, there is a loop in the control's parent chain, causing a never ending focus ancestry.

Description of user facing changes

It is now possible tin teract with data validation dropdown lists in Excel. this includes reporting the focused / selected item as you move up and down the list, and reporting focus back on the sheet when the list is closed with enter, space or escape.

Description of development approach

  • Excel appModule's isGoodUIAWindow treats this SysListView32 window as native UIA.
  • The parent property on this NvDAObject has been overridden to return the desktop, skipping the loop between SysListView32 and __XLACOOUTER windows.
  • AScript on this NvDAObject bound to enter, space and escape, corrects NvDA's focus back to the sheet.

Testing strategy:

In Excel 365:

  • Opened a new blank sheet in Excel
  • On a1, went to the Data Validation dialog, chose a type of 'list' and added 'red, blue, green' as the source.
  • when focused back on a1, pressed alt+downArrow. Confirmed that NVDA reported "list red 1 of 3".
  • Moved focus / selection up and down the list with the arrow keys. Confirming that the correct item was reported.
  • Closed the list with enter, space and escape, and confirmed that NvDA reported focus move back to the sheet, specifically cell a1. For enter and space, confirmed that the value of the cell changed.
    Further testing from other people on other versions of Excel would be very welcome.

Known issues with pull request:

None known.

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.

Summary by CodeRabbit

  • New Features
    • Introduced enhanced accessibility for data validation dropdowns in Excel, improving user interaction with Microsoft Excel 2016 and later.
  • Bug Fixes
    • Addressed focus issues with the SysListView32 control, ensuring better focus behavior when interacting with data validation elements.
  • Documentation

@michaelDCurran michaelDCurran requested a review from a team as a code owner August 22, 2024 13:49
@coderabbitai

coderabbitai Bot commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

Walkthrough

The changes introduce a new class, BrokenDataValidationSysListView32, within the Excel app module to address accessibility issues with data validation dropdowns. This class provides methods for identifying and correcting focus behavior for the SysListView32 control to enhance user interaction with dropdown lists in Excel.

Changes

File Path Change Summary
source/appModules/excel.py Introduced BrokenDataValidationSysListView32 class with methods for handling dropdown focus and behavior. Modified AppModule.isGoodUIAWindow to incorporate checks for the new class.

Assessment against linked issues

Objective Addressed Explanation
NVDA should report the current list item while moving through the list (#[15138])
Enhance accessibility of data validation dropdowns (#[15138])

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
user_docs/en/changes.md (1)

46-46: Ensure clarity and consistency in changelog entries.

The changelog entry is clear and references the correct issue number. However, to maintain consistency with the style of one sentence per line, consider rephrasing it slightly for clarity:

* Interact with Data validation dropdown lists in Microsoft Excel 2016 and above. (#15138)

@CyrilleB79

Copy link
Copy Markdown
Contributor

With Microsoft® Excel® 2016 (16.0.5448.1000) MSO (16.0.5456.1000) 32 bits, this PR has no effect. Though, I was not experiencing #15138, and the situation is not degraded, fortunately.

When I open the list or move the focus in the list with up/downArrows, I get the following error with or without this PR but the values are reported anyway:

IO - inputCore.InputManager.executeGesture (17:24:27.254) - winInputHook (8808):
Input: kb(desktop):alt+downArrow
DEBUGWARNING - NVDAObjects.window.excel.ExcelBase._getDropdown (17:24:27.286) - MainThread (14396):
Could not locate dropdown list in previous objects
IO - speech.speech.speak (17:24:27.350) - MainThread (14396):
Speaking ['Classeur1', 'onglet', CancellableSpeech (still valid)]
IO - speech.speech.speak (17:24:27.352) - MainThread (14396):
Speaking ['Feuille Feuil1', 'onglet', CancellableSpeech (still valid)]
ERROR - eventHandler.executeEvent (17:24:27.357) - MainThread (14396):
error executing event: focusEntered on <NVDAObjects.Dynamic_ExcelCellEditableTextWithAutoSelectDetectionUIA object at 0x067EFC10> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 345, in executeEvent
  File "eventHandler.pyc", line 116, in __init__
  File "eventHandler.pyc", line 125, in next
  File "NVDAObjects\__init__.pyc", line 1299, in event_focusEntered
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\__init__.pyc", line 1124, in _get_isPresentableFocusAncestor
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\UIA\__init__.pyc", line 1789, in _get_presentationType
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\__init__.pyc", line 878, in _get_presentationType
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\UIA\excel.pyc", line 365, in _get_states
AttributeError: 'NoneType' object has no attribute 'getSelectedItemsCount'
IO - speech.speech.speak (17:24:27.368) - MainThread (14396):
Speaking ['liste déroulante', 'développé', 'A un menu déroulant de validation des données.', CancellableSpeech (still valid)]
IO - speech.speech.speak (17:24:27.377) - MainThread (14396):
Speaking ['1', CancellableSpeech (still valid)]
DEBUG - NVDAObjects.NVDAObject._get_placeholder (17:24:27.377) - MainThread (14396):
Potential unimplemented child class: <NVDAObjects.Dynamic_UIAExcel7WindowWindowNVDAObject object at 0x0B912570>
DEBUG - NVDAObjects.NVDAObject._get_placeholder (17:24:27.378) - MainThread (14396):
Potential unimplemented child class: <NVDAObjects.UIA.UIA object at 0x0B912190>
ERROR - eventHandler.executeEvent (17:24:27.379) - MainThread (14396):
error executing event: gainFocus on <NVDAObjects.UIA.ListItem object at 0x0B911F50> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 345, in executeEvent
  File "eventHandler.pyc", line 116, in __init__
  File "eventHandler.pyc", line 125, in next
  File "NVDAObjects\UIA\__init__.pyc", line 1382, in event_gainFocus
  File "NVDAObjects\__init__.pyc", line 1307, in event_gainFocus
  File "braille.pyc", line 2554, in handleGainFocus
  File "braille.pyc", line 2559, in _doNewObject
  File "braille.pyc", line 1928, in getFocusContextRegions
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\__init__.pyc", line 1124, in _get_isPresentableFocusAncestor
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\UIA\__init__.pyc", line 1789, in _get_presentationType
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\__init__.pyc", line 878, in _get_presentationType
  File "baseObject.pyc", line 62, in __get__
  File "baseObject.pyc", line 168, in _getPropertyViaCache
  File "NVDAObjects\UIA\excel.pyc", line 365, in _get_states
AttributeError: 'NoneType' object has no attribute 'getSelectedItemsCount'

Just mentioning. But since the values are still reported and the error seems independant of this PR, it's probably not worth caring.

@CyrilleB79

Copy link
Copy Markdown
Contributor

With Microsoft® Excel® 2016 MSO (Version 2407 Build 16.0.17830.20056) 32 bits:
I am not experiencing #15138 either (or do I do wrong steps to reproduce?). And this PR does not degrade the experience, i.e. it does not seem to change anything.

On contrary to my tests with Excel® 2016 (16.0.5448.1000), I do not get the errors in the log with Excel 2016 MSO (Version 2407 Build 16.0.17830.20056).

Comment thread user_docs/en/changes.md Outdated

@SaschaCowley SaschaCowley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this Mick! The fix works for me, and will certainly be very much appreciated when it lands.

Comment thread source/appModules/excel.py
Comment thread source/appModules/excel.py Outdated
Comment thread user_docs/en/changes.md Outdated
michaelDCurran and others added 2 commits August 25, 2024 16:35
Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

@SaschaCowley SaschaCowley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, all looks good to me now.

@SaschaCowley

Copy link
Copy Markdown
Member

@CyrilleB79

With Microsoft® Excel® 2016 MSO (Version 2407 Build 16.0.17830.20056) 32 bits:

I'm using the same version of Excel as you, with the only difference being that I'm using the x64 build, and I am able to reproduce #15138. All of the comments that give actual build versions on the original issue seem to be using 64 bit Office, so I wonder if this is an x64 issue.

Regardless, since performance isn't degraded at all for you, and you don't have the original issue, I don't think there's any harm in merging this.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Yes, given the tests I have made, I have no objection at all to merge this PR.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 27, 2024
@seanbudd seanbudd added this to the 2024.4 milestone Aug 27, 2024
@seanbudd seanbudd changed the base branch from master to beta August 27, 2024 03:12
@seanbudd seanbudd merged commit be10321 into beta Aug 27, 2024
@seanbudd seanbudd deleted the i15138 branch August 27, 2024 03:13
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.

NVDA can't read Data Validation drop down in Excel 365

4 participants