Skip to content

Fix table navigation in browse mode in MS Word for PR #13345#13396

Merged
michaelDCurran merged 1 commit into
nvaccess:masterfrom
mltony:word_fix_table
Mar 17, 2022
Merged

Fix table navigation in browse mode in MS Word for PR #13345#13396
michaelDCurran merged 1 commit into
nvaccess:masterfrom
mltony:word_fix_table

Conversation

@mltony

@mltony mltony commented Feb 27, 2022

Copy link
Copy Markdown
Contributor

Link to issue number:

Issue with navigating over merged cells in MS Word while in browse mode discussed in PR #13345.

Summary of the issue:

PR #13345 fixes navigating over merged cells in tables by caching the last column/row. Testing showed that it doesn't work in MS Word while in browse mode (focus mode works fine). This was tracked down to class UIABrowseModeDocumentTextInfo not implementing eq method.

Description of how this pull request fixes the issue:

Fixing that by implementing eq and hash functions in parent class RootProxyTextInfo.

Testing strategy:

Tested in MS Word. Tried with both checked and unchecked value of "Always use UI Automation to access Microsoft Word document controls when available".

Known issues with pull request:

N/A

Change log entries:

Doesn't deserve a separate entry, should be merged with #13345.

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

@mltony mltony requested a review from a team as a code owner February 27, 2022 00:29
@mltony mltony requested a review from seanbudd February 27, 2022 00:29
… class UIABrowseModeDocumentTextInfo comparable
@seanbudd

Copy link
Copy Markdown
Member

has this been tested with browse mode and focus mode?

@mltony

mltony commented Feb 28, 2022

Copy link
Copy Markdown
Contributor Author

@seanbudd,
Yes, tested with both browse mode and focus mode.

I cannot call parent hash function here - I spent quite some time debugging this. Here is the reason.
RootProxyTextInfo is a subclass of TextInfo, which is a subclass of baseObject.AutoPropertyObject. The latter overrides

def __new__(cls, *args, **kwargs):
    ...
    self.__instances[self]=None
    ...

So it uses the object as key in a dictionary during a call to __new__, which in turn calls __hash__(). However at this time none of the fields of the object are initialized, since __new__ is called before __init__. So self.innerTextInfo doesn't even exist at this point. So using identity hash function seems to be the only possibility here.

BTW on a tengential note, there seems to be a fundamental problems with __eq__ and __Hash__ functions. It appears that Python expects the object to be immutable to support custom __eq and __hash__ functions, whereas TextInfos are mutable. I don't quite understand caching in AutoPropertyObject, but it seems to me that identity hashing with custom __eq__ function might not work as expected. Python expects that for any two objects a and b if a == b then it must be that a.__hash__() == b.__hash__(), but this assumption is violated at least with TextInfos. Please note that I am not breaking things in this PR, as this is the way it works in for example class UIATextInfo. Not sure if this might cause some hard-to-debug problems in the future.

@mltony

mltony commented Mar 6, 2022

Copy link
Copy Markdown
Contributor Author

@seanbudd, This PR is ready for review (sorry if I confused you by my previous comment). What should be the next steps to have this one and #13345 merged?

@Adriani90

Copy link
Copy Markdown
Collaborator

Does this handle also i.e. rows of different number of cells? See for example isue #2683.

@mltony

mltony commented Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

This only forwards eq function for certain instances of TextInfo, it doesn't directly change any table-related behavior.

@michaelDCurran michaelDCurran merged commit c4586ea into nvaccess:master Mar 17, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 17, 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

Development

Successfully merging this pull request may close these issues.

5 participants