-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add speech recognition context to the Web Speech API #145
base: main
Are you sure you want to change the base?
Conversation
Hi @padenot, can you review this one too and also take a look at the explainer again when you get a chance? Feel free to assign another reviewer too if needed |
index.bs
Outdated
[Exposed=Window] | ||
interface SpeechRecognitionPhraseList { | ||
readonly attribute unsigned long length; | ||
getter SpeechRecognitionPhrase item(unsigned long index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is invalid WebIDL.
It would be either:
SpeechRecognition item(unsigned long index);
or
getter SpeechRecognition(unsigned long index)
What is the intent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to SpeechRecognitionPhrase item(unsigned long index)
, but using getter is actually how SpeechRecognitionResultList
is doing it, as well as some list objects I've seen in other specs, e.g. https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#the-domstringlist-interface. I thought it's a standard thing to always define a getter for a list, but I don't see that getter is required in our use case, so I can either keep it or remove it.
682fb24
to
5be471c
Compare
Explainer for speech recognition context is added in WebAudio#140
Hi @padenot, I've updated the specs as well as the explainer according to your comments. Please take a look again when you get a chance. Thanks! |
Can we please focus on either the explainer or the spec patch? If we have a spec patch, an explainer shouldn't be necessary. If we aren't comfortable writing the spec patch right now because we want to iterate, it doesn't seem useful to update the spec patch. Let me know which one I should look at first please? |
Hi @padenot, you can focus on the spec patch right now. I'm keeping the spec patch and the explainer in sync and the spec patch has many more details than the explainer, so if we can reach consensus on the spec, we can also reach consensus on the explainer easily. I think the explainer is still necessary when we want to launch this feature since we will be asked for a link to an explainer in many places. The explainer is also a good place to show why we want to add contextual biasing, and provides brief introduction on the changes for people who don't want to learn about every detail in specs. |
Explainer for speech recognition context is added in #140
This PR should be updated and merged after the explainer is approved and merged.
Preview | Diff