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

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

Merged
merged 2 commits into from
Feb 9, 2018

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran 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.

* 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.
…roup or similarItemInGroup if this object supports the IAccessibleTableCell interface as a cell's 2d info is much better position information.
@LeonarddeR
Copy link
Collaborator

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@michaelDCurran
Copy link
Member Author

michaelDCurran commented Jan 23, 2018 via email

@LeonarddeR
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Member Author

michaelDCurran commented Jan 23, 2018 via email

@@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link

PratikP1 commented Jan 24, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants