Updates to InlineAudio to support tts autoplay #39159
Conversation
maureensturgeon
force-pushed
the
LP-1561/inline-audio-changes
branch
2 times, most recently
from
a0437d9
to
349c4ad
Compare
| @@ -90,6 +104,11 @@ class InlineAudio extends React.Component { | |||
| src: PropTypes.string, | |||
| message: PropTypes.string, | |||
| style: PropTypes.object, | |||
| ttsAutoplayEnabled: PropTypes.bool, | |||
There was a problem hiding this comment.
This is added, but not being set to true anywhere in the code yet
| await component.instance().playAudio(); | ||
| expect(component.state().playing).to.be.true; | ||
| }); | ||
|
|
There was a problem hiding this comment.
I attempted for 2 days to figure out how to stub the DOMException and check that playing audio starts up after the first user interaction but was getting a lot of trouble with the promises involved in that code path and tests not waiting on promises. I involved other engineers and ultimately decided it was not worth the time it was taking. Please forgive this slight lack of coverage.
maureensturgeon
force-pushed
the
LP-1561/inline-audio-changes
branch
from
349c4ad
to
2aeced8
Compare
| componentDidMount() { | ||
| this.getAudioElement(); | ||
| if (this.props.ttsAutoplayEnabled && !this.state.autoplayed) { | ||
| const {autoplayTriggerElementId} = this.props; | ||
| this.autoplayTriggerElement = autoplayTriggerElementId |
There was a problem hiding this comment.
NIT: i'm personally of the opinion that we should declare all instance properties in or under the constructor (depending on whether or not they need access to this) even when they start out as null as a sort of "directory" for future devs, so i'd recommend adding autoplayTriggerElement = null around line 33
maureensturgeon
force-pushed
the
LP-1561/inline-audio-changes
branch
from
2aeced8
to
e0e4ce8
Compare
| }); | ||
| } | ||
|
|
||
| playAudio() { |
There was a problem hiding this comment.
What happens if playAudio() is called more than once? (I think there are some potential race conditions where this might happen?)
Adds a new prop,
ttsAutoplayEnabledto theInlineAudiocomponent. When true, the audio will automatically play when the component mounts. If the audio autoplay fails to play because of a DOMException, listeners will be added to the specified element if it exists (props. autoplayTriggerElementId) or document to play audio after user interaction.These changes are broken out from the larger group of changes needed to enable automatic TTS autoplay instructions. The full set of changes can be found here.
Links
Testing story
These changes were tested in the larger PR #38956. I turned on autoplay for a teacher's section, logged in as a student on Chrome, went to a lesson, checked that instructions were autoplaying. On reload there is a DOMException, I checked that interactions on the workspace trigger the instructions to play once.
I additionally verified on this branch that the InlineAudio still works with the new prop.
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: