Skip to content

Fix up of: Fix up of: Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8898)#10080

Merged
michaelDCurran merged 1 commit into
nvaccess:masterfrom
accessolutions:pr8898-mshtmlGetTableNotImplemented
Sep 11, 2019
Merged

Conversation

@JulienCochuyt

@JulienCochuyt JulienCochuyt commented Aug 13, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #9520
Fixes #7292 (comment) (only the error log in the comment, not the whole issue).
Faulty behavior introduced as of b02ed2d (#8898)

Summary of the issue:

MSHTML._get_table historically raises a NotImplementedError when there is no surrounding table.
IAccessible._get_table, on the other hand, returns None in the same situation.
IAccessible._get_selectionContainer, introduced by b02ed2d, calls self.table which can lead to the following error log:

ERROR - queueHandler.flushQueue (13:44:56.473):
Error in func _loadBufferDone from eventQueue
Traceback (most recent call last):
  File "queueHandler.pyo", line 53, in flushQueue
  File "virtualBuffers\__init__.pyo", line 464, in _loadBufferDone
  File "browseMode.pyo", line 1190, in event_treeInterceptor_gainFocus
  File "browseMode.pyo", line 1513, in event_gainFocus
  File "browseMode.pyo", line 1190, in <lambda>
  File "NVDAObjects\__init__.pyo", line 1030, in event_gainFocus
  File "NVDAObjects\__init__.pyo", line 918, in reportFocus
  File "speech.pyo", line 416, in speakObject
  File "speech.pyo", line 340, in speakObjectProperties
  File "baseObject.pyo", line 47, in __get__
  File "baseObject.pyo", line 147, in _getPropertyViaCache
  File "NVDAObjects\IAccessible\__init__.pyo", line 1265, in _get_selectionContainer
  File "baseObject.pyo", line 47, in __get__
  File "baseObject.pyo", line 147, in _getPropertyViaCache
  File "NVDAObjects\IAccessible\MSHTML.pyo", line 957, in _get_table
NotImplementedError

Description of how this pull request fixes the issue:

Return None instead of raising NotImplementedError in MSHTML._get_table, in coherence with its base class.

Testing performed:

Browsed with IE11 under Windows 10 to http://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html (which is the test case of #9520 and report focus on the example tab header.

Known issues with pull request:

This approach can theoretically break existing assumption of the method raising the exception, but other IAccessible implementation have been recently much more tested and return None in the same case.
So, I guess it is safer to apply the proposed change than to track every single call to _get_table and enclose in a try...except block, with the risk of missing a few of them in the process.

Change log entry:

Section: Bug fixes

NVDA is no longer silent when focusing an HTML tab header in Internet Explorer.

@JulienCochuyt
Fix up of: Fix up of: Don't announce 'selected' when the focus moves …
849c98a 
…in Google sheets if the focused cell is the only cell selected (nvaccess#8898)
@JulienCochuyt JulienCochuyt mentioned this pull request Aug 13, 2019
Closed
LeonarddeR
LeonarddeR reviewed Aug 27, 2019
View reviewed changes
Comment thread source/NVDAObjects/IAccessible/MSHTML.py
if nodeName=="TABLE": return MSHTML(HTMLNode=HTMLNode)
HTMLNode=HTMLNode.parentNode
raise NotImplementedError
return None

@LeonarddeR LeonarddeR Aug 27, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder whether this should be logged

@JulienCochuyt JulienCochuyt Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMHO, it shouldn't: This None is not an error, but rather a piece of information.
If the object is not part of a table, the other IAccessible implementations in NVDA simply return None without a notice.

Comment thread source/NVDAObjects/IAccessible/MSHTML.py
nodeName=HTMLNode.nodeName
if nodeName:
nodeName=nodeName.upper()
if nodeName=="TABLE": return MSHTML(HTMLNode=HTMLNode)

@LeonarddeR LeonarddeR Aug 27, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This does not support aria based tables (e.g. role grid). I don't think that's a problem, though it might deserve a change or a comment.

@JulienCochuyt JulienCochuyt Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not much aware of the aria based tables support, but please feel free to propose any useful comment to insert here.
As long as this PR does not change a thing in that scope - that is, no client code is expecting a NotImplementedError - I would suppose that further refining the process is out of the scope of this PR, don't you think?

@JulienCochuyt

JulienCochuyt commented Sep 11, 2019

Copy link
Copy Markdown
Contributor Author

cc @michaelDCurran

@michaelDCurran

michaelDCurran commented Sep 11, 2019

Copy link
Copy Markdown
Member

@JulienCochuyt I assume in the changelog you meant "table header" not "tab header"? Just checking before I merge and add that message to the changes file.

@JulienCochuyt

JulienCochuyt commented Sep 11, 2019

Copy link
Copy Markdown
Contributor Author

@JulienCochuyt I assume in the changelog you meant "table header" not "tab header"? Just checking before I merge and add that message to the changes file.

No, I indeed really meant "tab" header, aka. tab control, the control which allows to select a tab panel.
Feel free to set a better naming if this one is ambiguous.

@michaelDCurran

michaelDCurran commented Sep 11, 2019 via email

Copy link
Copy Markdown
Member

@michaelDCurran michaelDCurran merged commit 5eccf58 into nvaccess:master Sep 11, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Sep 11, 2019
michaelDCurran added a commit that referenced this pull request Sep 11, 2019
@michaelDCurran
Update changes file for #10080
b5e4f73
@JulienCochuyt JulienCochuyt mentioned this pull request Sep 11, 2019
Closed
@JulienCochuyt JulienCochuyt deleted the pr8898-mshtmlGetTableNotImplemented branch September 11, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

+1 more reviewer

@LeonarddeR LeonarddeR LeonarddeR left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2019.3

Development

Successfully merging this pull request may close these issues.

NVDA doesn't announce tabs in IE11 ARIA Tabs- NVDA does not list the total number of tabs in IE (Second Tab, tab selected, 2 of 3)

4 participants

@JulienCochuyt @michaelDCurran @LeonarddeR @nvaccessAuto