Skip to content

Commit

Permalink
Fix: Set media viewers' settings menu dimensions with javascript (#116)
Browse files Browse the repository at this point in the history 
This fixes a bug for when the localized items in the menu would not fit
in the menu's fixed width size set by CSS. This will also pave the way
for adding more dynamic submenus, e.g. when there are a variable number
of subtitles or audio tracks.

We could have made the width/height dynamically adjust using width=auto
or height=auto. But then CSS transitions don't work. So I resort to
a small amount of javascript in this commit to adjust dimensions
dynamically. Also, in order to have labels and values all be vertically
aligned as a column, I use CSS to give a table layout (display: table).
This brings an additional advantage for screen-readers - the label text
(e.g. SPEED) is no longer seen as in-line with the value (e.g. Normal),
so screen-readers will read them as separate words.
  • Loading branch information
@bhh1988 @tonyjin
bhh1988 authored and tonyjin committed May 16, 2017
1 parent 3ae639a commit 3c36f0a
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 97 deletions.
30 changes: 25 additions & 5 deletions src/lib/viewers/media/Settings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const MEDIA_SPEEDS = [
];

const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
<div role="menu">
<div class="bp-media-settings-menu-main bp-media-settings-menu" role="menu">
<div class="bp-media-settings-item bp-media-settings-item-speed" data-type="speed" tabindex="0" role="menuitem" aria-haspopup="true">
<div class="bp-media-settings-label" aria-label="${__('media_speed')}">${__('media_speed')}</div>
<div class="bp-media-settings-value">${__('media_speed_normal')}</div>
Expand All @@ -32,7 +32,7 @@ const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
<div class="bp-media-settings-arrow">${ICON_ARROW_RIGHT}</div>
</div>
</div>
<div class="bp-media-settings-options-speed" role="menu">
<div class="bp-media-settings-menu-speed bp-media-settings-menu" role="menu">
<div class="bp-media-settings-sub-item bp-media-settings-sub-item-speed" data-type="menu" tabindex="0" role="menuitem" aria-haspopup="true">
<div class="bp-media-settings-arrow">${ICON_ARROW_LEFT}</div>
<div class="bp-media-settings-label" aria-label="${__('media_speed')}">${__('media_speed')}</div>
Expand Down Expand Up @@ -62,7 +62,7 @@ const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
<div class="bp-media-settings-value">2.0</div>
</div>
</div>
<div class="bp-media-settings-options-quality" role="menu">
<div class="bp-media-settings-menu-quality bp-media-settings-menu" role="menu">
<div class="bp-media-settings-sub-item bp-media-settings-sub-item-quality" data-type="menu" tabindex="0" role="menuitem" aria-haspopup="true">
<div class="bp-media-settings-arrow">${ICON_ARROW_LEFT}</div>
<div class="bp-media-settings-label" aria-label="${__('media_quality')}">${__('media_quality')}</div>
Expand Down Expand Up @@ -137,6 +137,8 @@ class Settings extends EventEmitter {
*/
reset() {
this.settingsEl.className = CLASS_SETTINGS;
const mainMenu = this.settingsEl.querySelector('.bp-media-settings-menu-main');
this.setMenuContainerDimensions(mainMenu);
}

/**
Expand Down Expand Up @@ -175,6 +177,22 @@ class Settings extends EventEmitter {
}
}

/**
* Set the menu dimensions depending on which menu is being shown
*
* @private
* @param {HTMLElement} menu - The menu/submenu to use for sizing the container
* @return {void}
*/
setMenuContainerDimensions(menu) {
// NOTE: need to explicitly set the dimensions in order to get css transitions. width=auto doesn't work with css transitions
this.settingsEl.style.width = `${menu.offsetWidth + 18}px`;
// height = n * $item-height + 2 * $padding (see Settings.scss)
// where n is the number of displayed items in the menu
const sumHeight = [].reduce.call(menu.children, (sum, child) => sum + child.offsetHeight, 0);
this.settingsEl.style.height = `${sumHeight + 16}px`;
}

/**
* Finds the parent node that has dataset type
*
Expand Down Expand Up @@ -203,7 +221,9 @@ class Settings extends EventEmitter {
* @return {void}
*/
showSubMenu(type) {
const subMenu = this.settingsEl.querySelector(`.bp-media-settings-menu-${type}`);
this.settingsEl.classList.add(`bp-media-settings-show-${type}`);
this.setMenuContainerDimensions(subMenu);
// Move focus to the currently selected value
const curSelectedOption = this.settingsEl.querySelector(`[data-type="${type}"]${SELECTOR_SETTINGS_SUB_ITEM}.${CLASS_SETTINGS_SELECTED}`);
curSelectedOption.focus();
Expand Down Expand Up @@ -382,10 +402,10 @@ class Settings extends EventEmitter {
}

/**
* Getter for the value of quality
* Getter for the value of visible
*
* @public
* @return {string} The quality
* @return {boolean} Whether the settings menu is open (visible) or not
*/
isVisible() {
return this.visible;
Expand Down
152 changes: 60 additions & 92 deletions src/lib/viewers/media/Settings.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,138 +8,117 @@ $item-hover-color: #f6fafd;
border-radius: 2px;
bottom: 60px;
color: $fours;
display: none;
display: inline-block;
font-size: 10px;
height: 2 * ($item-height + $padding + 1px);
line-height: $item-height - $padding;
margin: 0;
overflow: hidden;
line-height: 0; // override line-height (only affects svgs in the menu, since nothing else is inline/inline-block)
max-height: 210px;
max-width: 400px;
-ms-overflow-style: -ms-autohiding-scrollbar;
overflow-x: hidden;
overflow-y: scroll;
padding: $padding;
position: absolute;
right: 16px;
transition: width .2s, height .2s;
width: 148px;
visibility: hidden;

.bp-media-settings-is-open & {
display: block;
}

// For MP3 and MP4 we only have speed option
.bp-media-mp4 &,
.bp-media-mp3 & {
height: $item-height + (2 * ($padding + 1px));
visibility: visible;
}

.bp-media-mp3 & {
bottom: 65px;
right: 0;
}
}

&.bp-media-settings-show-speed,
&.bp-media-settings-show-quality {
/* stylelint-disable declaration-no-important */
height: ((4 * $item-height) + (2 * $padding)) !important;
width: (88px + (2 * $padding)) !important;
/* stylelint-enable declaration-no-important */
}
.bp-media-settings-menu {
display: table;
}

.bp-media-settings-menu-quality,
.bp-media-settings-menu-speed,
.bp-media-settings-show-speed .bp-media-settings-menu-main,
.bp-media-settings-show-quality .bp-media-settings-menu-main {
display: none;
}

&.bp-media-settings-show-speed {
/* stylelint-disable declaration-no-important */
height: ((7 * $item-height) + (2 * $padding)) !important;
/* stylelint-enable declaration-no-important */
.bp-media-settings-show-speed .bp-media-settings-menu-speed,
.bp-media-settings-show-quality .bp-media-settings-menu-quality {
display: table;
}

// For MP3 and MP4 we only have speed option
.bp-media-mp4,
.bp-media-mp3 {
.bp-media-settings-item-quality {
display: none;
}
}

.bp-media-settings-item,
.bp-media-settings-sub-item {
cursor: pointer;
display: table-row;
height: $item-height;
outline: 0 none; // will be overridden by our own
overflow: hidden;
padding: $padding / 2;
white-space: nowrap;
}

.bp-media-settings-item:hover,
.bp-media-settings-sub-item:hover {
background-color: $item-hover-color;
&:hover {
background-color: $item-hover-color;

.bp-media-settings-label {
color: lighten($blue-steel, 50%);
.bp-media-settings-label {
color: lighten($blue-steel, 50%);
}

.bp-media-settings-value {
color: $blue-steel;
}
}

.bp-media-settings-value {
color: $blue-steel;
.bp-has-keyboard-focus &:focus {
box-shadow: inset 0 0 0 1px fade-out($black, .75); // Need a blacker border to contrast against white background
}
}

.bp-media-settings-item .bp-media-settings-label,
.bp-media-settings-item .bp-media-settings-value,
.bp-media-settings-sub-item .bp-media-settings-label,
.bp-media-settings-sub-item .bp-media-settings-value {
display: inline-block;
.bp-media-settings-label,
.bp-media-settings-value {
display: table-cell;
padding: $padding / 2;
vertical-align: middle;
}

.bp-media-settings-item .bp-media-settings-label {
.bp-media-settings-label {
color: $downtown-grey;
font-size: 11px;
font-weight: 400;
padding-left: $padding / 2;
text-align: left;
text-transform: uppercase;
width: 55px;
}

.bp-media-settings-value {
color: $sunset-grey;
font-size: 12px;
text-align: right;

.bp-media-settings-item & {
color: $sunset-grey;
font-weight: 400;
width: 48px;
text-align: left;
}

.bp-media-settings-sub-item & {
width: 30px;
text-align: center;
}

.bp-media-settings-selected & {
color: $box-blue;
}
}

.bp-media-settings-sub-item .bp-media-settings-label {
color: $downtown-grey;
font-weight: 400;
text-align: center;
text-transform: uppercase;
width: 48px;
}

.bp-media-settings-options-quality,
.bp-media-settings-options-speed,
.bp-media-settings-show-speed .bp-media-settings-item,
.bp-media-settings-show-quality .bp-media-settings-item {
display: none;
}

.bp-media-settings-show-speed .bp-media-settings-options-speed,
.bp-media-settings-show-quality .bp-media-settings-options-quality {
display: block;
.bp-media-settings-value {
color: $sunset-grey;
font-size: 12px;
padding-left: 10px;
text-align: left;

.bp-media-settings-sub-item {
display: block;
.bp-media-settings-selected & {
color: $box-blue;
}
}

.bp-media-settings-arrow {
display: inline-block;
height: 18px;
display: table-cell;
vertical-align: middle;
width: 18px;

svg {
height: 18px;
Expand All @@ -148,32 +127,21 @@ $item-hover-color: #f6fafd;
}

.bp-media-settings-icon {
display: inline-block;
height: 16px;
display: table-cell;
text-align: center;
vertical-align: middle;
visibility: hidden;
width: 30px;

svg {
height: 16px;
visibility: hidden;
width: 16px;
}

.bp-media-settings-selected & {
.bp-media-settings-selected & svg {
visibility: visible;
}

&:hover svg {
fill: $white;
}
}

.bp-has-keyboard-focus {
.bp-media-settings-item,
.bp-media-settings-sub-item {
&:focus {
box-shadow: inset 0 0 0 1px fade-out($black, .75); // Need a blacker border to contrast against white background
}
}
}
17 changes: 17 additions & 0 deletions src/lib/viewers/media/__tests__/Settings-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,15 @@ describe('lib/viewers/media/Settings', () => {

expect(settings.settingsEl).to.have.class('bp-media-settings');
});

it('should reset the menu container dimensions', () => {
const mainMenu = settings.settingsEl.querySelector('.bp-media-settings-menu-main');
sandbox.stub(settings, 'setMenuContainerDimensions');

settings.reset();

expect(settings.setMenuContainerDimensions).to.be.calledWith(mainMenu);
});
});

describe('findParentDataType()', () => {
Expand Down Expand Up @@ -507,6 +516,14 @@ describe('lib/viewers/media/Settings', () => {

expect(document.activeElement).to.equal(selected);
});

it('should recompute the menu container dimensions', () => {
sandbox.stub(settings, 'setMenuContainerDimensions');

settings.showSubMenu('speed');

expect(settings.setMenuContainerDimensions).to.be.called;
});
});

describe('chooseOption()', () => {
Expand Down

0 comments on commit 3c36f0a

Please sign in to comment.