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

Introduce start session algorithm #138

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

beaufortfrancois
Copy link
Contributor

@beaufortfrancois beaufortfrancois commented Feb 5, 2025

This PR addresses concerns raised in #135 (comment) and #126 (comment):

  • It makes sure start() and start(MediaStreamTrack audioTrack) throw if document is not fully active:
  • Request permission to use microphone is called only for start()
  • A new "start session algorithm" is introduced to formalize how things work when speech recognition starts.

The following tasks have been completed:

Implementation commitment:

  • Blink: (link to issue)
  • Gecko: (link to issue)
  • WebKit: (link to issue)

Preview | Diff

@beaufortfrancois
Copy link
Contributor Author

@evanbliu @padenot @youennf Let me know if that looks good to you

index.bs Outdated
@@ -339,6 +351,16 @@ See <a href="https://lists.w3.org/Archives/Public/public-speech-api/2012Sep/0072

</dl>

<p>When the <dfn>start session algorithm</dfn> with <var>requestMicrophonePermission</var> is invoked, the user agent MUST run the following steps:

1. If the [=current settings object=]'s [=relevant global object=]'s [=associated Document=] is NOT [=fully active=], throw an {{UnknownError}} and abort these steps.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I used UnknownError to match WebKit that already shipped Web Speech.

FYI Firefox uses InvalidStateError instead but did not ship Web Speech. So even though, I'd personally be in favor of InvalidStateError, I picked UnknownError so that web developers already catching this properly based on Safari don't have to update their code.

Copy link

Choose a reason for hiding this comment

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

I think webkit could align to whatever deemed appropriate, this is an edge case so this is likely not a web compat issue.
I also prefer InvalidStateError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you and @evanbliu also prefer InvalidStateError, I've updated this PR to use it.

index.bs Show resolved Hide resolved
@beaufortfrancois
Copy link
Contributor Author

Quick question for you folks!

When adding web platform tests for this in https://chromium-review.googlesource.com/c/chromium/src/+/6237075, I used frameWindow.SpeechRecognition = frameWindow.SpeechRecognition || frameWindow.webkitSpeechRecognition; so that it works in Safari and Chrome BUT technically, it's not a spec-compliant as webkitSpeechRecognition is not a thing. I still think there's value testing implementation with webkitSpeechRecognition but I'd love your thoughts on this.

@evanbliu
Copy link
Collaborator

Blink owners have requested that we drop the "webkit" prefix from the Web Speech API implementation in Chrome. We'll have to maintain backwards compatibility indefinitely, but eventually we'll probably pretend like it doesn't exist.

index.bs Outdated Show resolved Hide resolved
@beaufortfrancois
Copy link
Contributor Author

I've addressed @evanbliu's feedback and resolved conflicts. Let me know if there's more to address or if we can merge it.

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.

3 participants