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

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

Conversation

@JulienCochuyt
Copy link
Contributor

commented Aug 13, 2019

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.

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)
HTMLNode=self.HTMLNode
while HTMLNode:
nodeName=HTMLNode.nodeName
if nodeName:
nodeName=nodeName.upper()
if nodeName=="TABLE": return MSHTML(HTMLNode=HTMLNode)
HTMLNode=HTMLNode.parentNode
raise NotImplementedError
return None

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 27, 2019

Collaborator

I wonder whether this should be logged

This comment has been minimized.

Copy link
@JulienCochuyt

JulienCochuyt Aug 27, 2019

Author Contributor

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.

@@ -954,15 +954,15 @@ def setFocus(self):

def _get_table(self):
if self.role not in (controlTypes.ROLE_TABLECELL,controlTypes.ROLE_TABLEROW) or not self.HTMLNode:
raise NotImplementedError
return None
HTMLNode=self.HTMLNode
while HTMLNode:
nodeName=HTMLNode.nodeName
if nodeName:
nodeName=nodeName.upper()
if nodeName=="TABLE": return MSHTML(HTMLNode=HTMLNode)

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 27, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@JulienCochuyt

JulienCochuyt Aug 27, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@michaelDCurran michaelDCurran merged commit 5eccf58 into nvaccess:master Sep 11, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Sep 11, 2019

michaelDCurran added a commit that referenced this pull request Sep 11, 2019

@JulienCochuyt JulienCochuyt deleted the accessolutions:pr8898-mshtmlGetTableNotImplemented branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.