Skip to content

Commit

Permalink
fix(dash): Disable Settings menu when HD is not available (#1406)
Browse files Browse the repository at this point in the history
  • Loading branch information
Conrad Chan authored Jun 29, 2021
1 parent 6291777 commit 778e455
Show file tree
Hide file tree
Showing 11 changed files with 125 additions and 39 deletions.
4 changes: 3 additions & 1 deletion src/lib/viewers/controls/media/MediaSettings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,15 @@ export type Props = Partial<AudioTracksProps> &
Partial<SettingsProps> &
Partial<SubtitlesProps> &
AutoplayProps &
RateProps & { className?: string };
RateProps & { className?: string; isHDSupported?: boolean };

export default function MediaSettings({
audioTrack,
audioTracks = [],
autoplay,
badge,
className,
isHDSupported,
onAudioTrackChange = noop,
onAutoplayChange,
onQualityChange,
Expand Down Expand Up @@ -61,6 +62,7 @@ export default function MediaSettings({
{quality && (
<Settings.MenuItem
data-testid="bp-media-settings-quality"
isDisabled={!isHDSupported}
label={__('media_quality')}
target={Menu.QUALITY}
value={getQualityLabel(quality)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ describe('MediaSettings', () => {
const wrapper = getWrapper({ quality: 'auto', onQualityChange: jest.fn() });
expect(wrapper.exists(MediaSettingsMenuQuality)).toBe(true);
});

test('should render with isDisabled based on isHDSupported prop', () => {
const wrapper = getWrapper({ isHDSupported: false, quality: 'auto', onQualityChange: jest.fn() });
expect(wrapper.find({ target: 'quality' }).prop('isDisabled')).toBe(true);
});
});

describe('subtitles menu', () => {
Expand Down
4 changes: 4 additions & 0 deletions src/lib/viewers/controls/settings/SettingsMenuItem.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
.bp-SettingsMenuItem {
@include bp-SettingsRow;

&[aria-disabled='true'] {
cursor: default;
}

&:hover {
background-color: $hover-blue-background;

Expand Down
12 changes: 9 additions & 3 deletions src/lib/viewers/controls/settings/SettingsMenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,28 @@ import './SettingsMenuItem.scss';
export type Props = React.RefAttributes<HTMLDivElement> & {
className?: string;
label: string;
isDisabled?: boolean;
target: Menu;
value: string;
};
export type Ref = HTMLDivElement;

function SettingsMenuItem(props: Props, ref: React.Ref<Ref>): JSX.Element {
const { className, label, target, value, ...rest } = props;
const { className, label, isDisabled = false, target, value, ...rest } = props;
const { setActiveMenu } = React.useContext(SettingsContext);

const handleClick = (): void => {
if (isDisabled) {
return;
}

setActiveMenu(target);
};

const handleKeydown = (event: React.KeyboardEvent<Ref>): void => {
const key = decodeKeydown(event);

if (key !== 'ArrowRight' && key !== 'Enter' && key !== 'Space') {
if (isDisabled || (key !== 'ArrowRight' && key !== 'Enter' && key !== 'Space')) {
return;
}

Expand All @@ -34,6 +39,7 @@ function SettingsMenuItem(props: Props, ref: React.Ref<Ref>): JSX.Element {
return (
<div
ref={ref}
aria-disabled={isDisabled}
aria-haspopup="true"
className={classNames('bp-SettingsMenuItem', className)}
onClick={handleClick}
Expand All @@ -47,7 +53,7 @@ function SettingsMenuItem(props: Props, ref: React.Ref<Ref>): JSX.Element {
</div>
<div className="bp-SettingsMenuItem-value">{value}</div>
<div className="bp-SettingsMenuItem-arrow">
<IconArrowRight24 height={18} width={18} />
{!isDisabled && <IconArrowRight24 height={18} width={18} />}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,47 @@ describe('SettingsMenuItem', () => {

expect(context.setActiveMenu).toBeCalledTimes(calledTimes);
});

test('should not set the active menu when clicked while disabled', () => {
const context = getContext();
const wrapper = getWrapper({ isDisabled: true }, context);

wrapper.simulate('click');

expect(context.setActiveMenu).not.toBeCalled();
});

test.each`
key
${'ArrowRight'}
${'Enter'}
${'Space'}
`('should not set the active menu when $key is pressed while disabled', ({ key }) => {
const context = getContext();
const wrapper = getWrapper({ isDisabled: true }, context);

wrapper.simulate('keydown', { key });

expect(context.setActiveMenu).not.toBeCalled();
});
});

describe('render', () => {
test('should return a valid wrapper', () => {
const wrapper = getWrapper();

expect(wrapper.getDOMNode()).toHaveClass('bp-SettingsMenuItem');
expect(wrapper.getDOMNode().getAttribute('aria-disabled')).toBe('false');
expect(wrapper.find('.bp-SettingsMenuItem-label').contains('Speed')).toBe(true);
expect(wrapper.find('.bp-SettingsMenuItem-value').contains('Normal')).toBe(true);
expect(wrapper.exists(IconArrowRight24)).toBe(true);
});

test('should render as disabled when isDisabled is true', () => {
const wrapper = getWrapper({ isDisabled: true });

expect(wrapper.getDOMNode().getAttribute('aria-disabled')).toBe('true');
expect(wrapper.exists(IconArrowRight24)).toBe(false);
});
});
});
2 changes: 2 additions & 0 deletions src/lib/viewers/media/DashControls.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default function DashControls({
currentTime,
durationTime,
isAutoGeneratedSubtitles,
isHDSupported,
isPlaying,
isPlayingHD,
onAudioTrackChange,
Expand Down Expand Up @@ -74,6 +75,7 @@ export default function DashControls({
autoplay={autoplay}
badge={isPlayingHD ? <HDBadge /> : undefined}
className="bp-DashControls-settings"
isHDSupported={isHDSupported}
onAudioTrackChange={onAudioTrackChange}
onAutoplayChange={onAutoplayChange}
onQualityChange={onQualityChange}
Expand Down
8 changes: 5 additions & 3 deletions src/lib/viewers/media/DashViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ class DashViewer extends VideoBaseViewer {

const isHDSupported = this.hdVideoId !== -1;
this.selectedQuality = isHDSupported ? this.cache.get('media-quality') || 'auto' : 'sd';
this.setQuality(this.selectedQuality);
this.setQuality(this.selectedQuality, false);
}

/**
Expand Down Expand Up @@ -1045,12 +1045,13 @@ class DashViewer extends VideoBaseViewer {
/**
* Updates the selected quality and updates the player accordingly
* @param {string} quality - 'sd', 'hd', or 'auto'
* @param {boolean} [saveToCache] - Whether to save this value to the cache, defaults to true
* @emits qualitychange
* @return {void}
*/
setQuality(quality) {
setQuality(quality, saveToCache = true) {
const newQuality = quality !== 'sd' && quality !== 'hd' ? 'auto' : quality;
this.cache.set('media-quality', newQuality, true);
this.cache.set('media-quality', newQuality, saveToCache);
this.selectedQuality = newQuality;

switch (newQuality) {
Expand Down Expand Up @@ -1147,6 +1148,7 @@ class DashViewer extends VideoBaseViewer {
currentTime={this.mediaEl.currentTime}
durationTime={this.mediaEl.duration}
isAutoGeneratedSubtitles={!!this.autoCaptionDisplayer}
isHDSupported={this.hdVideoId !== -1}
isPlaying={!this.mediaEl.paused}
isPlayingHD={this.isPlayingHD()}
onAudioTrackChange={this.setAudioTrack}
Expand Down
5 changes: 5 additions & 0 deletions src/lib/viewers/media/__tests__/DashControls-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ describe('DashControls', () => {
expect(wrapper.find(VolumeControls).prop('onVolumeChange')).toEqual(onVolumeChange);
});

test.each([true, false])('should set isHDSupported prop on MediaSettings as %s', isHDSupported => {
const wrapper = getWrapper({ isHDSupported });
expect(wrapper.find(MediaSettings).prop('isHDSupported')).toBe(isHDSupported);
});

test('should not pass along badge if not playing HD', () => {
const wrapper = getWrapper({ badge: <CustomBadge /> });
expect(wrapper.find(MediaSettings).prop('badge')).toBeUndefined();
Expand Down
11 changes: 8 additions & 3 deletions src/lib/viewers/media/__tests__/DashViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -752,14 +752,14 @@ describe('lib/viewers/media/DashViewer', () => {
dash.loadUIReact();

expect(dash.selectedQuality).toBe('sd');
expect(dash.setQuality).toBeCalledWith('sd');
expect(dash.setQuality).toBeCalledWith('sd', false);
});

test('should set quality to auto if HD is supported and cache has no entry', () => {
dash.loadUIReact();

expect(dash.selectedQuality).toBe('auto');
expect(dash.setQuality).toBeCalledWith('auto');
expect(dash.setQuality).toBeCalledWith('auto', false);
});

test('should set quality to cache value if HD is supported and cache has an entry', () => {
Expand All @@ -768,7 +768,7 @@ describe('lib/viewers/media/DashViewer', () => {
dash.loadUIReact();

expect(dash.selectedQuality).toBe('hd');
expect(dash.setQuality).toBeCalledWith('hd');
expect(dash.setQuality).toBeCalledWith('hd', false);
});
});

Expand Down Expand Up @@ -1638,6 +1638,11 @@ describe('lib/viewers/media/DashViewer', () => {
expect(dash.emit).toBeCalledWith('qualitychange', 'auto');
expect(dash.renderUI).toBeCalled();
});

test('should not save to cache if saveToCache is false', () => {
dash.setQuality('auto', false);
expect(dash.cache.set).toBeCalledWith('media-quality', 'auto', false);
});
});

describe('initSubtitles()', () => {
Expand Down
70 changes: 41 additions & 29 deletions test/integration/media/DashViewer.e2e.test.js
Original file line number Diff line number Diff line change
@@ -1,57 +1,69 @@
import {
runAudioTracksTests,
runBaseMediaSettingsTests,
runLowQualityMenuTests,
runQualityMenuTests,
runSubtitlesTests,
} from '../../support/mediaSettingsTests';

describe('Dash Viewer', () => {
const token = Cypress.env('ACCESS_TOKEN');
const fileIdVideo = Cypress.env('FILE_ID_VIDEO_SUBTITLES_TRACKS');
const fileIdVideoSmall = Cypress.env('FILE_ID_VIDEO_SMALL');

describe('Media Settings Controls', () => {
describe('Without react controls', () => {
beforeEach(() => {
cy.visit('/');
cy.showPreview(token, fileIdVideo, {
viewers: { Dash: { useReactControls: false } },
});
const setupTest = (fileId, useReactControls) => {
cy.visit('/');
cy.showPreview(token, fileId, {
viewers: { Dash: { useReactControls } },
});

cy.showMediaControls();
cy.showMediaControls();

// Open the menu
cy.getByTitle('Settings').click();
});
// Open the menu
cy.getByTitle('Settings').click();
};

runBaseMediaSettingsTests();
describe('HD Video with Subtitles', () => {
describe('Media Settings Controls', () => {
describe('Without react controls', () => {
beforeEach(() => setupTest(fileIdVideo, false));

runQualityMenuTests();
runBaseMediaSettingsTests();

runAudioTracksTests();
runQualityMenuTests();

runSubtitlesTests();
});
runAudioTracksTests();

runSubtitlesTests();
});

describe('With react controls', () => {
beforeEach(() => setupTest(fileIdVideo, true));

describe('With react controls', () => {
beforeEach(() => {
cy.visit('/');
cy.showPreview(token, fileIdVideo, {
viewers: { Dash: { useReactControls: true } },
});
runBaseMediaSettingsTests();

cy.showMediaControls();
runQualityMenuTests();

// Open the menu
cy.getByTitle('Settings').click();
runAudioTracksTests();

runSubtitlesTests();
});
});
});

runBaseMediaSettingsTests();
describe('Non HD Video', () => {
describe('Media Settings Controls', () => {
describe('Without react controls', () => {
beforeEach(() => setupTest(fileIdVideoSmall, false));

runQualityMenuTests();
runLowQualityMenuTests();
});

runAudioTracksTests();
describe('With react controls', () => {
beforeEach(() => setupTest(fileIdVideoSmall, true));

runSubtitlesTests();
runLowQualityMenuTests();
});
});
});
});
12 changes: 12 additions & 0 deletions test/support/mediaSettingsTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ export function runQualityMenuTests() {
});
}

export function runLowQualityMenuTests() {
describe('Non HD Video', () => {
it('Should not have the Quality settings menu enabled', () => {
cy.getByTestId('bp-media-settings-quality')
.contains('480p')
.click({ force: true });

cy.getByTestId('bp-media-controls-hd').should('not.be.visible');
});
});
}

export function runAudioTracksTests() {
describe('Audiotracks Menu', () => {
it('Should be able to change the Audiotrack setting', () => {
Expand Down

0 comments on commit 778e455

Please sign in to comment.