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.

Sign up for GitHub

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

Jump to bottom

Updates to InlineAudio to support tts autoplay #39159

Merged
merged 1 commit into from Feb 24, 2021
Merged

Updates to InlineAudio to support tts autoplay #39159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
90 changes: 85 additions & 5 deletions apps/src/templates/instructions/InlineAudio.jsx
View file
Open in desktop
Expand Up @@ -82,6 +82,20 @@ const styles = {
}
};

// pulled from the example here https://developers.google.com/web/updates/2018/11/web-audio-autoplay
const AUDIO_ENABLING_DOM_EVENTS = [
'click',
'contextmenu',
'auxclick',
'dblclick',
'mousedown',
'mouseup',
'pointerup',
'touchend',
'keydown',
'keyup'
];

class InlineAudio extends React.Component {
static propTypes = {
assetUrl: PropTypes.func.isRequired,
Expand All @@ -90,6 +104,11 @@ class InlineAudio extends React.Component {
src: PropTypes.string,
message: PropTypes.string,
style: PropTypes.object,
ttsAutoplayEnabled: PropTypes.bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added, but not being set to true anywhere in the code yet


// when we need to wait for DOM event to trigger audio autoplay
// this is the element ID that we'll be listening to
autoplayTriggerElementId: PropTypes.string,

// Provided by redux
// To Log TTS usage
Expand All @@ -103,11 +122,26 @@ class InlineAudio extends React.Component {
playing: false,
error: false,
hover: false,
loaded: false
loaded: false,
autoplayed: false
};

constructor(props) {
super(props);
this.autoplayAudio = this.autoplayAudio.bind(this);
this.autoplayTriggerElement = null;
}

componentDidMount() {
this.getAudioElement();
if (this.props.ttsAutoplayEnabled && !this.state.autoplayed) {
const {autoplayTriggerElementId} = this.props;
this.autoplayTriggerElement = autoplayTriggerElementId
Copy link
Contributor

Choose a reason for hiding this comment

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

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

? document.getElementById(autoplayTriggerElementId)
: document;

this.playAudio();
}
}

componentWillUpdate(nextProps) {
Expand Down Expand Up @@ -152,7 +186,8 @@ class InlineAudio extends React.Component {

audio.addEventListener('ended', e => {
this.setState({
playing: false
playing: false,
autoplayed: this.props.ttsAutoplayEnabled
});
});

Expand Down Expand Up @@ -193,9 +228,7 @@ class InlineAudio extends React.Component {
this.state.playing ? this.pauseAudio() : this.playAudio();
};

playAudio() {
this.getAudioElement().play();
this.setState({playing: true});
recordPlayEvent() {
firehoseClient.putRecord({
study: 'tts-play',
study_group: 'v1',
Expand All @@ -210,6 +243,49 @@ class InlineAudio extends React.Component {
});
}

// adds event listeners to the DOM which trigger audio
// when a significant enough user interaction has happened
addAudioAutoplayTrigger() {
AUDIO_ENABLING_DOM_EVENTS.forEach(event => {
this.autoplayTriggerElement.addEventListener(event, this.autoplayAudio);
});
}

removeAudioAutoplayTrigger() {
AUDIO_ENABLING_DOM_EVENTS.forEach(event => {
this.autoplayTriggerElement.removeEventListener(
event,
this.autoplayAudio
);
});
}

playAudio() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if playAudio() is called more than once? (I think there are some potential race conditions where this might happen?)

return this.getAudioElement()
.play()
.then(() => {
this.setState({playing: true});
this.recordPlayEvent();
})
.catch(err => {
const shouldAutoPlay =
this.props.ttsAutoplayEnabled && !this.state.autoplayed;

// there wasn't significant enough user interaction to play audio automatically
// for more information about this issue on Chrome, see
// https://developers.google.com/web/updates/2017/09/autoplay-policy-changes
if (err instanceof DOMException && shouldAutoPlay) {
this.addAudioAutoplayTrigger();
} else {
throw err;
}
});
}

autoplayAudio() {
this.playAudio().then(() => this.removeAudioAutoplayTrigger());
}

pauseAudio() {
this.getAudioElement().pause();
this.setState({playing: false});
Expand Down Expand Up @@ -276,6 +352,10 @@ class InlineAudio extends React.Component {
}
}

InlineAudio.defaultProps = {
ttsAutoplayEnabled: false
};

export const StatelessInlineAudio = Radium(InlineAudio);
export default connect(function propsFromStore(state) {
return {
Expand Down
42 changes: 39 additions & 3 deletions apps/test/unit/templates/instructions/InlineAudioTest.js
View file
Open in desktop
Expand Up @@ -14,7 +14,14 @@ const DEFAULT_PROPS = {
style: {
button: {},
buttonImg: {}
}
},
ttsAutoplayEnabled: false
};

// this is a helper function which is used in a test to
// wait for all preceeding promises to resolve
const waitForPromises = async () => {
return Promise.resolve();
};

describe('InlineAudio', function() {
Expand Down Expand Up @@ -104,16 +111,43 @@ describe('InlineAudio', function() {
expect(component.exists('.inline-audio')).to.be.true;
});

it('can toggle audio', function() {
it('can toggle audio', async function() {
const component = mount(<StatelessInlineAudio {...DEFAULT_PROPS} />);

expect(component.state().playing).to.be.false;
component.instance().toggleAudio();
await waitForPromises();
expect(component.state().playing).to.be.true;
component.instance().toggleAudio();
await waitForPromises();
expect(component.state().playing).to.be.false;
});

it('autoplays if autoplay of text-to-speech is enabled', async function() {
maureensturgeon marked this conversation as resolved.
Show resolved Hide resolved
const component = mount(
<StatelessInlineAudio
assetUrl={function() {}}
ttsAutoplayEnabled={true}
/>
);

await waitForPromises();
expect(component.state().playing).to.be.true;
});

it('when playAudio resolves, state.playing set to true', async () => {
const component = mount(
<StatelessInlineAudio
assetUrl={function() {}}
ttsAutoplayEnabled={false}
/>
);

expect(component.state().playing).to.be.false;
await component.instance().playAudio();
expect(component.state().playing).to.be.true;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

it('only initializes Audio once', function() {
sinon.spy(window, 'Audio');
const component = mount(<StatelessInlineAudio {...DEFAULT_PROPS} />);
Expand Down Expand Up @@ -148,7 +182,9 @@ describe('InlineAudio', function() {
// Could extend this to have real EventTarget behavior,
// then write tests for 'ended' and 'error' events.
class FakeAudio {
play() {}
play() {
return Promise.resolve();
}
pause() {}
load() {}
// EventTarget interface
Expand Down