Skip to content

Conversation

@savoptik
Copy link
Contributor

@savoptik savoptik commented Mar 13, 2023

This enhancement covers basic API and shared code that should be implemented for the Accessibility Announcement feature.

CSR JDK-8304499

@mrserb @prrace @azuev-java please review


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8302687: Implement interfaces and shared code for announcement feature (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13001/head:pull/13001
$ git checkout pull/13001

Update a local copy of the PR:
$ git checkout pull/13001
$ git pull https://git.openjdk.org/jdk.git pull/13001/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13001

View PR using the GUI difftool:
$ git pr show -t 13001

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13001.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 13, 2023

👋 Welcome back asemenov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Mar 13, 2023

@savoptik The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Mar 13, 2023
/**
* messages do not interrupt the current speech, they are spoken after the screen reader has spoken the current phrase
*/
@Native private static final int ANNOUNCE_WITHOUT_INTERRUPTING_CURRENT_OUTPUT = AccessibleAnnouncer.ANNOUNCE_WITHOUT_INTERRUPTING_CURRENT_OUTPUT;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that adding @Native to AccessibleAnnouncer constants would simplify the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 13, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 13, 2023

Webrevs

super.createUI(frame, "Accessible Anouncer test");
}

public static void main(String[] args) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static void main(String[] args) throws Exception {
public static void main(String[] args) throws Exception {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 15, 2023

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@savoptik
Copy link
Contributor Author

Do not close this pull request.
@mrserb @prrace @azuev-java please review

*/
void announce(Accessible a, final String str, final int priority);

} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Minor: no newline at the last line of the file. Not an error just a cosmetic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@azuev-java azuev-java left a comment

Choose a reason for hiding this comment

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

Since there is a new interface and a default implementation for it the CSR process should be invoked. See https://wiki.openjdk.org/display/csr/CSR+FAQs

Q: What sort of changes require CSR review?
A: Any change to a JDK interface meant to be used outside of the JDK itself requires CSR review. In this context "interface" isn't limited to the Java programing language definition of an interface, but encompasses the broader concept of a protocol between the JDK and users of the JDK. Examples of interfaces by this definition include:

  • Changes to public and exported APIs in java.* and javax.* packages.

And that falls well within that clause.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 13, 2023

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@savoptik
Copy link
Contributor Author

You don't need to close this pull request.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 12, 2023

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@savoptik
Copy link
Contributor Author

You don't have to close anything.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 10, 2023

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@savoptik
Copy link
Contributor Author

You don't need to close this pull request.

@prrace
Copy link
Contributor

prrace commented Aug 17, 2023

There's a really tree of JBS issues related to this topic and I see a process problem.

The bug for this PR is https://bugs.openjdk.org/browse/JDK-8302687

But the CSR in this PR is listed as https://bugs.openjdk.org/browse/JDK-8304499
but that is actually the CSR for https://bugs.openjdk.org/browse/JDK-8302352
which is an umbrella bug.

So this is wrong.
The CSR must be the CSR for the bug in the PR, ie for something you will actually push!
Else skara and everyone else will get confused.

Then there's the overall question as to how important, or appropriate is this API ?
Seems like its not really an A11Y API, its an arbitrary text-to-speech API.
I think of the A11Y APIs as being tied to the UI and making it accessible, not
providing some other way of communicating something which isn't even in the UI.
Put it this way, if I am NOT using an AT to provide speech, how are you communicating
whatever it is to the user - something changes in the UI, right ?
So then the A11Y API will be able to report that already so why do you need this ?
Show me some equivalent cases and uses in platform ATs APIs

@savoptik
Copy link
Contributor Author

@prrace
"There's a really tree of JBS issues related to this topic and I see a process problem.
The bug for this PR is https://bugs.openjdk.org/browse/JDK-8302687
But the CSR in this PR is listed as https://bugs.openjdk.org/browse/JDK-8304499
but that is actually the CSR for https://bugs.openjdk.org/browse/JDK-8302352
which is an umbrella bug.
So this is wrong.
The CSR must be the CSR for the bug in the PR, ie for something you will actually push!
Else skara and everyone else will get confused."

Thank you for your comment. I can close this PR and open a new one with a different number.

"Then there's the overall question as to how important, or appropriate is this API ?
Seems like its not really an A11Y API, its an arbitrary text-to-speech API.
I think of the A11Y APIs as being tied to the UI and making it accessible, not
providing some other way of communicating something which isn't even in the UI.
Put it this way, if I am NOT using an AT to provide speech, how are you communicating
whatever it is to the user - something changes in the UI, right ?
So then the A11Y API will be able to report that already so why do you need this ?
Show me some equivalent cases and uses in platform ATs APIs"

This functionality is part of the a11y API, for announcing we use the capabilities of the screen reader.
For example, NSAccessibility API capabilities for Mac: https://developer.apple.com/documentation/appkit/nsaccessibilitynotificationname?language=objc
Or NVDA controller client for Windows: https://github.com/nvaccess/nvda/tree/master/extras/controllerClient
Using the usual TTS annunciation tools is not a good idea, as the default TTS settings on the user's system can differ from those in the screen reader down to the voice, which is very confusing, and there may also be a difference in speed, volume and pace of speech.
Also, the built-in system TTS can sound simultaneously with the screen reader, it will sound like two voices speaking at the same time. Therefore, it will not be possible to implement interruption of the current output.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 19, 2023

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 17, 2023

@savoptik This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client client-libs-dev@openjdk.org rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

5 participants