Fix table navigation commands through merged cells by caching last column/row index by seanbudd · Pull Request #13345 · nvaccess/nvda

seanbudd · 2022-02-16T05:57:32Z

Follow up on the work done by @mltony in #11923

The approach is the same, just that types are added and a dataclass is used to cache the selection.

Link to issue number:

Fixes #7278, #11919

Summary of the issue:

When issuing table navigation commands repeadetly, e.g. pressing Control+Alt+Down several times in a row, the original column index is currently not preserved. Thus, if there happens to be a merged cell on the way, the column index will be reset to the minimal column index of merged cell.

Description of how this pull request fixes the issue:

This PR caches original column index for vertical table navigation and original row index for horizontal table navigation.

Testing strategy:

Performed manual tests with chrome HTML pages described in Table navigation doesn't respect colspan #7278, Table navigation commands do not always navigate in the same column or row #11919
Performed manual tests with testTable.docx using STR in Table navigation doesn't respect colspan #7278
System tests are to be added to test the STR in Table navigation doesn't respect colspan #7278

Known issues with pull request

Change log entries:

Bug fixes

- The cursor does not switch row or column anymore when using table navigation to navigate through merged cells. (#7278)

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

LeonarddeR · 2022-02-16T06:36:48Z

+
+	def _getNearestTableCell(
+			self,
+			tableID,


What is the type of this?

I'm not sure, it's not documented anywhere. I spent some time investigating and couldn't get a clear idea. As the change/type is unrelated and no information is lost, I'm hesitant to spend too long investigating this.

I had a quick look. In virtual buffers it is an int, whereas in UIA it is a tuple (the runtime id). I guess it's safe to mark it any, as long as it can be compared.

Suggested change

tableID,

# At least known to be a tuple for UIA and an integer for virtual buffers

tableID, typing.Any,

I added a type and summary to the module level, as tableID is used as a parameter elsewhere in the file, and will need to get type information added eventually.

CyrilleB79 · 2022-02-16T22:38:03Z

Thanks @seanbudd to take over this PR.

I have not tested the snapshot of the current PR. But I have some points to add here:

As explained in Fixing table navigation commands by caching last column/row index #11923 (comment) in the original PR, the issue was also present in Word tables. Is it fixed in this PR?
Regarding the change log, I would have avoided the term "index" which is known by web developers, but maybe not by all users. What about this one:
The cursor does not switch row or column anymore when using table navigation to navigate through merged cells. (#7278)
Feel free to modify this further; I do not speak natively English.

seanbudd · 2022-02-16T22:52:17Z

Thanks @CyrilleB79, that changelog entry reads fine for me, I've updated the PR description.

I haven't tested with Word yet, however this PR uses the same approach as in #11923, so if the issue was present in Word with #11923, this is still the case.

seanbudd · 2022-02-18T02:03:52Z

@CyrilleB79 - It appears that the behaviour is fixed with MS Word as well now. This did need the latest master merged in to work with MS word, I think due to a related fix for table navigation in MS word.

CyrilleB79 · 2022-02-18T10:25:02Z

@seanbudd, no it is not completely fixed for MS Word; it behaves the same way as NVDA 2021.3.1.

More generally, I think that the table nav experience should be consistent between all the applications.
Under Word, I have created a 4x4 table, have filled the cell content with the cell's coordinates (as per Excel naming convention) and merged the following groups of cells:

C2+D2 (2 cells horizontally)
B3+B4 (2 cells vertically)
I have then saved this document as a Word document, as an html file and as a PDF file.
TestTableWord.docx
TestTableWord.pdf
TestTableWord.htm.txt

I have tested navigating the whole table horizontally, then vertically in either direction.

Table navigation is as expected in the html file (with Chrome).

For Word document I get the following issues:

C2+D2, move right arrow. Expected: "Table edge" reported. Actual: D1
A4, right, left. Actual C4, A4. Expected B3+B4, A4.
B3+B4, down. Actual: A4. Expected "Table edge" reported.
D1, down, up. Actual: D3, D1. Expected: C2+D2, D1.

For PDF document, I get the following issue:

A3, down. Expected: A4. Actual: empty cell.
A4, left. Expected: "Table edge" reported. Actual: empty cell.
A4, right, left. Actual C4, A4. Expected B3+B4, A4.
B3+B4, down. Actual: A4. Expected "Table edge" reported.
D1, down. Actual: "Table edge" reported. Expected: C2+D2
D3, up. Actual: "Table edge" reported. Expected: C2+D2

It makes a lot of issues, but many of them are probably related to each other.

mltony · 2022-02-21T01:12:09Z

@seanbudd, first of all thanks for taking over this PR!
@CyrilleB79, I tried to repro your use cases with this PR. For some reason I couldn't reproduce your issues in MS Word. In focus mode it works all as expected for me.
One interesting observation though is that it doesn't appear to work in MS Word in browse mode, that is behavior with merged cells is exactly the same as before this PR, that is it never reuses cached row/column index in merged cells. I spent a little time debugging and found out that UIABrowseModeDocumentTextInfo class doesn't have eq method implemented, and so Python defaults to identity eq, which results to the following:

focus.treeInterceptor.selection == focus.treeInterceptor.selection
False
We rely on comparing selections to check whether user has moved away when the cache should be invalidated, so we're having a false positive here always. It should be easy to fix though. UIABrowseModeDocumentTextInfo inherits from RootProxyTextInfo, and it appears to be a thin wrapper around another TextInfo class and eq and hash methods should be forwarded to the nested textInfo there. I actually tried to fix this myself but ran into issues with tooling.
As for PDF problems, I actually wouldn't get into this territory. PDF format has a long list of accessibility issues and in this case most likely word has produced a malformed table in PDF or something.

…reserveRowColumn-2

seanbudd · 2022-02-21T02:03:36Z

@seanbudd, first of all thanks for taking over this PR! @CyrilleB79, I tried to repro your use cases with this PR. For some reason I couldn't reproduce your issues in MS Word. In focus mode it works all as expected for me. One interesting observation though is that it doesn't appear to work in MS Word in browse mode, that is behavior with merged cells is exactly the same as before this PR, that is it never reuses cached row/column index in merged cells. I spent a little time debugging and found out that UIABrowseModeDocumentTextInfo class doesn't have eq method implemented, and so Python defaults to identity eq, which results to the following:

focus.treeInterceptor.selection == focus.treeInterceptor.selection
False
We rely on comparing selections to check whether user has moved away when the cache should be invalidated, so we're having a false positive here always. It should be easy to fix though. UIABrowseModeDocumentTextInfo inherits from RootProxyTextInfo, and it appears to be a thin wrapper around another TextInfo class and eq and hash methods should be forwarded to the nested textInfo there. I actually tried to fix this myself but ran into issues with tooling.
As for PDF problems, I actually wouldn't get into this territory. PDF format has a long list of accessibility issues and in this case most likely word has produced a malformed table in PDF or something.

This will only fix the problem if UIA is enabled. I'm not sure of way to compare the selection (TextInfos for a BrowseModeDocumentTextInfo).

seanbudd · 2022-02-21T02:09:15Z

To fix the UIA case, UIABrowseModeDocumentTextInfo should inherit (at least eq and hash) from UIATextInfo.

mltony · 2022-02-27T00:55:01Z

@seanbudd, I fixed the word issue in #13396.

…reserveRowColumn-2 # Conflicts: # tests/system/robot/chromeTests.py # tests/system/robot/chromeTests.robot

seanbudd · 2022-02-28T03:35:32Z

I've reopened this PR, so that the remaining bug can be fixed in a separate PR eg #13396

… class UIABrowseModeDocumentTextInfo comparable (#13396) 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.

michaelDCurran · 2022-03-24T23:43:17Z



+_TableID = Union[int, Tuple, Any]
+"""


I have never seen this pattern of an unnamed string following an assignment statement. Is it supposed to become the symbol's docstring? I'm not sure that actually works?

epydoc supports this, however I have discovered it is bad practice, and part of the rejected PEP 224.

It appears the correct pattern for this in future is typing.Annotated in python 3.9.

We have been using this pattern already in some contexts such as BACK_COMPAT_TO, appModuleHandler.helperLocalBindingHandle.

Intermittent GC error "unreachable objects" caused by PR #13345. Description of how this pull request fixes the issue: The root cause was that we used to store TextInfo object inside Document, thus creating a cyclic reference. Such reference loops cannot be deleted by zeroing reference counter and can only be deleted by full GC sweep. I am not sure why NVDA prints warnings in this case. This was fixed by not storing TextInfo in the first place. We now store last cell coordinates as lastRow/lastCol. Table cache is going to be valid as long as row/col of current selection are the same as stored lastRow/lastCol - and in this case we will be using trueRow/trueCol to compute next cell to preserve coordinates when navigating through merged cells.

Tony Malykh and others added 6 commits December 9, 2020 15:30

Fixing table navigation commands by caching last column/row index

651e9e1

Fix flake warning

39d0f1a

Fix flake warning

75d64ec

Changes as per Leonard's comments

7f6e8a1

Merge branch 'master' into fixTableNavigationPreserveRowColumn

e404b58

Create dataclass for table selection

7189c60

seanbudd changed the title ~~Fix table navigation preserve row column 2~~ Fixing table navigation commands by caching last column/row index Feb 16, 2022

seanbudd mentioned this pull request Feb 16, 2022

Fixing table navigation commands by caching last column/row index #11923

Closed

Add system test for #7278

4156618

seanbudd marked this pull request as ready for review February 16, 2022 06:15

seanbudd requested a review from a team as a code owner February 16, 2022 06:15

seanbudd requested a review from michaelDCurran February 16, 2022 06:15

LeonarddeR reviewed Feb 16, 2022

View reviewed changes

fix casing for constants

adebe81

seanbudd changed the title ~~Fixing table navigation commands by caching last column/row index~~ Fix table navigation commands through merged cells by caching last column/row index Feb 16, 2022

Add tableID type

cd9a968

This comment was marked as off-topic.

Sign in to view

fix documentation spacing

d0a0457

Merge branch 'master' into fixTableNavigationPreserveRowColumn-2

6f7ae0a

surfer0627 mentioned this pull request Feb 19, 2022

table navigation: add a new reference value, so that NVDA can remember the last cell coordination before the cursor was moved into a merged cell #8730

Closed

Merge remote-tracking branch 'origin/master' into fixTableNavigationP…

a10ea97

…reserveRowColumn-2

seanbudd marked this pull request as draft February 21, 2022 02:08

mltony mentioned this pull request Feb 27, 2022

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

Merged

5 tasks

seanbudd marked this pull request as ready for review February 28, 2022 02:39

Merge remote-tracking branch 'origin/master' into fixTableNavigationP…

23c58c1

…reserveRowColumn-2 # Conflicts: # tests/system/robot/chromeTests.py # tests/system/robot/chromeTests.robot

This comment was marked as off-topic.

Sign in to view

mltony mentioned this pull request Mar 6, 2022

Adding commands to jump to first/last row/column in tables #13435

Merged

5 tasks

feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022

michaelDCurran approved these changes Mar 24, 2022

View reviewed changes

seanbudd added 2 commits March 25, 2022 11:12

Merge branch 'master' into fixTableNavigationPreserveRowColumn-2

2dd3303

update changes

03aac89

seanbudd merged commit 44b16a1 into master Mar 25, 2022

seanbudd deleted the fixTableNavigationPreserveRowColumn-2 branch March 25, 2022 02:27

nvaccessAuto added this to the 2022.2 milestone Mar 25, 2022

lukaszgo1 mentioned this pull request Apr 4, 2022

Intermittent garbage handler errors in Firefox #13578

Closed

mltony mentioned this pull request Apr 16, 2022

Fixing GC unreachable error of table navigation #13613

Merged

5 tasks

	tableID,
	# At least known to be a tuple for UIA and an integer for virtual buffers
	tableID, typing.Any,

Uh oh!

Conversation

seanbudd commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request

Change log entries:

Code Review Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeonarddeR Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

CyrilleB79 commented Feb 16, 2022

Uh oh!

seanbudd commented Feb 16, 2022

Uh oh!

seanbudd commented Feb 18, 2022

Uh oh!

CyrilleB79 commented Feb 18, 2022

Uh oh!

mltony commented Feb 21, 2022

Uh oh!

seanbudd commented Feb 21, 2022

Uh oh!

seanbudd commented Feb 21, 2022

Uh oh!

mltony commented Feb 27, 2022

Uh oh!

seanbudd commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

michaelDCurran Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

seanbudd commented Feb 16, 2022 •

edited

Loading

seanbudd Feb 16, 2022 •

edited

Loading

seanbudd Feb 16, 2022 •

edited

Loading

seanbudd commented Feb 28, 2022 •

edited

Loading