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

Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected #8879

Merged
merged 5 commits into from Oct 30, 2018

Conversation

Projects
None yet
4 participants
@michaelDCurran
Contributor

michaelDCurran commented Oct 24, 2018

Link to issue number:

Fixes #8766

Summary of the issue:

When navigating cells in Google Sheets with GoogleShets Braille mode enabled, NVDA reports 'selected' for every cell the focus moves to.
Although this is technically correct (the cell is selected), it is not at all practical. Most if not all cells that are focused will be selected. So it ends up being redundant information.
We should change the experience so that:

  • If the focused cell is the only cell selected, then NvDA should not announce that it is selected.
  • If the focused cell is selected and more than one cell is selected, NVDA should announce that the focused cell is selected.
  • If the focused cell is not selected, NVDA should announce that the focused cell is 'not selected'.

Description of how this pull request fixes the issue:

This PR implements these rules by the following:

  • NvDAObjects now have a selectionContainer property, which should return the ancestor NVDAObject that manages the selection. E.g. for a table cell it would be the table. For a list item it would be the list. By default this property returns None.
  • Implement the selectionContainer property on IAccessible NVDAObjects. This implementation currently just returns the table NVDAObject if the NVDAObject has an ancestor table. In the future, other NVDAObjects such as UIA should implement this to make use of the UIA selection pattern etc.
  • Implement a getSelectedItemsCount method on NVDAObjects. This method should return the number of descendants currently selected. For performance reasons, the method takes a maxCount argument, which can be used to limet the amount of items counted, if an implementation has to literally fetch the items using a preallocated buffer in order to get the number of them. Implementations should try and fetch one more than maxCount, and if there are more items than maxCount, then sys.maxint should be returned, to denote many items.
  • Implemented getSelectedItemsCount on IAccessible NVDAObject. This implementation tries to fetch and count items with IAccessible::accSelect, and if that fails or is not available, then if the IAccessible supports IAccessibleTable2, IAccessibleTable2::nSelectedCells is used instead. Note that As Chrome does not implement accSelection on tables, it is necessary to fall back to nSelectedCells.
  • speech.speakObjectProperties: When speaking object properties for the reason of focus, remove the selected and selectable states from the object's states to be spoken, if the selectable state is present, the object has a selectionContainer, and its selectionContainer's getSelectedItemsCount is equal to 1.
  • controlTypes.processNegativeStates: report 'not selected' when there is the selectable state but not the selected state, if the role is tableCell, tableColumnHeader or tableRowHeader. Previously we were already doing this for tableRow.

Testing performed:

Created a new sheet in Google Sheets. Moved focus around the cells. NVDA no longer announced 'selected' if the focused cell was the only cell selected.

Known issues with pull request:

None.

Change log entry:

Bug fixes:

  • In Google Sheets with braille mode enabled, NVDA no longer announces 'selected' on every cell when moving focus between cells.
Don't announce 'selected' when the focus moves in Google sheets if th…
…e focused cell is the only cell selected.

@michaelDCurran michaelDCurran requested a review from feerrenrut Oct 24, 2018

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Oct 25, 2018

I updated the initial description in order for it to automatically close #8766

@leonardder

I'd be curious what this does with braille, but I"m currently unable to test with braille. I'd say we also want to apply the "don't present selected if only one item is selected" case to braille as well?

Show resolved Hide resolved source/NVDAObjects/IAccessible/__init__.py
@@ -727,7 +727,7 @@ def processNegativeStates(role, states, reason, negativeStates=None):
# but only if it is either focused or this is something other than a change event.
# The condition stops "not selected" from being spoken in some broken controls
# when the state change for the previous focus is issued before the focus change.
if role in (ROLE_LISTITEM, ROLE_TREEVIEWITEM, ROLE_TABLEROW) and STATE_SELECTABLE in states and (reason != REASON_CHANGE or STATE_FOCUSED in states):
if role in (ROLE_LISTITEM, ROLE_TREEVIEWITEM, ROLE_TABLEROW,ROLE_TABLECELL,ROLE_TABLECOLUMNHEADER,ROLE_TABLEROWHEADER) and STATE_SELECTABLE in states and (reason != REASON_CHANGE or STATE_FOCUSED in states):

