Java Access Bridge table cells should have the control's role#14348
Conversation
…than always having table cell role.
|
I assume Role.TABLECELL was enforced on these for a reason. It might be good to find out whether this will result in regressions, e.g. cells not announced as such. |
|
I have tested with some other tables, such as the Java demo application swingset2, and tables seem to work fine. Also interestingly before this change NVDA's object navigation did not see the table cell role, it actually got the control's actual role. The bug was only in focus navigation. |
That suggests that the overlay class doesn't apply when using object navigation. That could be something to look into as well. |
| class TableCell(JAB): | ||
|
|
||
| role=controlTypes.Role.TABLECELL | ||
|
|
||
| def _get_table(self): |
There was a problem hiding this comment.
I don't think changing the role of JAB.TableCell from controlTypes.Role.TABLECELL is the correct approach here. Is there no checkbox JAB control?
There was a problem hiding this comment.
NVDA has no JAB overlay class for checkbox. What such a class would contain I don't know as checkboxes work fine without, so what would such a class add? Also seeing how overlay classes get applied for JAB, we already have an issue that elif is used, so if a combobox is a direct child of a table then the Combobox overlay class is applied but not the table cell overlay class. If checkboxes were also to be a overlay class, should it be applied in the same way through an elif?
The problem is that in the Java Swing toolkit there is no such thing as a table cell control or table cell role, so to force table cell on a control which is the direct child of a table means you mask the correct role of the control.
Removal of the table cell role seems to work fine, NVDA is still aware of the table cell stuff as the overlay class is still applied (eg. it still reports column/row position).
There was a problem hiding this comment.
I agree with this approach in principle.
Java Access Bridge itself has no tableCell role. Authors place controls of what ever type on the grid as cells. I think the original reason I hardcoded role to tableCell many years back was that the tables I had seen at the time all used panel or label as the role for the cells, and it sounded a little odd. At the time I was unaware though that controls such as checkboxes could be used as cells.
Therefore, I think it is right to remove this role override and use the existing (more correct) role underneath.
However, at very least, I would suggest changing this to a proprty getter, e.g.
def _get_role(self):
Calling super and at very least checking if the role is Role.UNKNOWN and returning role.TABLECELL in that case. As it seems that our JAB gainFocus event handler specifically ignores NVDAObjects with a Role of UNKNOWN. So removing role tableCell may then start causing NVDA to ignore some gainFocus events where the author had not set the role on the tableCell.
Also, you may wish to suggest checking for a small subset of roles, such as label and panel, and instead returning tableCell. Roles that clearly have been chosen purely because there is not a tableCell role. Thus ensuring the original behaviour I saw does not appear again. Though I'd also be happy if you did not do this, keeping it more accurate to what the Author chose.
| class TableCell(JAB): | ||
|
|
||
| role=controlTypes.Role.TABLECELL | ||
|
|
||
| def _get_table(self): |
There was a problem hiding this comment.
I agree with this approach in principle.
Java Access Bridge itself has no tableCell role. Authors place controls of what ever type on the grid as cells. I think the original reason I hardcoded role to tableCell many years back was that the tables I had seen at the time all used panel or label as the role for the cells, and it sounded a little odd. At the time I was unaware though that controls such as checkboxes could be used as cells.
Therefore, I think it is right to remove this role override and use the existing (more correct) role underneath.
However, at very least, I would suggest changing this to a proprty getter, e.g.
def _get_role(self):
Calling super and at very least checking if the role is Role.UNKNOWN and returning role.TABLECELL in that case. As it seems that our JAB gainFocus event handler specifically ignores NVDAObjects with a Role of UNKNOWN. So removing role tableCell may then start causing NVDA to ignore some gainFocus events where the author had not set the role on the tableCell.
Also, you may wish to suggest checking for a small subset of roles, such as label and panel, and instead returning tableCell. Roles that clearly have been chosen purely because there is not a tableCell role. Thus ensuring the original behaviour I saw does not appear again. Though I'd also be happy if you did not do this, keeping it more accurate to what the Author chose.
|
Thanks for the detailed reply and that makes sense. I will go and add the property getter dealing with unknown. I will also have a look at panel, it may make sense to give that the table cell role as it is just a container. I think whether to handle panel will be determined by which gives the best results. |
|
I have done something similar to what @michaelDCurran said, but I did it in a slightly different way. I have done the handling in the JAB._get_role(self) function instead. My reasoning is that there was already some similar logic for lists and treeviews, so it seemed logical to keep this related logic together. This change though does mean that handling of Role.UNKNOWN is also dealt with by lists and treeviews, it seemed reasonable if we do it for tables to do it for those as well. Another consequence of doing it this way is that object navigation will also see these affected table cells as having Role.TABLECELL, if I had done it in the TableCell overlay class then object navigation would not get the Role.TABLECELL as it appears roles for object navigation are not taken from overlay classes. |
Link to issue number:
Fixes #14347
Summary of the issue:
Some table cells in Java applications may be controls, for example checkboxes, buttons, etc. NVDA needs to know the controls correct role to announce the state correctly. At the moment NVDA makes all table cells in Java Access Bridge applications have the table cell role, thus masking the real role of the control and so leading to strange state announcements.
Description of user facing changes
User will find controls in tables are more correctly announced by NVDA.
Description of development approach
Minimal change to achieve correct outcome.
Testing strategy:
Manual testing
Known issues with pull request:
None known.
Change log entries:
New features
Changes
Bug fixes
Controls in tables in applications using Java Access Bridge will now be more correctly announced by NVDA.
For Developers
Code Review Checklist: