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

michaelDCurran · 2018-10-24T23:36:09Z

Link to issue number:

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.

…e focused cell is the only cell selected.

LeonarddeR · 2018-10-25T06:40:56Z

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?

source/NVDAObjects/IAccessible/__init__.py

LeonarddeR · 2018-10-25T07:29:03Z

source/controlTypes.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):


Could you split this into multiple lines?

michaelDCurran · 2018-10-25T23:34:30Z

@LeonarddeR: Hi have addressed your review comments for IAccessible.getSelectedItemsCount and controlTypes.processNegativeStates. However: In regards to braille: because Braille does not currently show negative states such as 'not selected', if we took away 'selected' in this case there would be an ambiguity. And this would have always been the case, not just for Google Sheets. My feeling is that we should just leave braille changes out for now, until we can come up with a better presentation of 'not selected' verses 'selected' in braille... unless I'm not understanding you correctly here.

feerrenrut · 2018-10-29T04:12:20Z

source/NVDAObjects/IAccessible/__init__.py

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


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

feerrenrut · 2018-10-29T04:21:10Z

source/NVDAObjects/IAccessible/__init__.py

+		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.


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.

feerrenrut · 2018-10-29T04:26:23Z

source/speech.py

+
+	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:


Could you split these conditions up over multiple lines please.

michaelDCurran · 2018-10-29T06:40:53Z

@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 · 2018-10-30T06:45:58Z

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

…ts if the focused cell is the only cell selected (#8879)" This reverts commit 51f5b2f.

…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.

…gle sheets if the focused cell is the only cell selected (#8879)" (#8893)" This reverts commit b4e9e83.

…ts if the focused cell is the only cell selected (#8879)" This reverts commit 51f5b2f.

…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.

Don't announce 'selected' when the focus moves in Google sheets if th…

5a2708d

…e focused cell is the only cell selected.

michaelDCurran requested a review from feerrenrut October 24, 2018 23:36

LeonarddeR reviewed Oct 25, 2018

View reviewed changes

Address review comments.

e546281

feerrenrut suggested changes Oct 29, 2018

View reviewed changes

michaelDCurran added 2 commits October 29, 2018 15:46

Merge branch 'master' into getSelectedItemsCount

59f2b29

Address review comments.

f024dda

feerrenrut approved these changes Oct 30, 2018

View reviewed changes

Update what's new

58a8131

michaelDCurran merged commit 51f5b2f into master Oct 30, 2018

nvaccessAuto added this to the 2018.4 milestone Oct 30, 2018

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

Revert "Don't announce 'selected' when the focus moves in Google shee…

07a184f

…ts if the focused cell is the only cell selected (#8879)" This reverts commit 51f5b2f.

michaelDCurran mentioned this pull request Oct 30, 2018

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

Merged

michaelDCurran mentioned this pull request Oct 31, 2018

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

Merged

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

Revert "Revert "Don't announce 'selected' when the focus moves in Goo…

6c2ddf1

…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…

bfc4d91

…ts if the focused cell is the only cell selected (#8879)" This reverts commit 51f5b2f.

jcsteh mentioned this pull request Feb 27, 2020

In Google Sheets, when Braille support is enabled, individual cells are identified as "selected" #8766

Open

This was referenced Mar 5, 2020

List should announce how many items are selected microsoft/vscode#91061

Closed

Implement selectionContainer design pattern for lists and tree views #10861

Open

jcsteh mentioned this pull request May 1, 2024

Aria-selected is not narrated for the gridcell #11986

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaelDCurran commented Oct 24, 2018 •

edited by LeonarddeR

LeonarddeR commented Oct 25, 2018

LeonarddeR left a comment

LeonarddeR Oct 25, 2018

michaelDCurran commented Oct 25, 2018 via email

feerrenrut Oct 29, 2018

feerrenrut Oct 29, 2018

feerrenrut Oct 29, 2018

michaelDCurran commented Oct 29, 2018

michaelDCurran commented Oct 30, 2018

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

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

Conversation

michaelDCurran commented Oct 24, 2018 • edited by LeonarddeR

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

LeonarddeR commented Oct 25, 2018

LeonarddeR left a comment

Choose a reason for hiding this comment

LeonarddeR Oct 25, 2018

Choose a reason for hiding this comment

michaelDCurran commented Oct 25, 2018 via email

feerrenrut Oct 29, 2018

Choose a reason for hiding this comment

feerrenrut Oct 29, 2018

Choose a reason for hiding this comment

feerrenrut Oct 29, 2018

Choose a reason for hiding this comment

michaelDCurran commented Oct 29, 2018

michaelDCurran commented Oct 30, 2018

michaelDCurran commented Oct 24, 2018 •

edited by LeonarddeR