This comment has been minimized.

@leonardder

leonardder Oct 25, 2018

Collaborator

Could you split this into multiple lines?

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Oct 25, 2018

try:
sel=self.IAccessibleObject.accSelection
except COMError as e:
log.debugWarning("Error fetching accSelection, %s"%e)

This comment has been minimized.

@feerrenrut

feerrenrut Oct 29, 2018

Contributor

If this is going to be fairly common (and expected in chrome, perhaps it's worth lowing the level to debug)

return super(IAccessible,self).selectionContainer
def getSelectedItemsCount(self,maxCount):
# To fetch the number of selected items, we first try MSAA's accSelection, but if that fails in any way, we fall back to using IAccessibleTable2's nSelectedCells, if we are on an IAccessible2 table.

This comment has been minimized.

@feerrenrut

feerrenrut Oct 29, 2018

Contributor

I'd suggest splitting this into two internal helper methods:

  • _getSelectedItemsCount_MSAA
  • _getSelectedItemsCount_IAccesibleTable2

This way the the return paths can be simplified, and the two approaches are more clearly separated.

states=newPropertyValues.get('states')
if states is not None and reason==controlTypes.REASON_FOCUS:
if controlTypes.STATE_SELECTABLE in states and controlTypes.STATE_SELECTED in states and obj.selectionContainer and obj.selectionContainer.getSelectedItemsCount(2)==1:

This comment has been minimized.

@feerrenrut

feerrenrut Oct 29, 2018

Contributor

Could you split these conditions up over multiple lines please.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Oct 29, 2018

@feerrenrut I split out the accSelection stuff into its own helper method. But as the IAccessibleTable2 code is only one call I left it in the original method. I also addressed your other review comments.

@michaelDCurran michaelDCurran merged commit 51f5b2f into master Oct 30, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Oct 30, 2018

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Oct 30, 2018

This pr is causing 'not selected' to be announced when navigating by table cell in browse mode.

michaelDCurran added a commit that referenced this pull request Oct 30, 2018

Revert "Don't announce 'selected' when the focus moves in Google shee…
…ts if the focused cell is the only cell selected (#8879)"

This reverts commit 51f5b2f.

michaelDCurran added a commit that referenced this pull request Oct 30, 2018

Revert "Don't announce 'selected' when the focus moves in Google shee…
…ts if the focused cell is the only cell selected (#8879)" (#8893)

This reverts commit 51f5b2f.
The pr was causing 'not selected' to be announced when navigating by table cell in browse mdoe.

michaelDCurran added a commit that referenced this pull request Oct 31, 2018

Revert "Revert "Don't announce 'selected' when the focus moves in Goo…
…gle sheets if the focused cell is the only cell selected (#8879)" (#8893)"

This reverts commit b4e9e83.

michaelDCurran added a commit that referenced this pull request Oct 31, 2018

Revert "Don't announce 'selected' when the focus moves in Google shee…
…ts if the focused cell is the only cell selected (#8879)"

This reverts commit 51f5b2f.

michaelDCurran added a commit that referenced this pull request Oct 31, 2018

Revert "Ensure that labels explicitly set on divs and spans are repor…
…ted in braille and speech" (#8899)

* Revert "Revert "Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8879)" (#8893)"

This reverts commit b4e9e83.

* Revert "Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8879)"

This reverts commit 51f5b2f.

* Revert "Merge all vbufBackend dlls into nvdaHelperRemote.dll (#8866)"

This reverts commit 24f5123.

* Revert "Fix handling of table cells without a containing table in browse mode. (#8887)"

This reverts commit 5fe34c5.

* Revert "Ensure that  labels explicitly set on divs and spans are reported in braille and speech (#8886)"

This reverts commit fd24d81.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment