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

Excel with UIA: announce merged cells #12843

Merged
merged 6 commits into from
Jun 20, 2022

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Sep 14, 2021

Link to issue number:

None

Summary of the issue:

When UIA is enabled for Excel, merged cells were not mentioned as such. Instead, only the first coordinates of the merged range were reported.

Description of how this pull request fixes the issue:

There is no way to fetch the last cell in the range from Excel itself. The UIA implementation in Excel also refuses to provide real row and column numbers. Therefore we have to convert the alphabetical column representation to a column number, then correct the column number for column span and convert it back to alphabetical representation.

Testing strategy:

Tested several merged cells on an empty Excel sheet.

Known issues with pull request:

  1. Excel seems to expose only the visible part of the sheet to UIA. Therefore, if a merged cell spans more rows or columns than what's visible, the announcements are likely incorrect. Note that this also applies to selection.
  2. Initiating selection from a merged cell is broken, because the selection pattern reveals a null pointer as the first selected value. This is definitely a bug in Excel.

Change log entries:

Changes

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • API is compatible with existing addons.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@LeonarddeR LeonarddeR requested a review from a team as a code owner September 14, 2021 06:42
@seanbudd seanbudd added this to the 2021.3 milestone Oct 15, 2021
@feerrenrut
Copy link
Contributor

@seanbudd Could you elaborate on why this must go into the 2021.3 release, there doesn't seem to be a justification provided.

@cary-rowen
Copy link
Contributor

I am a frequent user of excel. I hope that this PR can be merged. Is there any problem that prevents this PR?

@seanbudd seanbudd self-requested a review October 18, 2021 05:20
@seanbudd seanbudd removed this from the 2021.3 milestone Oct 18, 2021
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 @LeonarddeR - do you know if MS is aware of the bug(s)?

source/NVDAObjects/UIA/excel.py Outdated Show resolved Hide resolved
for i in modGenerator(n)
)[::-1]

def _getNumberRepresentationForColumn(self, column):
Copy link
Member

Choose a reason for hiding this comment

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

Could this get a unit test with a couple of examples?

Copy link
Member

Choose a reason for hiding this comment

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

it appears that unittests cannot import from NVDAObjects.UIA.excel

======================================================================
ERROR: Failure: AttributeError ('NoneType' object has no attribute 'registerUIAProperty')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\sean\projects\nvda\.venv\lib\site-packages\nose\failure.py", line 39, in runTest       
    raise self.exc_val.with_traceback(self.tb)
  File "C:\Users\sean\projects\nvda\.venv\lib\site-packages\nose\loader.py", line 418, in loadTestsFromName
    addr.filename, addr.module)
  File "C:\Users\sean\projects\nvda\.venv\lib\site-packages\nose\importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "C:\Users\sean\projects\nvda\.venv\lib\site-packages\nose\importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "C:\Users\sean\AppData\Local\Programs\Python\Python37-32\lib\imp.py", line 234, in load_module   
    return load_source(name, filename, file)
  File "C:\Users\sean\AppData\Local\Programs\Python\Python37-32\lib\imp.py", line 171, in load_source   
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "C:\Users\sean\projects\nvda\tests\unit\test_excel.py", line 11, in <module>
    from NVDAObjects.UIA.excel import ExcelCell
  File "C:\Users\sean\projects\nvda\source\NVDAObjects\UIA\__init__.py", line 928, in <module>
    class UIA(Window):
  File "C:\Users\sean\projects\nvda\source\NVDAObjects\UIA\__init__.py", line 929, in UIA
    _UIACustomProps = UIAHandler.customProps.CustomPropertiesCommon.get()
  File "C:\Users\sean\projects\nvda\source\UIAHandler\customProps.py", line 75, in get
    cls._instance = cls()
  File "C:\Users\sean\projects\nvda\source\UIAHandler\customProps.py", line 83, in __init__
    uiaType=UIAutomationType.INT,
  File "<string>", line 6, in __init__
  File "C:\Users\sean\projects\nvda\source\UIAHandler\customProps.py", line 54, in __post_init__        
    self.id = NVDAHelper.localLib.registerUIAProperty(
AttributeError: 'NoneType' object has no attribute 'registerUIAProperty'

source/NVDAObjects/UIA/excel.py Outdated Show resolved Hide resolved
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@LeonarddeR
Copy link
Collaborator Author

Do you know if MS is aware of the bug(s)?

Nope. May be @michaelDCurran knows. I believe there's no such thing as a presentational row or column number like the ones implemented for IAccessible2. That would certainly help.

@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@seanbudd seanbudd removed the request for review from michaelDCurran June 14, 2022 04:18
@seanbudd seanbudd merged commit 0976a98 into nvaccess:master Jun 20, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jun 20, 2022
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.

5 participants