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

Improve presentation in Google sheets (with Braille mode enabled) #7935

Merged
merged 2 commits into from Feb 9, 2018

Conversation

Projects
None yet
5 participants
@michaelDCurran
Contributor

michaelDCurran commented Jan 23, 2018

Link to issue number:

None.

Summary of the issue:

Google has recently introduced a braille mode for Google Sheets, which exposes actual content via browser accessibility, rather than just forcing the AT to speak everything with a live region.
However, this new mode, uses a contenteditable table, thus NVDA tends to announce way too much inforamtion when navigating around. For instance, when moving down a row, NVDA announces the entire row, and also repetes the content of the cell several times. n Firefox it also announces x of y position info on the cell which is yet more pointless information.

Description of how this pull request fixes the issue:

This PR ensures that if an IAccessible NvDAObject has the readonly state, any existing editable state is removed. In other words, if an IAccessible2 implementation exposes both editable and readonly states, readonly will win. This was a compromize with Google for them to be able to use contenteditable on the table, they also expose the readonly state to convey the editable state should be overridden.
Handling the readonly state in this way ensures that NVDA no longer reports a readonly stable cell in a contenteditable table as being editable text. I.e. it no longer tries to announce the line of text at the caret on the cell.
Also, table rows in IAccessible2 table implementations are no longer treeted as being presentable focus ancestors. This stops the reporting of the entire row when moving from one to another. The cells themselves have enough information.
The PR also adds support for IAccessible2's rowtext and coltext attributes, by exposing them via an NVDAObject's cellCoordsText if one or both of these attributes exist. They are exposed simply as a concatination of colText and rowText, similar to how cell coordinates are presented in MS Excel. Thus Google sheets cell coordinates now are presented the same as MS excel.
Finally, IAccessible NvDAObjects no longer expose x of y position info if this object supports the IAccessibleTableCell interface. 2d table cell coordinates are much better than 1d index info.

Testing performed:

Navigated around a spreadsheet in Google Sheets with braille mode enabled. Verified the correct information was exposed in both speech and braille.

Known issues with pull request:

None.

Change log entry:

New features:

  • Early support for Google Sheets with Braille mode enabled.

michaelDCurran added some commits Jan 23, 2018

Improve presentation in Google Sheets (in braille mode).
* An IAccessible NVDAObject with both a readonly state and editable state should remove its editable state and class itself as just readonly. This ensures that table cells with the readonly state within a contenteditable (such as Google sheets) are correctly presented and do not announce as editable text.
* Report friendly cell coordinates for spreadsheets in Firefox and Chrome similar to MS Excel, by exposing coltext and rowtext IAccessible2 attributes via the cellCoordsText property.
IAccessible NVDAObject;s positionInfo property: don't expose indexInG…
…roup or similarItemInGroup if this object supports the IAccessibleTableCell interface as a cell's 2d info is much better position information.

@michaelDCurran michaelDCurran requested a review from feerrenrut Jan 23, 2018

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jan 23, 2018

I wonder whether this also works for sheets that have been imported from Microsoft Excel. I belief they are exposed in a somewhat different editor.

@@ -392,7 +392,7 @@ def findOverlayClasses(self,clsList):
clsList.insert(0, FocusableUnfocusableContainer)
if hasattr(self, "IAccessibleTextObject"):
if role==oleacc.ROLE_SYSTEM_TEXT or self.IA2States&IAccessibleHandler.IA2_STATE_EDITABLE:
if role==oleacc.ROLE_SYSTEM_TEXT or controlTypes.STATE_EDITABLE in self.states:

This comment has been minimized.

@leonardder

leonardder Jan 23, 2018

Collaborator

I'm a bit bothered about this one. Are you sure that this won't cause side effects in totally unrelated applications?

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Jan 23, 2018

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jan 23, 2018

@michaelDCurran commented on 23 Jan 2018, 08:53 CET:

I'm imagining you're worried that some subclass of IAccessible
NvDAObject has an implementation of states that adds the editable state,
yet  does not expect that the EditableText behaviour would be used.

Yes, that was exactly my concern. However, I missed that this behaviour will still be limited to IAccessible2 due to this logic only being applied when there is an IAccessibleTextObject, so I don't think it will cause anomalies.

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jan 23, 2018

I've tried to test this on two machines. Though I've been able to enable braille support, it seems that either using Next or this branch from source does not make any difference in the behavior. Could it be that there are different versions of this feature in circulation?

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Jan 23, 2018

@@ -643,15 +643,6 @@ def _get_presentationType(self):
return self.presType_layout
if role in (controlTypes.ROLE_TABLEROW,controlTypes.ROLE_TABLECOLUMN,controlTypes.ROLE_TABLECELL) and (not config.conf["documentFormatting"]["reportTables"] or not config.conf["documentFormatting"]["reportTableCellCoords"]):
return self.presType_layout
if role in (controlTypes.ROLE_TABLEROW,controlTypes.ROLE_TABLECOLUMN):

This comment has been minimized.

@leonardder

leonardder Jan 23, 2018

Collaborator

May be you could elaborate on why you removed this? I see some of this logic has been moved to ia2Web, but couldn't there be cases outside ia2Web (i.e. UIA) in which case ROLE_TABLEROW and ROLE_TABLECOLUMN are mapped to objects, in which case these objects will be of presentation type content erroneously?

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jan 23, 2018

@michaelDCurran commented on 23 jan. 2018 09:37 CET:

In a Google sheets spreadsheet, you should notice that NVDA will no
longer announce the text of the entire row when moving up and down a
column, and that it will not repete a lot of information on each cell.
also, Cell coordinates are now things like a1, not row 1 column 1.

I'm not always getting whole row announcements, but keep getting selection changed messages. Furthermore, I'm getting row 1 column 1 announcements when arrowing through the table. Object navigation reveals proper coordinates text for every cell though.

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jan 24, 2018

Looks like braille mode has entirely disappeared from my google sheets.
@michaelDCurran commented on 23 jan. 2018 07:23 CET:

Change log entry:

New features:

  • Improved support for Google Sheets with Braille mode enabled.

May be reword this to early support, similar to what has been done with Kindle math support in 2017.4?

@PratikP1

This comment has been minimized.

PratikP1 commented Jan 24, 2018

michaelDCurran added a commit that referenced this pull request Jan 26, 2018

@michaelDCurran michaelDCurran merged commit 0569e73 into master Feb 9, 2018

@nvaccessAuto nvaccessAuto removed the incubating label Feb 9, 2018

@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Feb 9, 2018

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