8384483: Create implementation of NSAccessibilityTable protocol - #2217
8384483: Create implementation of NSAccessibilityTable protocol#2217azuev-java wants to merge 5 commits into
Conversation
- Create first implementation; - Fix incorrect verbosity level when navigating to the table from other component;
…lectedRowsAttribute. We should select a set of rows, not a set of cells because that confuses VoiceOver and it falls back to reporting as first row is selected.
…ive accessibility peer for Lists; - Expose the focused item trough the corresponding function in AccessibleBase; - Resolve issue with incorrect row selection in MacAccessible;
|
👋 Welcome back kizune! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
|
Reviewers: @arapte @andy-goryachev-oracle |
There was a problem hiding this comment.
- opened monkey tester
- selected table view page
- chose Items: 500,000 rows
- enabled voice over
and it locked up with the beach ball. are you rendering all the rows somehow? I see it iterating in TableViewSkin:152 with rowIndex 143390 which isn't right.
MacAccessible:1152 iterates over the whole table (maxCount=500000).
edit: this looks like something this PR introduced, as it's not reproducible in master.
|
Just FYI, there seems to be multiple accessibility issues in TableView (present in master, so they are separate, pre-existing issues):
|
- Always return the NSArray of the appropriate size, otherwice VO reports the table size incorrectly; - To avoid querying of the entire content of the control fill in only visible rows +/- additional 10 to avoid incorrect reporting due to the VO caching when navigating with VO controls beyond the visible range; - To request the visible range from the control's skin a new a11y attribute added - VISIBLE_ITEM_RANGE. Since outside of the MacOS it is not used that will not create any problems on other platforms;
I have fixed it in the latest commit, should be working now. |
I will try to address these issues with the implementation for the table rows, if i will not be able to - i will create a separate bug to tackle it later. |
|
How do I trigger VISIBLE_ITEM_RANGE code path? |
|
Noted a weird thing (though pre-existing in master): after a cell in the 
also, the tooltip shows |
andy-goryachev-oracle
left a comment
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
basically works identically to the master branch.
two questions:
- accessibility cursor sort of breaks after editing a cell in both ListView and TableView
- question about updating visible range in case of mouse scrolling
| return new int[] { 0, 0 }; | ||
| } | ||
| int firstIndex = firstVisibleCell.getIndex(); | ||
| return new int[] { firstIndex, lastVisibleCell.getIndex() - firstIndex + 1 }; |
There was a problem hiding this comment.
this code is not being hit when scrolling with the mouse.
should the change in the viewport origin result in VISIBLE_ITEM_RANGE being updated?
There was a problem hiding this comment.
this code is not being hit when scrolling with the mouse.
should the change in the viewport origin result in VISIBLE_ITEM_RANGE being updated?
Not until the VO requests the data. Which happens when it detects change in the selected row or when we enter/exit the table or list with VO selection shortcuts.
| * <li>Parameters: </li> | ||
| * </ul> | ||
| */ | ||
| VISIBLE_ITEM_RANGE(int[].class), |
There was a problem hiding this comment.
- this is a new attribute, so this PR needs as CSR, correct?
- would it make more sense to use a more descriptive record instead of int[]?
minor: the values in this enum are not sorted alphabetically - should they? do we care?
There was a problem hiding this comment.
- this is a new attribute, so this PR needs as CSR, correct?
I have no idea. If that would be a change in the public API i would definitely say yes, for the attribute - since we add it and not changing the meaning of the existing attribute - may be? Gray area for me honestly. @kevinrushforth Any comments?
- would it make more sense to use a more descriptive record instead of int[]?
That would require more processing on the native side so i would just leave it as is. It is a technical attribute that is required to pass information from the skin level to the native a11y helper - it is not supposed to be available for any other purposes.
minor: the values in this enum are not sorted alphabetically - should they? do we care?
Not really. Changing the existing code just to sort the enum - i would hate to do so, i prefer to keep the history cleaner, it's an old code, technical binding of the parameters to the native bound variables, nobody really looks at it except the compiler.
There was a problem hiding this comment.
AccessibleAttribute is public API. The new attribute also needs @since 28.
CSR is needed.
(it's fine to keep it an int[])
There was a problem hiding this comment.
AccessibleAttribute is public API. The new attribute also needs
@since 28. CSR is needed.
Oh well. Time to learn how to create CSR for JavaFX. Yaaaaay.... [/sarcasm off]
There was a problem hiding this comment.
- this is a new attribute, so this PR needs as CSR, correct?
I have no idea. If that would be a change in the public API i would definitely say yes, for the attribute - since we add it and not changing the meaning of the existing attribute - may be? Gray area for me honestly. @kevinrushforth Any comments?
It's new API, so yes, a CSR is in order.
minor: the values in this enum are not sorted alphabetically - should they? do we care?
Not really. Changing the existing code just to sort the enum - i would hate to do so, i prefer to keep the history cleaner, it's an old code, technical binding of the parameters to the native bound variables, nobody really looks at it except the compiler.
Yeah, let's not do this. In addition to creating diff churn for no good reason, this would techincally be an incompatible change, since it would change the result of the "ordinal()" method. In practice I can't imagine that wouldn't matter, but ...
AccessibleAttribute is public API. The new attribute also needs
@since 28. CSR is needed.Oh well. Time to learn how to create CSR for JavaFX. Yaaaaay.... [/sarcasm off]
Same way as you would for the JDK. :)
There was a problem hiding this comment.
Yeah, let's not do this. In addition to creating diff churn for no good reason, this would techincally be an incompatible change, since it would change the result of the "ordinal()" method. In practice I can't imagine that wouldn't matter, but ...
Although, I see that we've inserted enums in the middle before, and this does too, so nm about the compatibility point.
Still, let's not have the churn.
It is being called from the a11y code to query the currently visible rows range when VO requests all rows. So just create a large list or table (like 100000 rows), put focus on one of the first rows and start VoiceOver. Then using mouse scroll somewhere far away from the currently focussed row and click on any element within the row. The VO should re-request the list of the rows and it should correctly narrate the newly selected element. If this query fails the VO will narrate "Empty row" instead of the actual content of the selected row or cell. |
Thats... interesting. Seems like another thing i might need to address while working on the row a11y peer. Totally unexpected. Where on earth did it get data for that that tooltip? |
oh sorry, this tooltip comes from the monkey tester... |
|
/csr |
|
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request. @azuev-java please create a CSR request for issue JDK-8384483 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
Re: scrolling. In theory, there is |
|
Another issue:
locks up both This is not a big issue for |
|
Created https://bugs.openjdk.org/browse/JDK-8390342 for VoiceOver lockup after select-all with large models. |
| * <li>Needs notify: no </li> | ||
| * <li>Return Type: {@code int[]} containing the first item index and the | ||
| * number of visible items </li> | ||
| * <li>Parameters: </li> |
There was a problem hiding this comment.
Parameters: should describe the structure (see BOUNDS_FOR_RANGE for example)
| * <ul> | ||
| * <li>Used by: ListView and TableView </li> | ||
| * <li>Needs notify: no </li> | ||
| * <li>Return Type: {@code int[]} containing the first item index and the |
There was a problem hiding this comment.
please specify how it encodes a no-range case.
it looks like the code returns [0,0], but is this right?
should it return an empty array or null instead?
There was a problem hiding this comment.
The {0, 0} is the valid range, we should not return empty array or null in any case.
3 participants
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2217/head:pull/2217$ git checkout pull/2217Update a local copy of the PR:
$ git checkout pull/2217$ git pull https://git.openjdk.org/jfx.git pull/2217/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2217View PR using the GUI difftool:
$ git pr show -t 2217Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2217.diff
Using Webrev
Link to Webrev Comment